Hi,considering data and length have valid values I have
the following:
struct object {
uint32_t length;
void *data;
};
void objfunc(void *data, uint32_t length)
{
struct object *obj;
obj = malloc(sizeof(*obj) + length);
Any call to malloc() can fail. Any code that calls malloc() should
therefore always check whether or not it has failed. This code ignores
that possibility, and even if it did, it provides no mechanism for
reported the fact that it failed. It would not pass review in my group.
obj->length = length;
/*now come the problems*/
obj->data = obj + 1;
/*Does this mean starting at the beginning of obj address
alloc space for sizeof(*obj)? Or does it mean starting
at the end of the space allocated for obj, allocate more
sizeof(*obj)*/
obj is a pointer to a "struct object", so obj+1 is therefore a pointer
to where the second such object would be, if it were part of an array of
"struct object". In other words, it points just after the memory
reserved for *obj itself, which is the first part of the allocated block
of memory that is not reserved for *obj. data is a void* pointer, so it
just points at that memory without pointing at an object of any
particular data type.
if(data)
memcpy(obj->data, data, length);
/*This I think I can understand. Copy length bytes of data into
obj->data. Correct?
No, it copies those bytes into the location where obj->data points at,
which happens to be the location immediately after the one where
obj->data itself is stored (unless there's padding bytes at the end of
"struct object".
/*And now the weird statement I really don't understand.*/
*(struct object **)obj->data = obj; /*I not even know how to read it!*/
The expression (struct object**)obj->data has undefined behavior unless
obj->data points at memory correctly aligned to store a "struct
object*". That memory is guaranteed to be correctly aligned to store a
"struct object", but not necessarily a "struct object *". I presume that
this is not a problem on the platform where this code normally runs; a
struct object contains a void*, and is therefore necessarily correctly
aligned to for a void*, and on many platforms a "struct object *" is
likely to have the same alignment requirements as a "void*". However, in
principle it could be a problem on other platforms.
*(struct object**)obj->data treats the memory that obj->data points at
as a "struct object *", and the entire statement sets that pointer to
the same value as is stored in 'obj' itself, overwriting whatever value
it was given by the memcpy() call.
The code therefore has undefined behavior if length < sizeof(obj),
because not enough memory has been set aside to store such a pointer. In
my opinion, this code should check whether length is long enough, and
return before even allocating the memory if it isn't. Again, the fact
that the interface to this program provides no mechanism for reporting
failure is problem.
/*The bare I know would make the if(data) statement irrelevant
because ...obj->data = obj would overwrite it. But I really
don't know what is happening here.*/
The memcpy() is not necessarily irrelevant, because it copies a total of
length bytes. If length > sizeof(obj), then the remaining bytes will be
unchanged by execution of the "weird" statement.
If the object that data points at must always be correctly aligned, and
sufficiently long, to store a "struct object*", then I would have
declared both data and obj->data as having the type struct object**, in
which case there would be no alignment issue, and the "weird" line would
have been much simpler:
*obj->data = obj;
Another thing to note is that objfunc() allocates memory, but pointers
to the allocated memory are stored in only three locations: 'obj', which
is local to the function, and disappears when the the function returns,
obj->data, which points inside the allocated block, and *(struct
object**)data, which points at the entire allocated block. Those last
two pointers are both stored inside the allocated block, and therefore
become completely inaccessible when this function returns, so there's no
way after that point to access the allocated memory, or to free() it.
This is a memory leak!
If this is actual working code, I must have made an error in my
analysis, which is quite possible, multiple levels of indirection are
always error-prone. I've triple-checked my analysis, but that's
unfortunately not sufficient to ensure that it's correct.
However, if I'm right, this code contains an example of one such error.
I think objfunc() should either return a pointer to the allocated
memory, or write such a pointer to *data. If the latter approach is
used, then data==NULL should be treated as an error condition at the
very beginning: memory should not even be allocated if data is null -
that's the key reason I prefer the other approach. A null pointer can be
returned if malloc() fails or length is not large enough.
