qemu-devel
[Top][All Lists]
Advanced

[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: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH] fix qemu_malloc() error check for size==0
Date: Tue, 19 May 2009 18:09:44 +0200
User-agent: Gnus/5.11 (Gnus v5.11) Emacs/22.3 (gnu/linux)

malc <address@hidden> writes:

> On Tue, 19 May 2009, Markus Armbruster wrote:
>
>> malc <address@hidden> writes:
[...]
> Once again, on Linux/GLIBC it will, on AIX it wont.
>
> 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.

malloc(3) says:

       If size is 0, then malloc() returns either NULL, or a unique
       pointer value that can later be successfully passed to free().

Matches ISO C.  It's all you need to know to write portable programs.

[...]
>> >> >                                              IOW making qemu_malloc[z]
>> >> > return whatever the underlying system returns is just hiding the bugs,
>> >> > the code becomes unportable.
>> >> 
>> >> Matter of taste.
>> >> 
>> >> 1. Deal with the implementation-definedness.  Every caller that could
>> >>    pass zero needs to take care not to confuse empty allocation with an
>> >>    out of memory condition.
>> >> 
>> >>    This is easier than it sounds when you check for out of memory in
>> >>    just one place, like we do.
>> >> 
>> >> 2. Remove the implementation-definedness.  Easiest way is to detect zero
>> >>    size in a wrapper (for us: qemu_malloc()) and bump it to one.
>> >
>> > And mine:
>> >   3. Abort the program if somebody tries it. Because so far history thought
>> >      me that nobody does 1.
>> 
>> 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. 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.

malloc(0) is perfectly portable.  The fact that dereferencing the return
value may or may not fault doesn't change that.  What happens on
dereferencing of an invalid pointer is never portable.

>> >> 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.

Quoting from there:

    If ptr is a null pointer, realloc() behaves like malloc() for the
    specified size.

So, realloc(whatever, 0) uses 1. when malloc(0) does.

>> >> 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));
> }

Exactly.  Callers who used the fact that malloc(size) is perfectly
portable for size>=0 then have to use qemu_malloc(size ? size : 1).

>> I don't care whether we pick 1., 2. or 3., but I'd prefer we pick the
>> same for qemu_malloc() and qemu_realloc().  We currently use 1. for
>> qemu_realloc().  My point is: if you pick something else for
>> qemu_malloc(), please consider changing qemu_realloc().  That's all.
>
> I see.

Good.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]