[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: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0 |
Date: |
Tue, 19 May 2009 11:28:47 -0300 |
User-agent: |
Mutt/1.5.18 (2008-05-17) |
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.
>
> 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:
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.
>
>
<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."
> > >>
> > >> 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.
--
Eduardo
- [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 <=
- 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, 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