Question regarding safety of a malloc()

[Andrew Shepson]

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!

 

[Rob Kendrick]

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.

 

[Ben Pfaff]

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.

 

[Eric Sosman]

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;

 

[Barry Schwarz]

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.

 

[Nick Keighley]

Subject: Question regarding safety of a malloc()

what does "safety" mean?

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]

Nick Keighley wrote:




But not to me. Look again!

arg! I knew there had to be a reason why no one else had spotted the
"error"