[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0
From: |
malc |
Subject: |
Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0 |
Date: |
Tue, 19 May 2009 18:48:25 +0400 (MSD) |
On Tue, 19 May 2009, Eduardo Habkost wrote:
> On Tue, May 19, 2009 at 06:06:56PM +0400, malc wrote:
> > On Tue, 19 May 2009, Markus Armbruster wrote:
> >
> > > malc <address@hidden> writes:
> > >
> >
> > [..snip..]
> >
> > > >> diff --git a/block-qcow2.c b/block-qcow2.c
> > > >> index 9aa7261..d4556ef 100644
> > > >> --- a/block-qcow2.c
> > > >> +++ b/block-qcow2.c
> > > >> @@ -1809,6 +1809,12 @@ static int
> > > >> qcow_read_snapshots(BlockDriverState *bs)
> > > >> int64_t offset;
> > > >> uint32_t extra_data_size;
> > > >>
> > > >> + if (!s->nb_snapshots) {
> > > >> + s->snapshots = NULL;
> > > >> + s->snapshots_size = 0;
> > > >> + return 0;
> > > >> + }
> > > >> +
> > > >> offset = s->snapshots_offset;
> > > >> s->snapshots = qemu_mallocz(s->nb_snapshots *
> > > >> sizeof(QCowSnapshot));
> > > >> if (!s->snapshots)
> > > >>
> > > >> Can't see what this hunk accomplishes. If we remove it, the loop
> > > >> rejects, and we thus execute:
> > > >>
> >
> > Once again, on Linux/GLIBC it will, on AIX it wont.
>
> Why not? It will. If nb_snapshots is 0, it won't enter the loop. The
> problem with that code was the "if (!s->snapshots)" check, not the
> qemu_mallocz(0) call.
Because qemu_mallocz on AIX will be terminated by oom_check.
>
>
> >
> > And FWIW despite behaviour of malloc(0) being marked as implementation
> > defined i have sa far was unable to find any documentaiton (Linux man
> > pages, GLIBC info files) witht the actual definition, unlike on AIX
> > where man pages make it crystal clear what happens.
>
> You don't need to have the exact behavior defined, as long as:
I certainly don't, the standard certainly says the implementation is
obliged to document it, that's what seprates implementation-defined
from unspecified behaviour.
> 1) You call free(p) later
> 2) You don't dereference the returned pointer (just like you can't
> dereference p[n] on a malloc(n) block)
> 3) You don't assume anything about the returned value when size==0
>
> My point is that this is valid malloc() usage, and there may be existing
> qemu code relying on that, and I don't see any reason to put a trap for
> code that would be valid malloc()/free() usage.
Okay, at this point we both expressed our points of view and have to
agree to disagree.
>
> >
> >
> <snip>
> > >
> > > Tries what? Passing zero to qemu_malloc()? That's legitimate. And
> > > with allocation functions that cannot return failure, it's hardly
> > > dangerous, isn't it?
> >
> > That's legitimate only if one writes unportable code targeting single
> > system and knowing how it was defined.
>
> No, that's legitimate and portable. You just can't assume anything about
> the returned value.
>
>
>
> > As for being dangerous, yes it
> > is: dereferencing the returned pointer, while UB, doesn't trigger a
> > SEGFAULT on, at least, this machine with Linux.
> >
> > > >> qemu_realloc() currently uses 1.
> >
> > void *qemu_realloc(void *ptr, size_t size)
> > {
> > if (size)
> > return oom_check(realloc(ptr, size));
> > else
> > return realloc(ptr, size);
> > }
> >
> > There is nothing implementation defined about realloc(whatever, 0), it
> > has a defined meaning in POSIX:
> > http://opengroup.org/onlinepubs/007908775/xsh/realloc.html
> >
> > So it doesn't use 1.
> >
>
> realloc() return value is specified exactly the same way malloc() is:
>
> "If size is 0, either a null pointer or a unique pointer that can be
> successfully passed to free() is returned."
Nope, quoting from above page:
If size is 0 and ptr is not a null pointer, the object pointed to is
freed.
>
>
> > > >>
> > > >> realloc(NULL, sz) is specified to be equivalent to malloc(sz). It
> > > >> would
> > > >> be kind of nice to keep that for qemu_realloc() and qemu_malloc().
> > > >>
> > > >
> > > > qemu_realloc shouldn't be called qemu_realloc if doesn't do that. The
> > > > part
> > > > about qemu_malloc escapes me.
> > >
> > > qemu_malloc() & friends never fail. Checking their value for failure is
> > > pointless. Therefore, 1. is practical.
> > >
> > > 2. is certainly practical as well.
> > >
> > > 3. is like 2, with the (size ? size : 1) pushed into callers. I find
> > > that mildly annoying.
> >
> > Huh, that's not at all what i proposed. What i had in mind is:
> >
> > void *qemu_malloc(size_t size)
> > {
> > if (!size) abort();
> > return oom_check(malloc(size));
> > }
>
> Understood. And that's exactly what I think we should not do.
>
>
--
mailto:address@hidden
- [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0, Eduardo Habkost, 2009/05/18
- Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0, malc, 2009/05/18
- Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0, Eduardo Habkost, 2009/05/18
- Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0, malc, 2009/05/18
- Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0, Markus Armbruster, 2009/05/19
- Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0, malc, 2009/05/19
- Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0, Markus Armbruster, 2009/05/19
- Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0, malc, 2009/05/19
- Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0, Eduardo Habkost, 2009/05/19
- Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0,
malc <=
- Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0, Eduardo Habkost, 2009/05/19
- Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0, malc, 2009/05/19
- Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0, Eduardo Habkost, 2009/05/19
- Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0, Jamie Lokier, 2009/05/19
- Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0, Eduardo Habkost, 2009/05/19
- Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0, Jamie Lokier, 2009/05/19
- Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0, Eduardo Habkost, 2009/05/19
- Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0, Jamie Lokier, 2009/05/19
- Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0, Markus Armbruster, 2009/05/19
- Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0, Eduardo Habkost, 2009/05/19