Trouble with malloc().

[Pegboy]

I am having trouble with malloc() again for a PC app I am developing. The
method of the suspicious line of code seems to be Ok on a embedded platform,
but not with the PC platform. The embedded platform uses a different
compiler. I feel like I'm overlooking a very simple problem, but I can't
see it. I would appreciate any help. Thank you.

I am trying to allocate memory for a structure of type NAT_S which contains
a pointer to a structure of type NAT_ENTRY_S (see code below).

The compiler reports warning 'Suspicious pointer conversion' as indicated
below in the code. This is the only warning or error I get.

This code appears to work but the next call to malloc() following this, the
app crashes. In some instances, ReadNat() is not called and the app works
Ok. So the problem points to within ReadNat(), which is where the warning
is.

I examined/watched the address given to 'nat' and 'nat->entries' and they
are different by 24 bytes when they should be different by 6. The sizeof(
NAT_S ) is 6.

/************************************************************/
typedef struct
{
short a;
short b;
long c;
long d;
} NAT_ENTRY_S;

typedef struct
{
short num_entries;
NAT_ENTRY_S *entries;
} NAT_S;

NAT_S *ReadNat( FILE *file, long offset )
{
NAT_S *nat;
short i, num_entries;

fseek( file, offset, SEEK_SET );
num_entries = ReadShort( file );

if( (nat = malloc( sizeof( NAT_S ) + (num_entries * sizeof(
NAT_ENTRY_S )) )) != NULL )
{
nat->num_entries = num_entries;
nat->entries = nat + sizeof( NAT_S ); /* COMPILER WARNING:
Suspicious pointer conversion */

for( i = 0; i < num_entries; i++ )
{
/* load entries here */
}
}

return ifd;
}
/********************************************************/

Again, thank you.

 

[Kevin Goodsell]

I am having trouble with malloc() again for a PC app I am developing. The
method of the suspicious line of code seems to be Ok on a embedded platform,
but not with the PC platform. The embedded platform uses a different
compiler. I feel like I'm overlooking a very simple problem, but I can't
see it. I would appreciate any help. Thank you.

I am trying to allocate memory for a structure of type NAT_S which contains
a pointer to a structure of type NAT_ENTRY_S (see code below).

The compiler reports warning 'Suspicious pointer conversion' as indicated
below in the code. This is the only warning or error I get.

This code appears to work but the next call to malloc() following this, the
app crashes. In some instances, ReadNat() is not called and the app works
Ok. So the problem points to within ReadNat(), which is where the warning
is.

I examined/watched the address given to 'nat' and 'nat->entries' and they
are different by 24 bytes when they should be different by 6. The sizeof(
NAT_S ) is 6.

/************************************************************/
typedef struct
{
short a;
short b;
long c;
long d;
} NAT_ENTRY_S;

typedef struct
{
short num_entries;
NAT_ENTRY_S *entries;
} NAT_S;

NAT_S *ReadNat( FILE *file, long offset )
{
NAT_S *nat;
short i, num_entries;

fseek( file, offset, SEEK_SET );
num_entries = ReadShort( file );

if( (nat = malloc( sizeof( NAT_S ) + (num_entries * sizeof(
NAT_ENTRY_S )) )) != NULL )
{
nat->num_entries = num_entries;
nat->entries = nat + sizeof( NAT_S ); /* COMPILER WARNING:
Suspicious pointer conversion */

I'll say it's suspicious. It's also most likely the cause of your
problem. What makes you think that nat + sizeof(NAT_S) yields a memory
address that will be properly aligned for a NAT_ENTRY_S? You cannot
simply place an object anywhere in memory that is convenient. Some
objects in some implementations must be aligned on certain boundaries
(e.g., a 4-byte integer might be required to start on an address that is
divisible by 4).

malloc() is required to return a pointer to memory that is properly
aligned for any type, but that address plus some arbitrary value is NOT
required to yield a pointer to memory that is properly aligned for some
random type.

-Kevin

 

[Artie Gold]

I'll say it's suspicious. It's also most likely the cause of your
problem. What makes you think that nat + sizeof(NAT_S) yields a memory
address that will be properly aligned for a NAT_ENTRY_S? You cannot
simply place an object anywhere in memory that is convenient. Some
objects in some implementations must be aligned on certain boundaries
(e.g., a 4-byte integer might be required to start on an address that is
divisible by 4).

Though alignment *may* be an issue here (practically speaking,
however, I doubt it) -- the problem is deeper. The value of the
expression:

nat + sizeof( NAT_S )

is equivalent to:

&(nat[sizeof(NAT_S)])

(probably not what you want) which is then assigned to a
not-necessarily-compatible pointer type. [Remember, pointer
arithmetic is performed in units of the size of the type of the
object to which a pointer points.]

I would recommend to the OP to turn up the warning levels as high as
possible. This would reveal the *real* problem.

malloc() is required to return a pointer to memory that is properly
aligned for any type, but that address plus some arbitrary value is NOT
required to yield a pointer to memory that is properly aligned for some
random type.

-Kevin

HTH,
--ag

 

[Kevin Goodsell]

Though alignment *may* be an issue here (practically speaking, however,
I doubt it) -- the problem is deeper. The value of the expression:

nat + sizeof( NAT_S )

is equivalent to:

&(nat[sizeof(NAT_S)])

Yeah, that's right. I somehow failed to realize what was actually
happening, and read the code the way that the OP probably meant for it
to be, something like this:

(char *)nat + sizeof(NAT_S)

-Kevin

 

[CBFalconer]

I am having trouble with malloc() again for a PC app I am developing. The
method of the suspicious line of code seems to be Ok on a embedded platform,
but not with the PC platform. The embedded platform uses a different
compiler. I feel like I'm overlooking a very simple problem, but I can't
see it. I would appreciate any help. Thank you.

I am trying to allocate memory for a structure of type NAT_S which contains
a pointer to a structure of type NAT_ENTRY_S (see code below).

The compiler reports warning 'Suspicious pointer conversion' as indicated
below in the code. This is the only warning or error I get.

This code appears to work but the next call to malloc() following this, the
app crashes. In some instances, ReadNat() is not called and the app works
Ok. So the problem points to within ReadNat(), which is where the warning
is.

I examined/watched the address given to 'nat' and 'nat->entries' and they
are different by 24 bytes when they should be different by 6. The sizeof(
NAT_S ) is 6.

/************************************************************/
typedef struct
{
short a;
short b;
long c;
long d;
} NAT_ENTRY_S;

typedef struct
{
short num_entries;
NAT_ENTRY_S *entries;
} NAT_S;

NAT_S *ReadNat( FILE *file, long offset )
{
NAT_S *nat;
short i, num_entries;

fseek( file, offset, SEEK_SET );
num_entries = ReadShort( file );

if( (nat = malloc( sizeof( NAT_S ) + (num_entries * sizeof(
NAT_ENTRY_S )) )) != NULL )
{
nat->num_entries = num_entries;
nat->entries = nat + sizeof( NAT_S ); /* COMPILER WARNING:
Suspicious pointer conversion */

for( i = 0; i < num_entries; i++ )
{
/* load entries here */
}
}

return ifd;
}

I am NOT reading things in detail, but I conclude you are trying
to allocate memory to hold the number of entries to be read, using
two entities. NAT_S is a header, holding the count and a pointer
to an array of NAT_ENTRY_S thinggummies. You have to allocate
those things separately, and it will be handy to make a function
to do it:

/* returns NULL for failure, else allocated space for n items */
NAT_S *makespacefor(size_t n)
{
NAT_S *p;

/* first, make space for the header */
if (p = malloc(sizeof *p)) {
/* header created, make space for the array */
p->num_entries = n;
if (p->entries = malloc(n * sizeof *(p->entries))) {
/* array space created - maybe initialize it */
}
else {
/* abject failure */
free(p);
p = NULL;
}
}
return p;
} /* makespacefor */

I disapprove strongly of your identifier names, but the point here
is to create the storage as simply as possible, note any failures,
and go on from there.

Keep functions as simple as possible, and have them do one job
only.

 

[Pegboy]

Thanks all.

I've found that I'm simply going to have to allocate them separately, like
CBFalconer is saying.

To get rid of the warning message, I can change:
nat->entries = nat + sizeof( NAT_S );
to:
nat->entries = (NAT_ENTRY_S *)nat + sizeof( NAT_S );
but that doesn't solve the problem (app crashing).

Thanks again.

 

[Eric Sosman]

Thanks all.

I've found that I'm simply going to have to allocate them separately, like
CBFalconer is saying.

To get rid of the warning message, I can change:
nat->entries = nat + sizeof( NAT_S );
to:
nat->entries = (NAT_ENTRY_S *)nat + sizeof( NAT_S );
but that doesn't solve the problem (app crashing).

That's probably because you've forgotten something
rather important about pointer arithmetic: It is always
done in multiples of the size of the pointed-to object.
So your original `nat + sizeof(NAT_S)' does not point
to a spot `NAT_S' bytes beyond where `nat' itself points,
it points to a spot `NAT_S' *elements* beyond the start.
Since `nat' points to a `NAT_S' object, the sum advances
by `sizeof(NAT_S) * sizeof(NAT_S)' bytes -- which is
probably a little more than you intended, no?

The "correction" has a similar problem. In this case,
`(NAT_ENTRY_S*)nat' points to a `NAT_ENTRY_S' object, and
the arithmetic is done in terms of the size of that object.
The advance is now `sizeof(NAT_ENTRY_S) * sizeof(NAT_S)'
bytes -- again, more than you meant.

The simplest way to write the sum so it advances over
one `NAT_S' instance is `(NAT_ENTRY_S*)(nat + 1)'. Alas,
this can also fail, but for an entirely different reason:
alignment. There are perfectly portable ways to deal with
this problem, but they're surpassingly ugly -- and code
that's hard to read is more likely to be damaged during
maintenance than code that's clear. Unless you are really
allocating a whole lot of these things, to the point where
the per-allocation overhead (typically four to eight bytes)
is the difference betwen success and failure, you'd be well
advised to use separate allocations to hold separate data
types.

 

[Chris Torek]

The simplest way to write the sum so it advances over
one `NAT_S' instance is `(NAT_ENTRY_S*)(nat + 1)'. Alas,
this can also fail, but for an entirely different reason:
alignment. There are perfectly portable ways to deal with
this problem, but they're surpassingly ugly ... [In general]
you'd be well advised to use separate allocations to hold separate data
types.

In this particular case, however -- where one item is a just
a "count header", and the remaining items are the actual data
structures -- C99's "flexible array member" might be the ideal
solution:

struct zag {
/* your data here */
};

struct zog {
int nzags;
struct zag zags[];
};

Now the routine to allocate N "zag"s enclosed in a "zog" might
look like:

struct zog *newzog(int nzags) {
struct zog *p;

if ((p = malloc(sizeof *p + nzags * sizeof p->zags[0])) == NULL)
... handle allocation failure ...
p->nzags = nzags;
/* fill in the zags, p->zags, for i in [0..nzags) if desired */
return p;
}

C89 has no "clean" way to implement this, but if the OP will consult
the FAQ, he will find the "dirty" method that will no doubt work
for him.

 

[Eric Sosman]

[... concerning the "struct hack" ...]
C89 has no "clean" way to implement this, but if the OP will consult
the FAQ, he will find the "dirty" method that will no doubt work
for him.

It can be done cleanly -- or "conformingly," at any rate --
in C89, at the price of ugliness. Here's an allocation for M
occurrences of `struct one' followed by N of `struct two':

struct fake { char x; struct two y; };
#define TWO_ALIGN offsetof(struct fake, y)
#define ONE_SPACE ((M * sizeof(struct one) + TWO_ALIGN - 1) \
/ TWO_ALIGN * TWO_ALIGN)
struct one *ptr1 = malloc(ONE_SPACE + N * sizeof(struct two));
/* assume success */
struct two *ptr2 = (struct two*)((char*)ptr1 + ONE_SPACE)

Personally, I find the ugliness of this "clean" solution so
overpowering that I've actually resorted to it only once. But
in the common case of wanting to store a single `struct head'
followed by a character string (which needs no alignment), the
situation is much simpler:

struct head *ptr = malloc(sizeof *ptr + strlen(string) + 1);
/* assume success */
strcpy( (char*)(ptr + 1), string);