Question regarding safety of a malloc()

Discussion in 'C Programming' started by Andrew Shepson, Jun 2, 2010.

  1. Hello group!

    i'm currently working on code where i find myself repeatedly having to
    allocating two objects to store some private data, e.g.:

    whio_stream * whio_stream_for_dev( struct whio_dev * dev, bool
    takeOwnership )
    {
    if( ! dev ) return 0;
    whio_stream * str = (whio_stream*)malloc( sizeof(whio_stream) );
    if( ! str ) return 0;
    *str = whio_stream_dev_init;
    whio_stream_dev * sd = (whio_stream_dev*) (str + sizeof
    (whio_stream));
    if( ! sd )
    {
    free(str);
    return 0;
    }
    str->implData = sd;
    sd->dev = dev;
    sd->ownsDev = takeOwnership;
    return str;

    }

    The significant bit there is the two mallocs. Today i figured i'd try
    to consolidate that, and turned the above code into:

    whio_stream * whio_stream_for_dev( struct whio_dev * dev, bool
    takeOwnership )
    {
    if( ! dev ) return 0;
    whio_stream * str = (whio_stream*)malloc( sizeof(whio_stream) +
    sizeof(whio_stream_dev) );
    if( ! str ) return 0;
    *str = whio_stream_dev_init;
    whio_stream_dev * sd = (whio_stream_dev*) (str + sizeof
    (whio_stream));
    str->implData = sd;
    sd->dev = dev;
    sd->ownsDev = takeOwnership;
    return str;

    }

    and i changed my cleanup routine to not free the sd object (since free
    (str) will now free that). That "seems to work", but now to my
    question:

    Is that second approach safe or am i just asking for alignment-related
    (or platform-specific) problems?

    If it is NOT safe, can you recommend a portable workaround which will
    achieve the goal of using only one malloc instead of two?

    Many thanks in advance for your insights!
     
    Andrew Shepson, Jun 2, 2010
    #1
    1. Advertising

  2. Andrew Shepson

    Rob Kendrick Guest

    On Wed, 2 Jun 2010 17:11:53 +0000 (UTC)
    Andrew Shepson <> wrote:

    > If it is NOT safe, can you recommend a portable workaround which will
    > achieve the goal of using only one malloc instead of two?


    My taste, should you really want to avoid the two malloc() calls, is to
    use a struct. Other tastes may differ.

    But to be honest, I'd just use two malloc() calls. C library
    allocators can be scarily efficient these days.

    B.
     
    Rob Kendrick, Jun 2, 2010
    #2
    1. Advertising

  3. Andrew Shepson

    Ben Pfaff Guest

    Andrew Shepson <> writes:

    > whio_stream * str = (whio_stream*)malloc( sizeof(whio_stream) +
    > sizeof(whio_stream_dev) );


    [...]

    > Is that second approach safe or am i just asking for alignment-related
    > (or platform-specific) problems?


    You are asking for alignment-related, platform-specific problems.

    > If it is NOT safe, can you recommend a portable workaround which will
    > achieve the goal of using only one malloc instead of two?


    I have two suggestions:

    * Use two separate malloc()s. Unless you know that this
    is too slow, e.g. through profiling, you are wasting
    your time trying to optimize it.

    * Embed the two structures into a larger struct, e.g.:

    struct pair {
    struct whio_stream stream;
    struct whio_stream_dev stream_dev;
    };

    Then you can allocate memory for the larger structure
    knowing that alignment will be correct.
    --
    char a[]="\n .CJacehknorstu";int putchar(int);int main(void){unsigned long b[]
    ={0x67dffdff,0x9aa9aa6a,0xa77ffda9,0x7da6aa6a,0xa67f6aaa,0xaa9aa9f6,0x11f6},*p
    =b,i=24;for(;p+=!*p;*p/=4)switch(0[p]&3)case 0:{return 0;for(p--;i--;i--)case+
    2:{i++;if(i)break;else default:continue;if(0)case 1:putchar(a[i&15]);break;}}}
     
    Ben Pfaff, Jun 2, 2010
    #3
  4. Andrew Shepson

    Eric Sosman Guest

    On 6/2/2010 1:11 PM, Andrew Shepson wrote:
    > Hello group!
    >
    > i'm currently working on code where i find myself repeatedly having to
    > allocating two objects to store some private data, e.g.:
    >
    > whio_stream * whio_stream_for_dev( struct whio_dev * dev, bool
    > takeOwnership )
    > {
    > if( ! dev ) return 0;
    > whio_stream * str = (whio_stream*)malloc( sizeof(whio_stream) );
    > if( ! str ) return 0;
    > *str = whio_stream_dev_init;
    > whio_stream_dev * sd = (whio_stream_dev*) (str + sizeof
    > (whio_stream));


    This is surely wrong -- but see below.

    > if( ! sd )
    > {
    > free(str);
    > return 0;
    > }
    > str->implData = sd;
    > sd->dev = dev;
    > sd->ownsDev = takeOwnership;
    > return str;
    >
    > }
    >
    > The significant bit there is the two mallocs.


    I see only one. Is this, perhaps, a neither-fish-nor-fowl
    jumble of the "before" and the "after" into a "don't go there?"

    > Today i figured i'd try
    > to consolidate that, and turned the above code into:
    >
    > whio_stream * whio_stream_for_dev( struct whio_dev * dev, bool
    > takeOwnership )
    > {
    > if( ! dev ) return 0;
    > whio_stream * str = (whio_stream*)malloc( sizeof(whio_stream) +
    > sizeof(whio_stream_dev) );
    > if( ! str ) return 0;
    > *str = whio_stream_dev_init;
    > whio_stream_dev * sd = (whio_stream_dev*) (str + sizeof
    > (whio_stream));


    This is wrong. Remember what happens when you add an integer
    to a pointer: You "step" in units of the pointed-to type. So if
    sizeof(whio_stream) is 12, say, you've got `str+12' and you're
    trying to produce an address 144 bytes beyond where `str' points.
    You could correct this by writing

    ...sd = (whio_stream_dev*)(str + 1);

    But that would quite likely be wrong, too! There's something
    called "alignment," which means that objects of certain types can
    only begin at addresses that are divisible (when considered as
    numbers) by certain divisors. Perhaps an `int' must begin on a
    4-byte boundary, or a `whio_stream' on an 8-byte boundary. It's
    up to the implementation to decide what types need alignment, and
    how much.

    ... and if `whio_stream' is 12 bytes long and needs 4-byte
    alignment (let's say), while `whio_stream_dev' is 8 bytes long
    and needs 8-byte alignment, the `sd' pointer will aim at a spot
    where it's not legal for a `whio_stream_dev' to begin. There's
    no telling what will happen: Your code might run incorrectly, or
    slowly, or not at all.

    Perhaps the simplest way to fix both problems is to use a
    one-off struct to hold both objects:

    struct {
    whio_stream str;
    /* compiler will insert padding here if needed */
    whio_stream_dev sd;
    } *ptr;
    ptr = malloc(sizeof *ptr);
    if (ptr == NULL) return 0;
    ptr->str = whio_stream_dev_init;
    ptr->str.implData = &ptr->sd;
    ptr->sd.dev = &ptr->dev;
    ptr->sd.ownsDev = takeOwnership;
    return &ptr->str;

    --
    Eric Sosman
    lid
     
    Eric Sosman, Jun 2, 2010
    #4
  5. On Wed, 2 Jun 2010 17:11:53 +0000 (UTC), Andrew Shepson
    <> wrote:

    >Hello group!
    >
    >i'm currently working on code where i find myself repeatedly having to
    >allocating two objects to store some private data, e.g.:


    'snip

    >The significant bit there is the two mallocs. Today i figured i'd try
    >to consolidate that, and turned the above code into:
    >
    >whio_stream * whio_stream_for_dev( struct whio_dev * dev, bool
    >takeOwnership )
    >{
    > if( ! dev ) return 0;
    > whio_stream * str = (whio_stream*)malloc( sizeof(whio_stream) +
    >sizeof(whio_stream_dev) );


    Consider the case where the type whio_stream consumes an odd multiple
    of four bytes and the type whio_stream_dev requires alignment on a
    multiple of 8. Clearly you are not allocating enough memory.

    And the cast serves no purpose.

    > if( ! str ) return 0;
    > *str = whio_stream_dev_init;


    *str has type whio_stream. The value you are assigning appears to
    have type whio_stream_dev. If not, you have a really poor naming
    convention and you will need to show us the declarations for the types
    as well as the objects used by the code.

    > whio_stream_dev * sd = (whio_stream_dev*) (str + sizeof
    >(whio_stream));


    str is a pointer to type whio_stream. Therefore, all address
    arithmetic based on this pointer will be performed in units of
    sizeof(whio_stream). Just as an example, if str points to 0x1000 and
    sizeof(whio_stream) is 16, then:

    malloc allocated 36 bytes of usable storage (0x1000 through
    0x1023).

    Ignoring the undefined behavior invoked by computing this value,
    sd now contains the value 0x1100 (0x1000 + 16*16 or
    0x1000 + 0x10*0x10) which is clearly not in the range of the allocated
    memory.

    You can solve the computation error with either

    whio_stream_dev * sd =
    (whio_stream_dev*) ((char*)str + sizeof(whio_stream));

    or the equivalent

    whio_stream_dev * sd = (whio_stream_dev*) (str + 1);

    but neither will resolve the alignment issue alluded to above (which
    will also invoke undefined behavior).

    > str->implData = sd;
    > sd->dev = dev;
    > sd->ownsDev = takeOwnership;
    > return str;
    >
    >}
    >
    >and i changed my cleanup routine to not free the sd object (since free
    >(str) will now free that). That "seems to work", but now to my
    >question:
    >
    >Is that second approach safe or am i just asking for alignment-related
    >(or platform-specific) problems?


    Not at all; yes you are.

    >If it is NOT safe, can you recommend a portable workaround which will
    >achieve the goal of using only one malloc instead of two?


    As others have mentioned, include both objects inside an envelope
    structure.

    What about two calls to malloc bothers you? Will you really be
    calling this function often and rapidly enough that it makes a
    difference?

    While you are at it, consider the issue of fragmented memory. If both
    types are very large and your program allocates and frees memory
    repeatedly for these and other types, it is not beyond reason that a
    large malloc will fail while two smaller ones could succeed.

    --
    Remove del for email
     
    Barry Schwarz, Jun 3, 2010
    #5
  6. Subject: Question regarding safety of a malloc()

    what does "safety" mean?


    On 2 June, 18:11, Andrew Shepson <> wrote:

    > Hello group!


    hello individual


    > i'm currently working on code where i find myself repeatedly having to
    > allocating two objects to store some private data, e.g.:
    >
    > whio_stream * whio_stream_for_dev( struct whio_dev * dev, bool
    > takeOwnership )
    > {
    >     if( ! dev ) return 0;
    >     whio_stream * str = (whio_stream*)malloc( sizeof(whio_stream) );


    you don't need the cast, and it may conceal errors. Some people would
    recommend using sizeof *str rather than sizeof(whio_stream).

    whio_stream* str = malloc (sizeof *str);

    >     if( ! str ) return 0;
    >     *str = whio_stream_dev_init;


    oops. Haven't you just lost the malloc()ed block? This looks like a
    memory leak to me.


    >     whio_stream_dev * sd = (whio_stream_dev*) (str + sizeof
    > (whio_stream));


    I'm guessing there's supposed to be another malloc() there... It's
    always a good idea to post your /actual/ code. What is (str +
    sizeof(whio_stream)) supposed to be then?


    >     if( ! sd )
    >     {
    >         free(str);


    you're free()ing something that wasn't malloc()ed. The Undefined
    Behaviour Beast is baying at the door.

    >         return 0;
    >     }
    >     str->implData = sd;
    >     sd->dev = dev;
    >     sd->ownsDev = takeOwnership;
    >     return str;
    >
    > }
    >
    > The significant bit there is the two mallocs.


    only one in the code I can see...


    > Today i figured i'd try
    > to consolidate that, and turned the above code into:
    >
    > whio_stream * whio_stream_for_dev( struct whio_dev * dev, bool
    > takeOwnership )
    > {
    >     if( ! dev ) return 0;
    >     whio_stream * str = (whio_stream*)malloc( sizeof(whio_stream) +
    > sizeof(whio_stream_dev) );
    >     if( ! str ) return 0;
    >     *str = whio_stream_dev_init;
    >     whio_stream_dev * sd = (whio_stream_dev*) (str + sizeof
    > (whio_stream));
    >     str->implData = sd;
    >     sd->dev = dev;
    >     sd->ownsDev = takeOwnership;
    >     return str;
    > }
    >
    > and i changed my cleanup routine to not free the sd object (since free
    > (str) will now free that). That "seems to work",


    it still looks to have the same bug to me...


    > but now to my question:
    >
    > Is that second approach safe or am i just asking for alignment-related
    > (or platform-specific) problems?


    as other posters have said, yes you have alignment problems.

    > If it is NOT safe, can you recommend a portable workaround which will
    > achieve the goal of using only one malloc instead of two?
    >
    > Many thanks in advance for your insights!


    I'd just use two malloc()s


    --

    "They that can give up essential liberty to
    obtain a little temporary safety deserve
    neither liberty nor safety."
    -- Benjamin Franklin
     
    Nick Keighley, Jun 3, 2010
    #6
  7. On 3 June, 10:20, Richard Heathfield <> wrote:
    > Nick Keighley wrote:
    >
    > <snip>
    >
    > >      whio_stream* str = malloc (sizeof *str);

    >
    > >>     if( ! str ) return 0;
    > >>     *str = whio_stream_dev_init;

    >
    > > oops. Haven't you just lost the malloc()ed block?

    >
    > No.
    >
    > > This looks like a memory leak to me.

    >
    > But not to me. Look again! :)



    arg! I knew there had to be a reason why no one else had spotted the
    "error"
     
    Nick Keighley, Jun 3, 2010
    #7
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. John
    Replies:
    13
    Views:
    710
  2. somenath

    Question regarding malloc casing

    somenath, Dec 2, 2007, in forum: C Programming
    Replies:
    10
    Views:
    508
    Joe Wright
    Dec 3, 2007
  3. somenath
    Replies:
    10
    Views:
    649
    Barry Schwarz
    Dec 6, 2007
  4. Replies:
    13
    Views:
    528
    James Kanze
    Mar 9, 2008
  5. Stephan Beal

    Question regarding safety of a malloc()

    Stephan Beal, Dec 24, 2008, in forum: C Programming
    Replies:
    17
    Views:
    533
Loading...

Share This Page