qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH] use qemu_malloc and friends consistently
Date: Fri, 29 May 2009 14:51:39 +0200
User-agent: Gnus/5.11 (Gnus v5.11) Emacs/22.3 (gnu/linux)

malc <address@hidden> writes:

> On Fri, 29 May 2009, Anthony Liguori wrote:
>
>> malc wrote:
>> > On Fri, 29 May 2009, Anthony Liguori wrote:
>> >
>> >
>> > Dereference of NULL is UB[1] and dereferencing result of malloc(1) will
>> > just plain work.
>> >
>>
>> So let's ignoring returning NULL as a possibility..
>>
>> >> Putting the abort() in there is going to introduce a ton of subtle bugs,
>> >> I vote for changing qemu_malloc() to have a sane behavior.
>> >>
>> >
>> > And those will be caught, given one a chance to analyze things, unlike
>> > head in the sand approach of hoping things would just work.
>> >
>> > After doing some research, after the aforementioned lengthy discussion,
>> > the only free OS that straight-forwardly described what it does was
>> > OpenBSD:
>> >
>> > http://www.openbsd.org/cgi-bin/man.cgi?query=malloc&apropos=0&sektion=0&manpath=OpenBSD+Current&arch=i386&format=html

Neat.

What happens when malloc(0) can't obtain space in "shared protected
pages"?  I figure it fails and returns NULL.

>> > P.S. So far the abort that went into qemu_malloc caught one usage of zero
>> >      allocation (once again coming from qcow2).'
>> >
>>
>> But the zero allocation isn't a bug if we return malloc(1).  This is a
>> common convention and while it may not be portable to every platform's
>> underlying malloc, we can make this convention portable with qemu_malloc().
>
> Yes we can. I argue that this is not a good convention.
>
>>
>> At the end of the day, the result is a harder to misuse qemu_malloc()
>> and that's a very good thing.  I don't want a user to "discover" a
>> non-portable use of malloc() while trying to do something important.
>
> Options:
>
> a. return NULL
> b. return malloc(1)
> c. abort
> d. do what OpenBSD does
>
> Pros/cons:
>
> a. Pros: Simple to implement
>          Matches one of original malloc behaviours
>
>    Cons: Observable
>          Dereference == UB
>          Breaks the assumption that qemu_malloc should never return NULL
>
>
> b. Pros: Simple to implement
>          Matches one of original malloc behaviours
>
>    Cons: Useless allocation
>          No guard against accidental dereferences
>          Might result in a return of a NULL
>
>
> c. Pros: Simple to implement
>          Helps in finding call-sites
>
>    Cons: Doesn't match match the standard prescribed behaviours
>          Will abort the application even if the call-site is prepared
>          to cope with the fact that it requested zero bytes
>
>
> d. Pros: Matches one of original malloc behaviours
>          Provides safety net against accidental dereferences
>
>    Cons: Not trivial to implement (and also complicates the whole family
>          qemu_realloc/qemu_free)
>          Doesn't help in identifying call-sites

           Might result in a return of a NULL

> In a nutshell what i argue is that, if someone doesn't need any memory
> it shouldn't be asking for it, and it's not that unlikely that the
> author never considered the possibility of his code requesting zero
> bytes of memory, so in my view helping to locate all offenders and
> auditing them is good enough reason for option c, that's why it's
> there in repository.

Do we want a drop-in replacement for malloc() or not?

If we do, then options a, b and d are fine[*].  c is not, because it
breaks working code.  What the programmer thought when he wrote the
working code is immaterial.


[*] None of them guarantee that malloc(0) != NULL always, although b and
d make it unlikely.




reply via email to

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