Patchwork Permit zero-sized qemu_malloc() & friends

login
register
mail settings
Submitter Markus Armbruster
Date Dec. 5, 2009, 1:55 p.m.
Message ID <m3zl5x1q5x.fsf@crossbow.pond.sub.org>
Download mbox | patch
Permalink /patch/40399/
State New
Headers show

Comments

Markus Armbruster - Dec. 5, 2009, 1:55 p.m.
Anthony Liguori <anthony@codemonkey.ws> writes:

> Markus Armbruster wrote:
>> Commit a7d27b53 made zero-sized allocations a fatal error, deviating
>> from ISO C's malloc() & friends.  Revert that, but take care never to
>> return a null pointer, like malloc() & friends may do (it's
>> implementation defined), because that's another source of bugs.
>>
>> Rationale: while zero-sized allocations might occasionally be a sign of
>> something going wrong, they can also be perfectly legitimate.  The
>> change broke such legitimate uses.  We've found and "fixed" at least one
>> of them already (commit eb0b64f7, also reverted by this patch), and
>> another one just popped up: the change broke qcow2 images with virtual
>> disk size zero, i.e. images that don't hold real data but only VM state
>> of snapshots.
>>   
>
> I still believe that it is poor practice to pass size==0 to *malloc().
> I think actively discouraging this in qemu is a good thing because
> it's a broken idiom.

What's the impact of such usage?  What would improve for users if it
were eradicated?  For developers?

I believe the answer the first two questions is "nothing in particular",
and the answer to the last one is "hassle".  But I'd be happy to see
*specific* examples (code, not hand-waving) for where I'm wrong.

I'm opposed to "fixing" something without a real gain, not just because
it's work, but also because like any change it's going to introduce new
bugs.

I'm particularly critical of outlawing zero size for uses like
malloc(N*sizeof(T).  These are fairly common in our code.  Having to add
special treatment for N==0 is a trap for the unwary.  Which means we'll
never be done chasing this stuff.  Moreover, the special treatment
clutters the code, and requiring it without a clear benefit is silly.
Show me the benefit, and I'll happily reconsider.

For a specific example, let's look at the first example from my commit
message again: load_elf32(), load_elf64() in hw/elf_ops.h.  "Fix" for
its "broken" usage of qemu_mallocz(), shot from the hip, entirely
untested:


Anyone fancy to review a hundred or two of these?

And what would it buy us?  Let's see what the old code does:

* It mallocs a buffer for the PHT.

* It seeks to the PHT, and reads it.

* It maybe byte-swaps the PHT.

* It iterates over the PHT to load segments.

Works just fine for empty PHTs and empty segments.

The new code does slightly less work for empty PHTs and segments (rare
case), and very slightly more work for non-empty ones (common case).
Neither of it matters.  What matters is that it is nested two levels
deeper.

Benefit?

> That said, we've had this change in for a while and it's painfully
> obviously we haven't eliminated all of these instances in qemu.
> Knowing that we still have occurrences of this and actively exit()'ing
> when they happen is pretty much suicidal in a production environment.

Agreed.

> It's not a bug, it's just a poor practice.
>
> Here's what I propose.  I think we should introduce a
> CONFIG_DEBUG_ZERO_MALLOC.  When this is set, we should
> assert(size!=0).  Otherwise, we should return malloc(1) as Markus'
> patch does below.
>
> Using the same rules as we follow for -Werror, we should enable
> CONFIG_DEBUG_ZERO_MALLOC for the development tree and disable it for
> any releases.
>
> This helps us make qemu better during development while not
> unnecessarily causing us harm in a production environment.  What
> happens here long term I think remains to be seen.  But for right now,
> I think this is an important change to make for 0.12.

We'll clutter the code, and we'll forever chase the uses of malloc()
that haven't received their clutter, yet.  CONFIG_DEBUG_ZERO_MALLOC will
exist forever, and be universally off in production builds.

I'd like to see some real benefit for that, please.
Laurent Desnogues - Dec. 5, 2009, 2:14 p.m.
Hello,

probably a stupid remark, but I'll do it anyway :-)

Let's assume there are some contexts where doing a memory
allocation with a size of 0 is invalid while in some other contexts
it is valid.  Wouldn't it make sense in that case to have two
functions that do memory allocation?

Of course such a split would require full review of existing code
and might introduce complexities for instance for realloc (was
the original memory block allocated with a possible size of 0
or not?).


Laurent
malc - Dec. 5, 2009, 5:08 p.m.
On Sat, 5 Dec 2009, Markus Armbruster wrote:

> Anthony Liguori <anthony@codemonkey.ws> writes:
> 
> > Markus Armbruster wrote:
> >> Commit a7d27b53 made zero-sized allocations a fatal error, deviating
> >> from ISO C's malloc() & friends.  Revert that, but take care never to
> >> return a null pointer, like malloc() & friends may do (it's
> >> implementation defined), because that's another source of bugs.
> >>
> >> Rationale: while zero-sized allocations might occasionally be a sign of
> >> something going wrong, they can also be perfectly legitimate.  The
> >> change broke such legitimate uses.  We've found and "fixed" at least one
> >> of them already (commit eb0b64f7, also reverted by this patch), and
> >> another one just popped up: the change broke qcow2 images with virtual
> >> disk size zero, i.e. images that don't hold real data but only VM state
> >> of snapshots.
> >>   
> >
> > I still believe that it is poor practice to pass size==0 to *malloc().
> > I think actively discouraging this in qemu is a good thing because
> > it's a broken idiom.
> 
> What's the impact of such usage?  What would improve for users if it
> were eradicated?  For developers?
> 
> I believe the answer the first two questions is "nothing in particular",
> and the answer to the last one is "hassle".  But I'd be happy to see
> *specific* examples (code, not hand-waving) for where I'm wrong.
> 
> I'm opposed to "fixing" something without a real gain, not just because
> it's work, but also because like any change it's going to introduce new
> bugs.
> 
> I'm particularly critical of outlawing zero size for uses like
> malloc(N*sizeof(T).  These are fairly common in our code.  Having to add
> special treatment for N==0 is a trap for the unwary.  Which means we'll
> never be done chasing this stuff.  Moreover, the special treatment
> clutters the code, and requiring it without a clear benefit is silly.
> Show me the benefit, and I'll happily reconsider.
> 
> For a specific example, let's look at the first example from my commit
> message again: load_elf32(), load_elf64() in hw/elf_ops.h.  "Fix" for
> its "broken" usage of qemu_mallocz(), shot from the hip, entirely
> untested:

Excellent example, now consider this:

  read$ cat << eof | gcc -x c -
  #define _GNU_SOURCE
  #include <err.h>
  #include <unistd.h>
  #include <stdlib.h>
  #include <fcntl.h>

  int main (void)
  {
      int fd = open ("/dev/zero", 0);
      int ret;
      void *p = (void *) -1;

      if (fd == -1) err (1, "open");
      ret = read (fd, p, 0);
      if (ret != 0) err (1, "read");
      return 0;
  }
  eof
  read$ ./a.out                
  a.out: read: Bad address

Even though that linux's read(2) man page claims [1]:

DESCRIPTION
       read()  attempts to read up to count bytes from file descriptor fd into
       the buffer starting at buf.

       If count is zero, read() returns zero and has  no  other  results.   If
       count is greater than SSIZE_MAX, the result is unspecified.

[..snip..]

P.S. It would be interesting to know how this code behaves under OpenBSD, with
     p = malloc (0);

[1] As does, in essence, http://www.opengroup.org/onlinepubs/7990989775/xsh/read.html
Avi Kivity - Dec. 5, 2009, 5:23 p.m.
On 12/05/2009 07:08 PM, malc wrote:
>
>> What's the impact of such usage?  What would improve for users if it
>> were eradicated?  For developers?
>>
>> I believe the answer the first two questions is "nothing in particular",
>> and the answer to the last one is "hassle".  But I'd be happy to see
>> *specific* examples (code, not hand-waving) for where I'm wrong.
>>
>> I'm opposed to "fixing" something without a real gain, not just because
>> it's work, but also because like any change it's going to introduce new
>> bugs.
>>
>> I'm particularly critical of outlawing zero size for uses like
>> malloc(N*sizeof(T).  These are fairly common in our code.  Having to add
>> special treatment for N==0 is a trap for the unwary.  Which means we'll
>> never be done chasing this stuff.  Moreover, the special treatment
>> clutters the code, and requiring it without a clear benefit is silly.
>> Show me the benefit, and I'll happily reconsider.
>>
>> For a specific example, let's look at the first example from my commit
>> message again: load_elf32(), load_elf64() in hw/elf_ops.h.  "Fix" for
>> its "broken" usage of qemu_mallocz(), shot from the hip, entirely
>> untested:
>>      
> Excellent example, now consider this:
>
>    read$ cat<<  eof | gcc -x c -
>    #define _GNU_SOURCE
>    #include<err.h>
>    #include<unistd.h>
>    #include<stdlib.h>
>    #include<fcntl.h>
>
>    int main (void)
>    {
>        int fd = open ("/dev/zero", 0);
>        int ret;
>        void *p = (void *) -1;
>
>        if (fd == -1) err (1, "open");
>        ret = read (fd, p, 0);
>        if (ret != 0) err (1, "read");
>        return 0;
>    }
>    eof
>    read$ ./a.out
>    a.out: read: Bad address
>
>    

I don't see how this is relevant.  Linux' read(2) may or may not be 
broken, but what does that have to do with breaking callers of 
qemu_malloc() for certain use cases?
Reimar Döffinger - Dec. 5, 2009, 6:30 p.m.
On Sat, Dec 05, 2009 at 08:08:27PM +0300, malc wrote:
>       ret = read (fd, p, 0);
>       if (ret != 0) err (1, "read");
>       return 0;
>   }
>   eof
>   read$ ./a.out                
>   a.out: read: Bad address
> 
> Even though that linux's read(2) man page claims [1]:
> 
> DESCRIPTION
>        read()  attempts to read up to count bytes from file descriptor fd into
>        the buffer starting at buf.
> 
>        If count is zero, read() returns zero and has  no  other  results.   If
>        count is greater than SSIZE_MAX, the result is unspecified.
> 
> [..snip..]
> 
> P.S. It would be interesting to know how this code behaves under OpenBSD, with
>      p = malloc (0);
> 
> [1] As does, in essence, http://www.opengroup.org/onlinepubs/7990989775/xsh/read.html

Which simply means that the linux man page was based on SUSv2 which obviously was quite badly written.
SUSv3 gets it right (proving along the way it is better to use the POSIX man pages, i.e. "man 3 read").
It might be good to suggest them to update the linux man page though.
Markus Armbruster - Dec. 6, 2009, 7:57 a.m.
malc <av1474@comtv.ru> writes:

> On Sat, 5 Dec 2009, Markus Armbruster wrote:
>
>> Anthony Liguori <anthony@codemonkey.ws> writes:
>> 
>> > Markus Armbruster wrote:
>> >> Commit a7d27b53 made zero-sized allocations a fatal error, deviating
>> >> from ISO C's malloc() & friends.  Revert that, but take care never to
>> >> return a null pointer, like malloc() & friends may do (it's
>> >> implementation defined), because that's another source of bugs.
>> >>
>> >> Rationale: while zero-sized allocations might occasionally be a sign of
>> >> something going wrong, they can also be perfectly legitimate.  The
>> >> change broke such legitimate uses.  We've found and "fixed" at least one
>> >> of them already (commit eb0b64f7, also reverted by this patch), and
>> >> another one just popped up: the change broke qcow2 images with virtual
>> >> disk size zero, i.e. images that don't hold real data but only VM state
>> >> of snapshots.
>> >>   
>> >
>> > I still believe that it is poor practice to pass size==0 to *malloc().
>> > I think actively discouraging this in qemu is a good thing because
>> > it's a broken idiom.
>> 
>> What's the impact of such usage?  What would improve for users if it
>> were eradicated?  For developers?
>> 
>> I believe the answer the first two questions is "nothing in particular",
>> and the answer to the last one is "hassle".  But I'd be happy to see
>> *specific* examples (code, not hand-waving) for where I'm wrong.
>> 
>> I'm opposed to "fixing" something without a real gain, not just because
>> it's work, but also because like any change it's going to introduce new
>> bugs.
>> 
>> I'm particularly critical of outlawing zero size for uses like
>> malloc(N*sizeof(T).  These are fairly common in our code.  Having to add
>> special treatment for N==0 is a trap for the unwary.  Which means we'll
>> never be done chasing this stuff.  Moreover, the special treatment
>> clutters the code, and requiring it without a clear benefit is silly.
>> Show me the benefit, and I'll happily reconsider.
>> 
>> For a specific example, let's look at the first example from my commit
>> message again: load_elf32(), load_elf64() in hw/elf_ops.h.  "Fix" for
>> its "broken" usage of qemu_mallocz(), shot from the hip, entirely
>> untested:
>
> Excellent example, now consider this:
>
>   read$ cat << eof | gcc -x c -
>   #define _GNU_SOURCE
>   #include <err.h>
>   #include <unistd.h>
>   #include <stdlib.h>
>   #include <fcntl.h>
>
>   int main (void)
>   {
>       int fd = open ("/dev/zero", 0);
>       int ret;
>       void *p = (void *) -1;
>
>       if (fd == -1) err (1, "open");
>       ret = read (fd, p, 0);
>       if (ret != 0) err (1, "read");
>       return 0;
>   }
>   eof
>   read$ ./a.out                
>   a.out: read: Bad address
>
> Even though that linux's read(2) man page claims [1]:
>
> DESCRIPTION
>        read()  attempts to read up to count bytes from file descriptor fd into
>        the buffer starting at buf.
>
>        If count is zero, read() returns zero and has  no  other  results.   If
>        count is greater than SSIZE_MAX, the result is unspecified.
>
> [..snip..]
>
> P.S. It would be interesting to know how this code behaves under OpenBSD, with
>      p = malloc (0);
>
> [1] As does, in essence, http://www.opengroup.org/onlinepubs/7990989775/xsh/read.html

Replace "p = (void *)-1" by "p = NULL" and it works just fine.

malloc() either returns a valid pointer or NULL, so what read() does for
other pointers doesn't matter.

qemu_malloc() always returns a valid pointer, but that's not even needed
in this case.
malc - Dec. 6, 2009, 8:39 a.m.
On Sun, 6 Dec 2009, Markus Armbruster wrote:

> malc <av1474@comtv.ru> writes:
>
> > On Sat, 5 Dec 2009, Markus Armbruster wrote:
> >
> >> Anthony Liguori <anthony@codemonkey.ws> writes:
> >>
> >> > Markus Armbruster wrote:
> >> >> Commit a7d27b53 made zero-sized allocations a fatal error, deviating
> >> >> from ISO C's malloc() & friends.  Revert that, but take care never to
> >> >> return a null pointer, like malloc() & friends may do (it's
> >> >> implementation defined), because that's another source of bugs.
> >> >>
> >> >> Rationale: while zero-sized allocations might occasionally be a sign of
> >> >> something going wrong, they can also be perfectly legitimate.  The
> >> >> change broke such legitimate uses.  We've found and "fixed" at least one
> >> >> of them already (commit eb0b64f7, also reverted by this patch), and
> >> >> another one just popped up: the change broke qcow2 images with virtual
> >> >> disk size zero, i.e. images that don't hold real data but only VM state
> >> >> of snapshots.
> >> >>
> >> >

[..snip..]


> >
> > P.S. It would be interesting to know how this code behaves under OpenBSD, with
> >      p = malloc (0);
> >
> > [1] As does, in essence, http://www.opengroup.org/onlinepubs/7990989775/xsh/read.html
>
> Replace "p = (void *)-1" by "p = NULL" and it works just fine.
>

That's why i asked for somone to run it on OpenBSD:

Quoting http://www.openbsd.org/cgi-bin/man.cgi?query=malloc&apropos=0&sektion=3&manpath=OpenBSD+Current&arch=i386&format=html

  Allocation of a zero size object returns a pointer to a zero size object.
  This zero size object is access protected, so any access to it will gen-
  erate an exception (SIGSEGV).  Many zero-sized objects can be placed con-
  secutively in shared protected pages.  The minimum size of the protection
  on each object is suitably aligned and sized as previously stated, but
  the protection may extend further depending on where in a protected zone
  the object lands.

> malloc() either returns a valid pointer or NULL, so what read() does for
> other pointers doesn't matter.

Replace "returns" with "should" and this still won't match the wording
of the standard, besides as "read" behaviour on Linux shows following
those are not high on the agenda.

      7.20.3.3  The malloc function

       Synopsis

       [#1]

               #include <stdlib.h>
               void *malloc(size_t size);

       Description

       [#2] The malloc function allocates space for an object whose
       size  is specified by size and whose value is indeterminate.

       Returns

       [#3] The malloc function returns either a null pointer or  a
       pointer to the allocated space.

I don't know what a "valid pointer" in this context represents.

> qemu_malloc() always returns a valid pointer, but that's not even needed
> in this case.
>

--
mailto:av1474@comtv.ru
Markus Armbruster - Dec. 6, 2009, 8:59 a.m.
malc <av1474@comtv.ru> writes:

> On Sun, 6 Dec 2009, Markus Armbruster wrote:
>
>> malc <av1474@comtv.ru> writes:
>>
>> > On Sat, 5 Dec 2009, Markus Armbruster wrote:
>> >
>> >> Anthony Liguori <anthony@codemonkey.ws> writes:
>> >>
>> >> > Markus Armbruster wrote:
>> >> >> Commit a7d27b53 made zero-sized allocations a fatal error, deviating
>> >> >> from ISO C's malloc() & friends.  Revert that, but take care never to
>> >> >> return a null pointer, like malloc() & friends may do (it's
>> >> >> implementation defined), because that's another source of bugs.
>> >> >>
>> >> >> Rationale: while zero-sized allocations might occasionally be a sign of
>> >> >> something going wrong, they can also be perfectly legitimate.  The
>> >> >> change broke such legitimate uses.  We've found and "fixed" at least one
>> >> >> of them already (commit eb0b64f7, also reverted by this patch), and
>> >> >> another one just popped up: the change broke qcow2 images with virtual
>> >> >> disk size zero, i.e. images that don't hold real data but only VM state
>> >> >> of snapshots.
>> >> >>
>> >> >
>
> [..snip..]
>
>
>> >
>> > P.S. It would be interesting to know how this code behaves under OpenBSD, with
>> >      p = malloc (0);
[...]
>>
>> Replace "p = (void *)-1" by "p = NULL" and it works just fine.
>>
>
> That's why i asked for somone to run it on OpenBSD:
>
> Quoting http://www.openbsd.org/cgi-bin/man.cgi?query=malloc&apropos=0&sektion=3&manpath=OpenBSD+Current&arch=i386&format=html
>
>   Allocation of a zero size object returns a pointer to a zero size object.
>   This zero size object is access protected, so any access to it will gen-
>   erate an exception (SIGSEGV).  Many zero-sized objects can be placed con-
>   secutively in shared protected pages.  The minimum size of the protection
>   on each object is suitably aligned and sized as previously stated, but
>   the protection may extend further depending on where in a protected zone
>   the object lands.

read(fd, malloc(0), 0) is just fine, because read() doesn't touch the
buffer when the size is zero.

>> malloc() either returns a valid pointer or NULL, so what read() does for
>> other pointers doesn't matter.
>
> Replace "returns" with "should" and this still won't match the wording
> of the standard, besides as "read" behaviour on Linux shows following
> those are not high on the agenda.
>
>       7.20.3.3  The malloc function
>
>        Synopsis
>
>        [#1]
>
>                #include <stdlib.h>
>                void *malloc(size_t size);
>
>        Description
>
>        [#2] The malloc function allocates space for an object whose
>        size  is specified by size and whose value is indeterminate.
>
>        Returns
>
>        [#3] The malloc function returns either a null pointer or  a
>        pointer to the allocated space.
>
> I don't know what a "valid pointer" in this context represents.

I can talk standardese, if you prefer :)

malloc() either returns either a null pointer or a pointer to the
allocated space.  In either case, you must not dereference the pointer.

OpenBSD chooses to return a pointer to the allocated space.  It chooses
to catch common ways to dereference the pointer.

Your "p = (void *)-1" is neither a null pointer nor can it point to
allocated space on your particular system.  Hence, it cannot be a value
of malloc() for any argument, and therefore what read() does with it on
that particular system doesn't matter.
Blue Swirl - Dec. 6, 2009, 9:02 a.m.
On Sun, Dec 6, 2009 at 10:39 AM, malc <av1474@comtv.ru> wrote:
> On Sun, 6 Dec 2009, Markus Armbruster wrote:
>
>> malc <av1474@comtv.ru> writes:
>>
>> > On Sat, 5 Dec 2009, Markus Armbruster wrote:
>> >
>> >> Anthony Liguori <anthony@codemonkey.ws> writes:
>> >>
>> >> > Markus Armbruster wrote:
>> >> >> Commit a7d27b53 made zero-sized allocations a fatal error, deviating
>> >> >> from ISO C's malloc() & friends.  Revert that, but take care never to
>> >> >> return a null pointer, like malloc() & friends may do (it's
>> >> >> implementation defined), because that's another source of bugs.
>> >> >>
>> >> >> Rationale: while zero-sized allocations might occasionally be a sign of
>> >> >> something going wrong, they can also be perfectly legitimate.  The
>> >> >> change broke such legitimate uses.  We've found and "fixed" at least one
>> >> >> of them already (commit eb0b64f7, also reverted by this patch), and
>> >> >> another one just popped up: the change broke qcow2 images with virtual
>> >> >> disk size zero, i.e. images that don't hold real data but only VM state
>> >> >> of snapshots.
>> >> >>
>> >> >
>
> [..snip..]
>
>
>> >
>> > P.S. It would be interesting to know how this code behaves under OpenBSD, with
>> >      p = malloc (0);
>> >
>> > [1] As does, in essence, http://www.opengroup.org/onlinepubs/7990989775/xsh/read.html
>>
>> Replace "p = (void *)-1" by "p = NULL" and it works just fine.
>>
>
> That's why i asked for somone to run it on OpenBSD:

$ cat mall.c
#define _GNU_SOURCE
#include <err.h>
#include <unistd.h>
#include <stdlib.h>
#include <fcntl.h>
#include <stdio.h>

int main (void)
{
    int fd = open ("/dev/zero", 0);
    int ret;
#if 0
    void *p = (void *) -1;
#else
    void *p = malloc(0);
#endif

    fprintf(stderr, "ptr %p\n", p);
    if (fd == -1) err (1, "open");
    ret = read (fd, p, 0);
    if (ret != 0) err (1, "read");
    return 0;
}
$ gcc mall.c
$ ./a.out
ptr 0x46974060
$

Changing read count to 1:
$ ./a.out
ptr 0x41ce0070
a.out: read: Bad address
malc - Dec. 6, 2009, 10:02 a.m.
On Sun, 6 Dec 2009, Blue Swirl wrote:

> On Sun, Dec 6, 2009 at 10:39 AM, malc <av1474@comtv.ru> wrote:
> > On Sun, 6 Dec 2009, Markus Armbruster wrote:

[..snip..]

> $ gcc mall.c
> $ ./a.out
> ptr 0x46974060
> $
> 
> Changing read count to 1:
> $ ./a.out
> ptr 0x41ce0070
> a.out: read: Bad address
> 

Thanks.
malc - Dec. 6, 2009, 10:22 a.m.
On Sun, 6 Dec 2009, Markus Armbruster wrote:

> malc <av1474@comtv.ru> writes:
> 

[..snip..]

> 
> read(fd, malloc(0), 0) is just fine, because read() doesn't touch the
> buffer when the size is zero.
> 

[..snip..]

Yet under linux the address is checked even for zero case.

> >
> > I don't know what a "valid pointer" in this context represents.
> 
> I can talk standardese, if you prefer :)
> 
> malloc() either returns either a null pointer or a pointer to the
> allocated space.  In either case, you must not dereference the pointer.
> 
> OpenBSD chooses to return a pointer to the allocated space.  It chooses
> to catch common ways to dereference the pointer.
> 
> Your "p = (void *)-1" is neither a null pointer nor can it point to
> allocated space on your particular system.  Hence, it cannot be a value
> of malloc() for any argument, and therefore what read() does with it on
> that particular system doesn't matter.
> 

Here, i believe, you are inventing artificial restrictions on how
malloc behaves, i don't see anything that prevents the implementor
from setting aside a range of addresses with 31st bit set as an
indicator of "zero" allocations, and then happily giving it to the
user of malloc and consumming it in free.
Avi Kivity - Dec. 6, 2009, 10:40 a.m.
On 12/06/2009 12:22 PM, malc wrote:
> Here, i believe, you are inventing artificial restrictions on how
> malloc behaves, i don't see anything that prevents the implementor
> from setting aside a range of addresses with 31st bit set as an
> indicator of "zero" allocations, and then happily giving it to the
> user of malloc and consumming it in free.
>    

The implementation needs to track which addresses it handed out, since 
it is required that malloc(0) != malloc(0) (unless both are NULL).
Markus Armbruster - Dec. 6, 2009, 11:10 a.m.
malc <av1474@comtv.ru> writes:

> On Sun, 6 Dec 2009, Markus Armbruster wrote:
>
>> malc <av1474@comtv.ru> writes:
>> 
>
> [..snip..]
>
>> 
>> read(fd, malloc(0), 0) is just fine, because read() doesn't touch the
>> buffer when the size is zero.
>> 
>
> [..snip..]
>
> Yet under linux the address is checked even for zero case.

Any value you can obtain from malloc() passes that check.

Why does the fact that you can construct pointers that don't pass this
check matter for our discussion of malloc()?

>> > I don't know what a "valid pointer" in this context represents.
>> 
>> I can talk standardese, if you prefer :)
>> 
>> malloc() either returns either a null pointer or a pointer to the
>> allocated space.  In either case, you must not dereference the pointer.
>> 
>> OpenBSD chooses to return a pointer to the allocated space.  It chooses
>> to catch common ways to dereference the pointer.
>> 
>> Your "p = (void *)-1" is neither a null pointer nor can it point to
>> allocated space on your particular system.  Hence, it cannot be a value
>> of malloc() for any argument, and therefore what read() does with it on
>> that particular system doesn't matter.
>> 
>
> Here, i believe, you are inventing artificial restrictions on how
> malloc behaves, i don't see anything that prevents the implementor
> from setting aside a range of addresses with 31st bit set as an
> indicator of "zero" allocations, and then happily giving it to the
> user of malloc and consumming it in free.

Misunderstanding?  Such behavior is indeed permissible, and I can't see
where I restricted it away.  An implementation that behaves as you
describe returns "pointer to allocated space".  That the pointer has
some funny bit set doesn't matter.  That it can't be dereferenced is
just fine.

I'm not sure what your point is.  If it is that malloc(0) can return a
value that cannot be passed to a zero-sized read(), then I fear you have
not made your point.
Paolo Bonzini - Dec. 6, 2009, 11:35 a.m.
On 12/06/2009 11:22 AM, malc wrote:
> Here, i believe, you are inventing artificial restrictions on how
> malloc behaves, i don't see anything that prevents the implementor
> from setting aside a range of addresses with 31st bit set as an
> indicator of "zero" allocations, and then happily giving it to the
> user of malloc and consumming it in free.

But it has to make it a valid address anyway.  If a zero-sized read 
treats it as invalid (SIGSEGV, EFAULT, whatever), malloc has failed to 
return a valid address and is not obeying its specification.

Paolo
malc - Dec. 6, 2009, 11:53 a.m.
On Sun, 6 Dec 2009, Avi Kivity wrote:

> On 12/06/2009 12:22 PM, malc wrote:
> > Here, i believe, you are inventing artificial restrictions on how
> > malloc behaves, i don't see anything that prevents the implementor
> > from setting aside a range of addresses with 31st bit set as an
> > indicator of "zero" allocations, and then happily giving it to the
> > user of malloc and consumming it in free.
> >    
> 
> The implementation needs to track which addresses it handed out, since it is
> required that malloc(0) != malloc(0) (unless both are NULL).

You haven't read carefully, i said range.
malc - Dec. 6, 2009, noon
On Sun, 6 Dec 2009, Markus Armbruster wrote:

> malc <av1474@comtv.ru> writes:
> 
> > On Sun, 6 Dec 2009, Markus Armbruster wrote:
> >
> >> malc <av1474@comtv.ru> writes:
> >> 
> >
> > [..snip..]
> >
> >> 
> >> read(fd, malloc(0), 0) is just fine, because read() doesn't touch the
> >> buffer when the size is zero.
> >> 
> >
> > [..snip..]
> >
> > Yet under linux the address is checked even for zero case.
> 
> Any value you can obtain from malloc() passes that check.
> 
> Why does the fact that you can construct pointers that don't pass this
> check matter for our discussion of malloc()?
> 
> >> > I don't know what a "valid pointer" in this context represents.
> >> 
> >> I can talk standardese, if you prefer :)
> >> 
> >> malloc() either returns either a null pointer or a pointer to the
> >> allocated space.  In either case, you must not dereference the pointer.
> >> 
> >> OpenBSD chooses to return a pointer to the allocated space.  It chooses
> >> to catch common ways to dereference the pointer.
> >> 
> >> Your "p = (void *)-1" is neither a null pointer nor can it point to
> >> allocated space on your particular system.  Hence, it cannot be a value
> >> of malloc() for any argument, and therefore what read() does with it on
> >> that particular system doesn't matter.
> >> 
> >
> > Here, i believe, you are inventing artificial restrictions on how
> > malloc behaves, i don't see anything that prevents the implementor
> > from setting aside a range of addresses with 31st bit set as an
> > indicator of "zero" allocations, and then happily giving it to the
> > user of malloc and consumming it in free.
> 
> Misunderstanding?  Such behavior is indeed permissible, and I can't see
> where I restricted it away.  An implementation that behaves as you
> describe returns "pointer to allocated space".  That the pointer has
> some funny bit set doesn't matter.  That it can't be dereferenced is
> just fine.
> 
> I'm not sure what your point is.  If it is that malloc(0) can return a
> value that cannot be passed to a zero-sized read(), then I fear you have
> not made your point.

One more attempt to make it clearer. If you agree that this behaviour
is permissible then the game is lost as things stand now under Linux,
since replacing [1]:

void *p = (void *) -1 
with:
void *p = (void *) 0x80000000

or anything else with said bit set will yield EFAULT. Consequently the
code you cited as a well behaving malloc(0) call site will bomb.

[1] Under 32bit Linux that is, with the usual split.
malc - Dec. 6, 2009, 12:02 p.m.
On Sun, 6 Dec 2009, Paolo Bonzini wrote:

> On 12/06/2009 11:22 AM, malc wrote:
> > Here, i believe, you are inventing artificial restrictions on how
> > malloc behaves, i don't see anything that prevents the implementor
> > from setting aside a range of addresses with 31st bit set as an
> > indicator of "zero" allocations, and then happily giving it to the
> > user of malloc and consumming it in free.
> 
> But it has to make it a valid address anyway.  If a zero-sized read treats it
> as invalid (SIGSEGV, EFAULT, whatever), malloc has failed to return a valid
> address and is not obeying its specification.

Once again - standard doesn't speak about "valid addresses".
Avi Kivity - Dec. 6, 2009, 12:07 p.m.
On 12/06/2009 01:53 PM, malc wrote:
> On Sun, 6 Dec 2009, Avi Kivity wrote:
>
>    
>> On 12/06/2009 12:22 PM, malc wrote:
>>      
>>> Here, i believe, you are inventing artificial restrictions on how
>>> malloc behaves, i don't see anything that prevents the implementor
>>> from setting aside a range of addresses with 31st bit set as an
>>> indicator of "zero" allocations, and then happily giving it to the
>>> user of malloc and consumming it in free.
>>>
>>>        
>> The implementation needs to track which addresses it handed out, since it is
>> required that malloc(0) != malloc(0) (unless both are NULL).
>>      
> You haven't read carefully, i said range.
>    

I did in fact.  Consider a loop

   malloc(0);
   p = malloc(0);
   while (1) {
       n = malloc(0);
       free(p);
       p = n;
   }

without some form of tracking, you won't be able to return unique 
addresses eventually.
malc - Dec. 6, 2009, 12:11 p.m.
On Sun, 6 Dec 2009, Avi Kivity wrote:

> On 12/06/2009 01:53 PM, malc wrote:
> > On Sun, 6 Dec 2009, Avi Kivity wrote:
> > 
> >    
> > > On 12/06/2009 12:22 PM, malc wrote:
> > >      
> > > > Here, i believe, you are inventing artificial restrictions on how
> > > > malloc behaves, i don't see anything that prevents the implementor
> > > > from setting aside a range of addresses with 31st bit set as an
> > > > indicator of "zero" allocations, and then happily giving it to the
> > > > user of malloc and consumming it in free.
> > > > 
> > > >        
> > > The implementation needs to track which addresses it handed out, since it
> > > is
> > > required that malloc(0) != malloc(0) (unless both are NULL).
> > >      
> > You haven't read carefully, i said range.
> >    
> 
> I did in fact.  Consider a loop
> 
>   malloc(0);
>   p = malloc(0);
>   while (1) {
>       n = malloc(0);
>       free(p);
>       p = n;
>   }
> 
> without some form of tracking, you won't be able to return unique addresses
> eventually.

So? There will be tracking, it's the same as with OpenBSD where
allocations of zero and greater than zero are handled differently.
Avi Kivity - Dec. 6, 2009, 12:23 p.m.
On 12/06/2009 02:11 PM, malc wrote:
>>>> The implementation needs to track which addresses it handed out, since it
>>>> is
>>>> required that malloc(0) != malloc(0) (unless both are NULL).
>>>>
>>>>          
>>> You haven't read carefully, i said range.
>>>
>>>        
>>
> So? There will be tracking, it's the same as with OpenBSD where
> allocations of zero and greater than zero are handled differently.
>    


That's exactly what I said.  Some tracking is needed.
Paolo Bonzini - Dec. 6, 2009, 4:23 p.m.
On 12/06/2009 01:02 PM, malc wrote:
> On Sun, 6 Dec 2009, Paolo Bonzini wrote:
>
>> On 12/06/2009 11:22 AM, malc wrote:
>>> Here, i believe, you are inventing artificial restrictions on how
>>> malloc behaves, i don't see anything that prevents the implementor
>>> from setting aside a range of addresses with 31st bit set as an
>>> indicator of "zero" allocations, and then happily giving it to the
>>> user of malloc and consumming it in free.
>>
>> But it has to make it a valid address anyway.  If a zero-sized read treats it
>> as invalid (SIGSEGV, EFAULT, whatever), malloc has failed to return a valid
>> address and is not obeying its specification.
>
> Once again - standard doesn't speak about "valid addresses".

For that matter, POSIX doesn't mention EFAULT at all, and doesn't 
include detecting "valid addresses" among the things that read can do 
before returning 0.  So if an OS extends POSIX with EFAULT, it had 
better provide a malloc that is consistent with whatever definition of 
"valid address" EFAULT uses.  While if it doesn't provide EFAULT, read 
should return 0 for the OS to be conforming to POSIX, and the whole 
discussion is moot.

Paolo
Paolo Bonzini - Dec. 6, 2009, 4:23 p.m.
On 12/06/2009 01:00 PM, malc wrote:
> or anything else with said bit set will yield EFAULT. Consequently the
> code you cited as a well behaving malloc(0) call site will bomb.

It is not well-behaving unless the definition of EFAULT is changed 
consistently.

Paolo
Kevin Wolf - Dec. 7, 2009, 8:35 a.m.
Am 06.12.2009 13:00, schrieb malc:
> On Sun, 6 Dec 2009, Markus Armbruster wrote:
> 
>> malc <av1474@comtv.ru> writes:
>>
>>> On Sun, 6 Dec 2009, Markus Armbruster wrote:
>>>
>>>> malc <av1474@comtv.ru> writes:
>>>>
>>>
>>> [..snip..]
>>>
>>>>
>>>> read(fd, malloc(0), 0) is just fine, because read() doesn't touch the
>>>> buffer when the size is zero.
>>>>
>>>
>>> [..snip..]
>>>
>>> Yet under linux the address is checked even for zero case.
>>
>> Any value you can obtain from malloc() passes that check.
>>
>> Why does the fact that you can construct pointers that don't pass this
>> check matter for our discussion of malloc()?
>>
>>>>> I don't know what a "valid pointer" in this context represents.
>>>>
>>>> I can talk standardese, if you prefer :)
>>>>
>>>> malloc() either returns either a null pointer or a pointer to the
>>>> allocated space.  In either case, you must not dereference the pointer.
>>>>
>>>> OpenBSD chooses to return a pointer to the allocated space.  It chooses
>>>> to catch common ways to dereference the pointer.
>>>>
>>>> Your "p = (void *)-1" is neither a null pointer nor can it point to
>>>> allocated space on your particular system.  Hence, it cannot be a value
>>>> of malloc() for any argument, and therefore what read() does with it on
>>>> that particular system doesn't matter.
>>>>
>>>
>>> Here, i believe, you are inventing artificial restrictions on how
>>> malloc behaves, i don't see anything that prevents the implementor
>>> from setting aside a range of addresses with 31st bit set as an
>>> indicator of "zero" allocations, and then happily giving it to the
>>> user of malloc and consumming it in free.
>>
>> Misunderstanding?  Such behavior is indeed permissible, and I can't see
>> where I restricted it away.  An implementation that behaves as you
>> describe returns "pointer to allocated space".  That the pointer has
>> some funny bit set doesn't matter.  That it can't be dereferenced is
>> just fine.
>>
>> I'm not sure what your point is.  If it is that malloc(0) can return a
>> value that cannot be passed to a zero-sized read(), then I fear you have
>> not made your point.
> 
> One more attempt to make it clearer. If you agree that this behaviour
> is permissible then the game is lost as things stand now under Linux,
> since replacing [1]:
> 
> void *p = (void *) -1 
> with:
> void *p = (void *) 0x80000000

Then this implementation of malloc(0) isn't suitable for a libc running
on Linux - despite its general permissibility. You have a point if, and
only if, you can reproduce such misbehaviour with a real malloc of a
real libc that isn't constructed to work against the kernel but to fit
it. So far I can't see that you have found any.

By the way, this whole discussion about theoretical malloc(0) return
values doesn't even matter here. The patch replaces it by malloc(1),
with which we both are very confident that you can pass its return value
to read for reading 0 bytes, right?

Kevin
Markus Armbruster - Dec. 7, 2009, 9:42 a.m.
malc <av1474@comtv.ru> writes:

> On Sun, 6 Dec 2009, Markus Armbruster wrote:
>
>> malc <av1474@comtv.ru> writes:
>> 
>> > On Sun, 6 Dec 2009, Markus Armbruster wrote:
>> >
>> >> malc <av1474@comtv.ru> writes:
>> >> 
>> >
>> > [..snip..]
>> >
>> >> 
>> >> read(fd, malloc(0), 0) is just fine, because read() doesn't touch the
>> >> buffer when the size is zero.
>> >> 
>> >
>> > [..snip..]
>> >
>> > Yet under linux the address is checked even for zero case.
>> 
>> Any value you can obtain from malloc() passes that check.
>> 
>> Why does the fact that you can construct pointers that don't pass this
>> check matter for our discussion of malloc()?
>> 
>> >> > I don't know what a "valid pointer" in this context represents.
>> >> 
>> >> I can talk standardese, if you prefer :)
>> >> 
>> >> malloc() either returns either a null pointer or a pointer to the
>> >> allocated space.  In either case, you must not dereference the pointer.
>> >> 
>> >> OpenBSD chooses to return a pointer to the allocated space.  It chooses
>> >> to catch common ways to dereference the pointer.
>> >> 
>> >> Your "p = (void *)-1" is neither a null pointer nor can it point to
>> >> allocated space on your particular system.  Hence, it cannot be a value
>> >> of malloc() for any argument, and therefore what read() does with it on
>> >> that particular system doesn't matter.
>> >> 
>> >
>> > Here, i believe, you are inventing artificial restrictions on how
>> > malloc behaves, i don't see anything that prevents the implementor
>> > from setting aside a range of addresses with 31st bit set as an
>> > indicator of "zero" allocations, and then happily giving it to the
>> > user of malloc and consumming it in free.
>> 
>> Misunderstanding?  Such behavior is indeed permissible, and I can't see
>> where I restricted it away.  An implementation that behaves as you
>> describe returns "pointer to allocated space".  That the pointer has
>> some funny bit set doesn't matter.  That it can't be dereferenced is
>> just fine.
>> 
>> I'm not sure what your point is.  If it is that malloc(0) can return a
>> value that cannot be passed to a zero-sized read(), then I fear you have
>> not made your point.
>
> One more attempt to make it clearer. If you agree that this behaviour
> is permissible then the game is lost as things stand now under Linux,
> since replacing [1]:
>
> void *p = (void *) -1 
> with:
> void *p = (void *) 0x80000000
>
> or anything else with said bit set will yield EFAULT. Consequently the
> code you cited as a well behaving malloc(0) call site will bomb.
>
> [1] Under 32bit Linux that is, with the usual split.

You can't just pull pointers out of your ear and expect stuff to work.

malloc() is free to return a pointer to allocated space that is set up
in a way that catches access beyond the allocated size.  OpenBSD does
that for size zero; it allocates one byte then, from pages that are used
only for zero-sized allocations, and takes care to disable access to
these pages with mprotect(..., PROT_NONE)[*].  Since read(..., 0) does
not access beyond the allocated size, it still works just fine.

If you replace glibc's malloc() to get OpenBSD-like behavior, you can't
just make up some pointer to a memory area you believe to be unused, you
have to do it right, like OpenBSD does.


[*] Check out omalloc_make_chunks() at
http://www.openbsd.org/cgi-bin/cvsweb/src/lib/libc/stdlib/malloc.c?rev=1.121;content-type=text%2Fplain
malc - Dec. 7, 2009, 10 a.m.
On Mon, 7 Dec 2009, Markus Armbruster wrote:

> malc <av1474@comtv.ru> writes:
> 
> > On Sun, 6 Dec 2009, Markus Armbruster wrote:
> >
> >> malc <av1474@comtv.ru> writes:
> >> 
> >> > On Sun, 6 Dec 2009, Markus Armbruster wrote:
> >> >
> >> >> malc <av1474@comtv.ru> writes:
> >> >> 
> >> >
> >> > [..snip..]
> >> >
> >> >> 
> >> >> read(fd, malloc(0), 0) is just fine, because read() doesn't touch the
> >> >> buffer when the size is zero.
> >> >> 
> >> >
> >> > [..snip..]
> >> >
> >> > Yet under linux the address is checked even for zero case.
> >> 
> >> Any value you can obtain from malloc() passes that check.
> >> 
> >> Why does the fact that you can construct pointers that don't pass this
> >> check matter for our discussion of malloc()?
> >> 
> >> >> > I don't know what a "valid pointer" in this context represents.
> >> >> 
> >> >> I can talk standardese, if you prefer :)
> >> >> 
> >> >> malloc() either returns either a null pointer or a pointer to the
> >> >> allocated space.  In either case, you must not dereference the pointer.
> >> >> 
> >> >> OpenBSD chooses to return a pointer to the allocated space.  It chooses
> >> >> to catch common ways to dereference the pointer.
> >> >> 
> >> >> Your "p = (void *)-1" is neither a null pointer nor can it point to
> >> >> allocated space on your particular system.  Hence, it cannot be a value
> >> >> of malloc() for any argument, and therefore what read() does with it on
> >> >> that particular system doesn't matter.
> >> >> 
> >> >
> >> > Here, i believe, you are inventing artificial restrictions on how
> >> > malloc behaves, i don't see anything that prevents the implementor
> >> > from setting aside a range of addresses with 31st bit set as an
> >> > indicator of "zero" allocations, and then happily giving it to the
> >> > user of malloc and consumming it in free.
> >> 
> >> Misunderstanding?  Such behavior is indeed permissible, and I can't see
> >> where I restricted it away.  An implementation that behaves as you
> >> describe returns "pointer to allocated space".  That the pointer has
> >> some funny bit set doesn't matter.  That it can't be dereferenced is
> >> just fine.
> >> 

Here you agree that it's permissible.

> >> I'm not sure what your point is.  If it is that malloc(0) can return a
> >> value that cannot be passed to a zero-sized read(), then I fear you have
> >> not made your point.
> >
> > One more attempt to make it clearer. If you agree that this behaviour
> > is permissible then the game is lost as things stand now under Linux,
> > since replacing [1]:
> >
> > void *p = (void *) -1 
> > with:
> > void *p = (void *) 0x80000000
> >
> > or anything else with said bit set will yield EFAULT. Consequently the
> > code you cited as a well behaving malloc(0) call site will bomb.
> >
> > [1] Under 32bit Linux that is, with the usual split.
> 
> You can't just pull pointers out of your ear and expect stuff to work.

And here you don't. Which renders whole discussion rather pointless.

> 
> malloc() is free to return a pointer to allocated space that is set up
> in a way that catches access beyond the allocated size.  OpenBSD does
> that for size zero; it allocates one byte then, from pages that are used
> only for zero-sized allocations, and takes care to disable access to
> these pages with mprotect(..., PROT_NONE)[*].  Since read(..., 0) does
> not access beyond the allocated size, it still works just fine.
> 
> If you replace glibc's malloc() to get OpenBSD-like behavior, you can't
> just make up some pointer to a memory area you believe to be unused, you
> have to do it right, like OpenBSD does.
> 
> 
> [*] Check out omalloc_make_chunks() at
> http://www.openbsd.org/cgi-bin/cvsweb/src/lib/libc/stdlib/malloc.c?rev=1.121;content-type=text%2Fplain
>
Kevin Wolf - Dec. 7, 2009, 10:17 a.m.
Am 07.12.2009 11:00, schrieb malc:
>>>> Misunderstanding?  Such behavior is indeed permissible, and I can't see
>>>> where I restricted it away.  An implementation that behaves as you
>>>> describe returns "pointer to allocated space".  That the pointer has
>>>> some funny bit set doesn't matter.  That it can't be dereferenced is
>>>> just fine.
>>>>
> 
> Here you agree that it's permissible.

He's just differentiating between values that are generally permissible
in terms of the C standard...

>> You can't just pull pointers out of your ear and expect stuff to work.
> 
> And here you don't. Which renders whole discussion rather pointless.

...and values that can be used by a working implementation on Linux.
That's quite a difference. It is obvious that a libc implementation must
match the kernel, or nothing will work.

Kevin
Markus Armbruster - Dec. 7, 2009, 10:35 a.m.
malc <av1474@comtv.ru> writes:

> On Mon, 7 Dec 2009, Markus Armbruster wrote:
>
>> malc <av1474@comtv.ru> writes:
>> 
>> > On Sun, 6 Dec 2009, Markus Armbruster wrote:
>> >
>> >> malc <av1474@comtv.ru> writes:
>> >> 
>> >> > On Sun, 6 Dec 2009, Markus Armbruster wrote:
>> >> >
>> >> >> malc <av1474@comtv.ru> writes:
>> >> >> 
>> >> >
>> >> > [..snip..]
>> >> >
>> >> >> 
>> >> >> read(fd, malloc(0), 0) is just fine, because read() doesn't touch the
>> >> >> buffer when the size is zero.
>> >> >> 
>> >> >
>> >> > [..snip..]
>> >> >
>> >> > Yet under linux the address is checked even for zero case.
>> >> 
>> >> Any value you can obtain from malloc() passes that check.
>> >> 
>> >> Why does the fact that you can construct pointers that don't pass this
>> >> check matter for our discussion of malloc()?
>> >> 
>> >> >> > I don't know what a "valid pointer" in this context represents.
>> >> >> 
>> >> >> I can talk standardese, if you prefer :)
>> >> >> 
>> >> >> malloc() either returns either a null pointer or a pointer to the
>> >> >> allocated space.  In either case, you must not dereference the pointer.
>> >> >> 
>> >> >> OpenBSD chooses to return a pointer to the allocated space.  It chooses
>> >> >> to catch common ways to dereference the pointer.
>> >> >> 
>> >> >> Your "p = (void *)-1" is neither a null pointer nor can it point to
>> >> >> allocated space on your particular system.  Hence, it cannot be a value
>> >> >> of malloc() for any argument, and therefore what read() does with it on
>> >> >> that particular system doesn't matter.
>> >> >> 
>> >> >
>> >> > Here, i believe, you are inventing artificial restrictions on how
>> >> > malloc behaves, i don't see anything that prevents the implementor
>> >> > from setting aside a range of addresses with 31st bit set as an
>> >> > indicator of "zero" allocations, and then happily giving it to the
>> >> > user of malloc and consumming it in free.
>> >> 
>> >> Misunderstanding?  Such behavior is indeed permissible, and I can't see
>> >> where I restricted it away.  An implementation that behaves as you
>> >> describe returns "pointer to allocated space".  That the pointer has
>> >> some funny bit set doesn't matter.  That it can't be dereferenced is
>> >> just fine.
>> >> 
>
> Here you agree that it's permissible.

We were talking about ISO C, so by "implementation" I meant an
implementation of ISO C, not an application program using it.  Sorry if
I didn't make that sufficiently clear.

>> >> I'm not sure what your point is.  If it is that malloc(0) can return a
>> >> value that cannot be passed to a zero-sized read(), then I fear you have
>> >> not made your point.
>> >
>> > One more attempt to make it clearer. If you agree that this behaviour
>> > is permissible then the game is lost as things stand now under Linux,
>> > since replacing [1]:
>> >
>> > void *p = (void *) -1 
>> > with:
>> > void *p = (void *) 0x80000000
>> >
>> > or anything else with said bit set will yield EFAULT. Consequently the
>> > code you cited as a well behaving malloc(0) call site will bomb.
>> >
>> > [1] Under 32bit Linux that is, with the usual split.
>> 
>> You can't just pull pointers out of your ear and expect stuff to work.
>
> And here you don't. Which renders whole discussion rather pointless.

And here we're talking about making up pointers in a portable
application program.  Which QEMU is.

>                     Which renders whole discussion rather pointless.

It's only tangentially related to the question whether qemu_malloc()
should accept zero arguments anyway.

>> malloc() is free to return a pointer to allocated space that is set up
>> in a way that catches access beyond the allocated size.  OpenBSD does
>> that for size zero; it allocates one byte then, from pages that are used
>> only for zero-sized allocations, and takes care to disable access to
>> these pages with mprotect(..., PROT_NONE)[*].  Since read(..., 0) does
>> not access beyond the allocated size, it still works just fine.
>> 
>> If you replace glibc's malloc() to get OpenBSD-like behavior, you can't
>> just make up some pointer to a memory area you believe to be unused, you
>> have to do it right, like OpenBSD does.
>> 
>> 
>> [*] Check out omalloc_make_chunks() at
>> http://www.openbsd.org/cgi-bin/cvsweb/src/lib/libc/stdlib/malloc.c?rev=1.121;content-type=text%2Fplain

Patch

diff --git a/hw/elf_ops.h b/hw/elf_ops.h
index 6093dea..003d2ef 100644
--- a/hw/elf_ops.h
+++ b/hw/elf_ops.h
@@ -219,51 +219,53 @@  static int glue(load_elf, SZ)(const char *name, int fd, int64_t address_offset,
 
     glue(load_symbols, SZ)(&ehdr, fd, must_swab, clear_lsb);
 
-    size = ehdr.e_phnum * sizeof(phdr[0]);
-    lseek(fd, ehdr.e_phoff, SEEK_SET);
-    phdr = qemu_mallocz(size);
-    if (!phdr)
-        goto fail;
-    if (read(fd, phdr, size) != size)
-        goto fail;
-    if (must_swab) {
-        for(i = 0; i < ehdr.e_phnum; i++) {
-            ph = &phdr[i];
-            glue(bswap_phdr, SZ)(ph);
-        }
-    }
-
-    total_size = 0;
-    for(i = 0; i < ehdr.e_phnum; i++) {
-        ph = &phdr[i];
-        if (ph->p_type == PT_LOAD) {
-            mem_size = ph->p_memsz;
-            /* XXX: avoid allocating */
-            data = qemu_mallocz(mem_size);
-            if (ph->p_filesz > 0) {
-                if (lseek(fd, ph->p_offset, SEEK_SET) < 0)
-                    goto fail;
-                if (read(fd, data, ph->p_filesz) != ph->p_filesz)
-                    goto fail;
+    if (ehdr.e_phnum) {
+        size = ehdr.e_phnum * sizeof(phdr[0]);
+        lseek(fd, ehdr.e_phoff, SEEK_SET);
+        phdr = qemu_mallocz(size);
+        if (!phdr)
+            goto fail;
+        if (read(fd, phdr, size) != size)
+            goto fail;
+        if (must_swab) {
+            for(i = 0; i < ehdr.e_phnum; i++) {
+                ph = &phdr[i];
+                glue(bswap_phdr, SZ)(ph);
             }
-            /* address_offset is hack for kernel images that are
-               linked at the wrong physical address.  */
-            addr = ph->p_paddr + address_offset;
+        }
 
-            snprintf(label, sizeof(label), "phdr #%d: %s", i, name);
-            rom_add_blob_fixed(label, data, mem_size, addr);
+        total_size = 0;
+        for(i = 0; i < ehdr.e_phnum; i++) {
+            ph = &phdr[i];
+            if (ph->p_type == PT_LOAD) {
+                mem_size = ph->p_memsz;
+                if (memsize) {
+                    data = qemu_mallocz(mem_size);
+                    if (ph->p_filesz > 0) {
+                        if (lseek(fd, ph->p_offset, SEEK_SET) < 0)
+                            goto fail;
+                        if (read(fd, data, ph->p_filesz) != ph->p_filesz)
+                            goto fail;
+                    }
+                    /* address_offset is hack for kernel images that are
+                       linked at the wrong physical address.  */
+                    addr = ph->p_paddr + address_offset;
 
-            total_size += mem_size;
-            if (addr < low)
-                low = addr;
-            if ((addr + mem_size) > high)
-                high = addr + mem_size;
+                    snprintf(label, sizeof(label), "phdr #%d: %s", i, name);
+                    rom_add_blob_fixed(label, data, mem_size, addr);
 
-            qemu_free(data);
-            data = NULL;
+                    total_size += mem_size;
+                    qemu_free(data);
+                    data = NULL;
+                }
+                if (addr < low)
+                    low = addr;
+                if ((addr + mem_size) > high)
+                    high = addr + mem_size;
+            }
         }
+        qemu_free(phdr);
     }
-    qemu_free(phdr);
     if (lowaddr)
         *lowaddr = (uint64_t)(elf_sword)low;
     if (highaddr)