Hi,

In a memory manager i wrote, i get corruption somewhere from
newly malloced (gnu malloc) memory overwriting existing heap variables.

I have:

----------------------------------------------------------------------------
#include<stddef.h>

#define DATA_PTR_FROM_BLOB(blob)    ((void*)(blob) + offsetof(Blob, data))
									
#define BLOB_FROM_DATA_PTR(ptr)    ((void*)(ptr) - offsetof(Blob, data))

typedef struct Blob {
     int magic;
     size_t payloadsize;
     unsigned char data[0];
     struct Blob *next, *prev; // overwritten by payload starting at data[0]
} Blob;
----------------------------------------------------------------------------

Macro offsetof() comes from stddef.h (gcc 4.0.3 on debian linux).

I've been doing:

   struct Blob ablob;
   ...
   void *dataptr = DATA_PTR_FROM_BLOB(ablob);
   ...
   Blob *blobheader = BLOB_FROM_DATA_PTR(dataptr);
   ...

Will this have alignment problems with gaps in the Blob struct?

Russell Shaw <rjshawN_o@s_pam.netspace.net.au> writes:

> In a memory manager i wrote, i get corruption somewhere from
> newly malloced (gnu malloc) memory overwriting existing heap variables.

If the corruption is reproducible, the easiest way to catch it is
with data breakpoint. On P3 and above, gdb implements data
breakpoints via (hardware) debug registers, and they are very fast.

> #define DATA_PTR_FROM_BLOB(blob)    ((void*)(blob) + offsetof(Blob, data))
> #define BLOB_FROM_DATA_PTR(ptr)    ((void*)(ptr) - offsetof(Blob, data))

Pointer arithmetic on 'void *' is non-portable. 
Quick, which of the two expressions below is true?

   ((void *)p + 1) == ((char *)p + 1)
   ((void *)p + 1) == ((int *)p + 1)

>    Blob *blobheader = BLOB_FROM_DATA_PTR(dataptr);
>
> Will this have alignment problems with gaps in the Blob struct?

No. The offsetof() takes care of accounting for them.

Cheers,
-- 
In order to understand recursion you must first understand recursion.
Remove /-nsp/ for email.

Russell Shaw <rjshawN_o@s_pam.netspace.net.au> writes:

> In a memory manager i wrote, i get corruption somewhere from
> newly malloced (gnu malloc) memory overwriting existing heap variables.
>
> typedef struct Blob {
>     int magic;
>     size_t payloadsize;
>     unsigned char data[0];
>     struct Blob *next, *prev; // overwritten by payload starting at data[0]
> } Blob;

You declared data[] as having 0 elements.  Thus, it overlaps with
next and prev.  (Is this really a surprise?)

ANSI C doesn't allow 0-element arrays.  You must be using some
compiler extension.

I recommend moving data[] to the end of the struct.
-- 
Ben Pfaff 
email: blp@cs.stanford.edu
web: http://benpfaff.org

On Tue, 15 Nov 2005 03:31:52 +1100
Russell Shaw <rjshawN_o@s_pam.netspace.net.au> wrote:

> typedef struct Blob {
>      int magic;
>      size_t payloadsize;
>      unsigned char data[0];
>      struct Blob *next, *prev; // overwritten by payload starting at data[0]

	Well yes and if the payload size is greater than the size of the
two pointers something else is going to get overwritten and you have no
idea what that something else will be.

-- 
C:>WIN                                      |   Directable Mirror Arrays
The computer obeys and wins.                | A better way to focus the sun
You lose and Bill collects.                 |    licences available see
                                            |    http://www.sohara.org/

Russell Shaw <rjshawN_o@s_pam.netspace.net.au> writes:
> In a memory manager i wrote, i get corruption somewhere from
> newly malloced (gnu malloc) memory overwriting existing heap variables.
>
> I have:
>
> ----------------------------------------------------------------------------
> #include<stddef.h>
>
> #define DATA_PTR_FROM_BLOB(blob)    ((void*)(blob) + offsetof(Blob, data))
> 									
> #define BLOB_FROM_DATA_PTR(ptr)    ((void*)(ptr) - offsetof(Blob, data))
>
> typedef struct Blob {
>      int magic;
>      size_t payloadsize;
>      unsigned char data[0];
>      struct Blob *next, *prev; // overwritten by payload starting at data[0]
> } Blob;
> ----------------------------------------------------------------------------
>
> Macro offsetof() comes from stddef.h (gcc 4.0.3 on debian linux).
>
> I've been doing:
>
>    struct Blob ablob;
>    ...
>    void *dataptr = DATA_PTR_FROM_BLOB(ablob);
>    ...
>    Blob *blobheader = BLOB_FROM_DATA_PTR(dataptr);
>    ...
>
> Will this have alignment problems with gaps in the Blob struct?

The use of a zero-element array is not allowed in standard C.  There's
a more nearly conforming version declares a 1-element array, but
allocates more memory so elements past the end of the array can be
accessed; this is known as the "struct hack".  See
<http://www.eskimo.com/~scs/C-faq/q2.6.html> for more information.

But the array must be the last declared member of the struct;
otherwise it will write over any following members.

C99 adds, and I'm fairly sure gcc supports, a conforming version of
the struct hack known as the "flexible array member".  The declaration
uses empty brackets rather than the fictitious [0] or [1]:
    unsigned char data[];
Search the gcc documentation, or a recent C reference, for "flexible
array member".

-- 
Keith Thompson (The_Other_Keith) kst-u@mib.org  <http://www.ghoti.net/~kst>
San Diego Supercomputer Center             <*>  <http://users.sdsc.edu/~kst>
We must do something.  This is something.  Therefore, we must do this.

Russell Shaw wrote:
> Hi,
>
> In a memory manager i wrote, i get corruption somewhere from
> newly malloced (gnu malloc) memory overwriting existing heap variables.
>
> I have:
>
> ----------------------------------------------------------------------------
> #include<stddef.h>
>
> #define DATA_PTR_FROM_BLOB(blob)    ((void*)(blob) + offsetof(Blob, data))
>
> #define BLOB_FROM_DATA_PTR(ptr)    ((void*)(ptr) - offsetof(Blob, data))
>
> typedef struct Blob {
>      int magic;
>      size_t payloadsize;
>      unsigned char data[0];
>      struct Blob *next, *prev; // overwritten by payload starting at data[0]
> } Blob;
> ----------------------------------------------------------------------------
>
> Macro offsetof() comes from stddef.h (gcc 4.0.3 on debian linux).
>
> I've been doing:
>
>    struct Blob ablob;
>    ...
>    void *dataptr = DATA_PTR_FROM_BLOB(ablob);
>    ...
>    Blob *blobheader = BLOB_FROM_DATA_PTR(dataptr);
>    ...
>
> Will this have alignment problems with gaps in the Blob struct?

Others have already explained this, but just to make it clear I think
what you want is this:

typedef struct Blob {
      int magic;
      size_t payloadsize;
      struct Blob *next, *prev;
      unsigned char data[0];
} Blob;

Happened to me once when I had to add a member to my struct. Always
declare stuff BEFORE the data section. Otherwise the member variable
and data will corrupt each other.

"slebetman@yahoo.com" <slebetman@gmail.com> writes:

> Others have already explained this, but just to make it clear I think
> what you want is this:

He probably doesn't. When writing a memory manager, it is a *very*
common technique to have a "Blob" either allocated, or on the
free-list. 

Thus it makes sense to share the storage for forward/back links and
the user data, because either one or the other is used at any given
moment, but never both at the same time.

I think the OP did this *intentionally* (and provided a comment
stating this intent), and everybody who pointed out that data[]
would overwrite the links missed the point. But only the OP can
say for sure.

Cheers,
-- 
In order to understand recursion you must first understand recursion.
Remove /-nsp/ for email.

Paul Pluzhnikov wrote:
> Russell Shaw <rjshawN_o@s_pam.netspace.net.au> writes:
> 
>>In a memory manager i wrote, i get corruption somewhere from
>>newly malloced (gnu malloc) memory overwriting existing heap variables.
> 
> If the corruption is reproducible, the easiest way to catch it is
> with data breakpoint. On P3 and above, gdb implements data
> breakpoints via (hardware) debug registers, and they are very fast.
> 
>>#define DATA_PTR_FROM_BLOB(blob)    ((void*)(blob) + offsetof(Blob, data))
>>#define BLOB_FROM_DATA_PTR(ptr)    ((void*)(ptr) - offsetof(Blob, data))
> 
> Pointer arithmetic on 'void *' is non-portable. 
> Quick, which of the two expressions below is true?
> 
>    ((void *)p + 1) == ((char *)p + 1)
>    ((void *)p + 1) == ((int *)p + 1)

My only assumption is that a pointer to a void assumes the basic increment
of the pointer is one byte, and that char is also one byte. To make it
portable, you let autoconf set some #defines. Therefore the first is true
because sizeof(int) is usually 4-bytes or more on PCs.

>>   Blob *blobheader = BLOB_FROM_DATA_PTR(dataptr);
>>
>>Will this have alignment problems with gaps in the Blob struct?
> 
> No. The offsetof() takes care of accounting for them.
> 
> Cheers,

Ben Pfaff wrote:
> Russell Shaw <rjshawN_o@s_pam.netspace.net.au> writes:
> 
>>In a memory manager i wrote, i get corruption somewhere from
>>newly malloced (gnu malloc) memory overwriting existing heap variables.
>>
>>typedef struct Blob {
>>    int magic;
>>    size_t payloadsize;
>>    unsigned char data[0];
>>    struct Blob *next, *prev; // overwritten by payload starting at data[0]
>>} Blob;
> 
> You declared data[] as having 0 elements.  Thus, it overlaps with
> next and prev.  (Is this really a surprise?)
> 
> ANSI C doesn't allow 0-element arrays.  You must be using some
> compiler extension.

I'm using "data" as an empty marker for the first free memory location following
that point. Anything written starting at "data" will overwrite next/prev,
and extend outside the struct. It's a bit like using alloca(0), but i'm doing
it on the heap.

> I recommend moving data[] to the end of the struct.

Ben Pfaff wrote:
> Russell Shaw <rjshawN_o@s_pam.netspace.net.au> writes:
> 
>>In a memory manager i wrote, i get corruption somewhere from
>>newly malloced (gnu malloc) memory overwriting existing heap variables.
>>
>>typedef struct Blob {
>>    int magic;
>>    size_t payloadsize;
>>    unsigned char data[0];
>>    struct Blob *next, *prev; // overwritten by payload starting at data[0]
>>} Blob;
> 
> You declared data[] as having 0 elements.  Thus, it overlaps with
> next and prev.  (Is this really a surprise?)
> 
> ANSI C doesn't allow 0-element arrays.  You must be using some
> compiler extension.

I'm using gcc-4. I don't see why they should disallow empty array decs.,
because it's good for marking a memory location used for variable size
storage.

> I recommend moving data[] to the end of the struct.

Keith Thompson wrote:
> Russell Shaw <rjshawN_o@s_pam.netspace.net.au> writes:
> 
>>In a memory manager i wrote, i get corruption somewhere from
>>newly malloced (gnu malloc) memory overwriting existing heap variables.
>>
>>I have:
>>
>>----------------------------------------------------------------------------
>>#include<stddef.h>
>>
>>#define DATA_PTR_FROM_BLOB(blob)    ((void*)(blob) + offsetof(Blob, data))
>>									
>>#define BLOB_FROM_DATA_PTR(ptr)    ((void*)(ptr) - offsetof(Blob, data))
>>
>>typedef struct Blob {
>>     int magic;
>>     size_t payloadsize;
>>     unsigned char data[0];
>>     struct Blob *next, *prev; // overwritten by payload starting at data[0]
>>} Blob;
>>----------------------------------------------------------------------------
>>
>>Macro offsetof() comes from stddef.h (gcc 4.0.3 on debian linux).
>>
>>I've been doing:
>>
>>   struct Blob ablob;
>>   ...
>>   void *dataptr = DATA_PTR_FROM_BLOB(ablob);
>>   ...
>>   Blob *blobheader = BLOB_FROM_DATA_PTR(dataptr);
>>   ...
>>
>>Will this have alignment problems with gaps in the Blob struct?
> 
> The use of a zero-element array is not allowed in standard C.  There's
> a more nearly conforming version declares a 1-element array, but
> allocates more memory so elements past the end of the array can be
> accessed; this is known as the "struct hack".  See
> <http://www.eskimo.com/~scs/C-faq/q2.6.html> for more information.

I'm using a gcc extension for zero length arrays.

> But the array must be the last declared member of the struct;
> otherwise it will write over any following members.

When Blob is taken out of storage for use, user data starts at "data",
overwriting next/prev pointers. Those pointers are only used when the
blob is back in the pool, so 8 bytes are gained for free during use.

> C99 adds, and I'm fairly sure gcc supports, a conforming version of
> the struct hack known as the "flexible array member".  The declaration
> uses empty brackets rather than the fictitious [0] or [1]:
>     unsigned char data[];
> Search the gcc documentation, or a recent C reference, for "flexible
> array member".

5.11 in gcc-4.0 info explains it. Unfortunately, it says:

  * Flexible array members may only appear as the last member of a
    `struct' that is otherwise non-empty.

Steve O'Hara-Smith wrote:
> On Tue, 15 Nov 2005 03:31:52 +1100
> Russell Shaw <rjshawN_o@s_pam.netspace.net.au> wrote:
> 
>>typedef struct Blob {
>>     int magic;
>>     size_t payloadsize;
>>     unsigned char data[0];
>>     struct Blob *next, *prev; // overwritten by payload starting at data[0]
> 
> 	Well yes and if the payload size is greater than the size of the
> two pointers something else is going to get overwritten and you have no
> idea what that something else will be.

Blob is actually the head of a variable size memory chunk, with an arbitrary
size payload starting at "data".

Paul Pluzhnikov wrote:
> "slebetman@yahoo.com" <slebetman@gmail.com> writes:
> 
>>Others have already explained this, but just to make it clear I think
>>what you want is this:
> 
> He probably doesn't. When writing a memory manager, it is a *very*
> common technique to have a "Blob" either allocated, or on the
> free-list.
> 
> Thus it makes sense to share the storage for forward/back links and
> the user data, because either one or the other is used at any given
> moment, but never both at the same time.
> 
> I think the OP did this *intentionally* (and provided a comment
> stating this intent), and everybody who pointed out that data[]
> would overwrite the links missed the point. But only the OP can
> say for sure.
> 
> Cheers,

This is what i'm doing. I've since determined that the bug wasn't
even in the memory manager stuff. I thought it was very likely to
be here, because it has only been lightly tested. I was extending
a memory segment from my main app, but realloc() can move all the
data to some place else, thereby invalidating all pointers that
were pointing there;)

Russell Shaw wrote:

> When Blob is taken out of storage for use, user data starts at "data",
> overwriting next/prev pointers. Those pointers are only used when the
> blob is back in the pool, so 8 bytes are gained for free during use.

Was there something wrong with using a union to express this?

-- 
Chris "another virtual machine" Dollin
time is just space under pressure. squeeze!

On 2005-11-15, Paul Pluzhnikov <ppluzhnikov-nsp@charter.net> wrote:
> "slebetman@yahoo.com" <slebetman@gmail.com> writes:
>
>> Others have already explained this, but just to make it clear I think
>> what you want is this:
>
> He probably doesn't. When writing a memory manager, it is a *very*
> common technique to have a "Blob" either allocated, or on the
> free-list. 
>
> Thus it makes sense to share the storage for forward/back links and
> the user data, because either one or the other is used at any given
> moment, but never both at the same time.
>
> I think the OP did this *intentionally* (and provided a comment
> stating this intent), and everybody who pointed out that data[]
> would overwrite the links missed the point. But only the OP can
> say for sure.

I'd suggest a union to make it explicit then.

On Tue, 15 Nov 2005 19:36:38 +1100
Russell Shaw <rjshawN_o@s_pam.netspace.net.au> wrote:

> Steve O'Hara-Smith wrote:
> > On Tue, 15 Nov 2005 03:31:52 +1100
> > Russell Shaw <rjshawN_o@s_pam.netspace.net.au> wrote:
> > 
> >>typedef struct Blob {
> >>     int magic;
> >>     size_t payloadsize;
> >>     unsigned char data[0];
> >>     struct Blob *next, *prev; // overwritten by payload starting at data[0]
> > 
> > 	Well yes and if the payload size is greater than the size of the
> > two pointers something else is going to get overwritten and you have no
> > idea what that something else will be.
> 
> Blob is actually the head of a variable size memory chunk, with an arbitrary
> size payload starting at "data".

	It didn't look like it in the code fragment you posted viz:

   struct Blob ablob;
   ...
   void *dataptr = DATA_PTR_FROM_BLOB(ablob);
   ...
   Blob *blobheader = BLOB_FROM_DATA_PTR(dataptr);

	Now if you had something like:

   struct Blob *ablob = malloc (ENOUGH_SPACE)
....

	you might get somewhere - but I can't see why you want to overwrite
the next and prev pointers, as others have suggested the variable length
array should be at the end (and for portability declared as data[] or 
data[1]).

-- 
C:>WIN                                      |   Directable Mirror Arrays
The computer obeys and wins.                | A better way to focus the sun
You lose and Bill collects.                 |    licences available see
                                            |    http://www.sohara.org/

Chris Dollin wrote:
> Russell Shaw wrote:
> 
>>When Blob is taken out of storage for use, user data starts at "data",
>>overwriting next/prev pointers. Those pointers are only used when the
>>blob is back in the pool, so 8 bytes are gained for free during use.
> 
> Was there something wrong with using a union to express this?

If i had have used a union where the inner struct type did not include
the next/prev pointers, i still would have needed the data[0] marker
as an identifier for where to put data, so i can't see how a union
would make things any simpler.

Russell Shaw wrote:

> Chris Dollin wrote:
>> Russell Shaw wrote:
>> 
>>>When Blob is taken out of storage for use, user data starts at "data",
>>>overwriting next/prev pointers. Those pointers are only used when the
>>>blob is back in the pool, so 8 bytes are gained for free during use.
>> 
>> Was there something wrong with using a union to express this?
> 
> If i had have used a union where the inner struct type did not include
> the next/prev pointers, i still would have needed the data[0] marker
> as an identifier for where to put data, so i can't see how a union
> would make things any simpler.

A union is the way to express that the two components [may] share store.
It makes it /explicit/ that you don't mind that assigning to one field
clobbers the other. 

    union 
        {
        struct { struct Blob *next, *prev; }
        unsigned char data[1 /* struct hack here */]; }
        } payloadOrPointers;

-- 
Chris "another virtual machine" Dollin
time is just space under pressure. squeeze!

Russell Shaw <rjshawN_o@s_pam.netspace.net.au> writes:
> Paul Pluzhnikov wrote:
>> Russell Shaw <rjshawN_o@s_pam.netspace.net.au> writes:
>>>In a memory manager i wrote, i get corruption somewhere from
>>>newly malloced (gnu malloc) memory overwriting existing heap variables.
>> If the corruption is reproducible, the easiest way to catch it is
>> with data breakpoint. On P3 and above, gdb implements data
>> breakpoints via (hardware) debug registers, and they are very fast.
>>
>>>#define DATA_PTR_FROM_BLOB(blob)    ((void*)(blob) + offsetof(Blob, data))
>>>#define BLOB_FROM_DATA_PTR(ptr)    ((void*)(ptr) - offsetof(Blob, data))
>> Pointer arithmetic on 'void *' is non-portable. Quick, which of the
>> two expressions below is true?
>>    ((void *)p + 1) == ((char *)p + 1)
>>    ((void *)p + 1) == ((int *)p + 1)
>
> My only assumption is that a pointer to a void assumes the basic increment
> of the pointer is one byte, and that char is also one byte. To make it
> portable, you let autoconf set some #defines. Therefore the first is true
> because sizeof(int) is usually 4-bytes or more on PCs.

You can't depend on that though.  It's 8 bytes here on a "modern"
system, running FreeBSD/amd64

"Chris Dollin" <kers@hpl.hp.com> wrote in message 
news:dlcap2$r7o$1@malatesta.hpl.hp.com...

> Russell Shaw wrote:

>> When Blob is taken out of storage for use, user data starts at "data",
>> overwriting next/prev pointers. Those pointers are only used when the
>> blob is back in the pool, so 8 bytes are gained for free during use.

> Was there something wrong with using a union to express this?

    You will take a performance hit if you use a union. That may or may not 
matter.

    DS

"David Schwartz" <davids@webmaster.com> writes:

> "Chris Dollin" <kers@hpl.hp.com> wrote in message 
> news:dlcap2$r7o$1@malatesta.hpl.hp.com...
>
>> Russell Shaw wrote:
>
>>> When Blob is taken out of storage for use, user data starts at "data",
>>> overwriting next/prev pointers. Those pointers are only used when the
>>> blob is back in the pool, so 8 bytes are gained for free during use.
>
>> Was there something wrong with using a union to express this?
>
>     You will take a performance hit if you use a union. That may or may not 
> matter.

Why would a union cause a performance hit?
-- 
Ben Pfaff 
email: blp@cs.stanford.edu
web: http://benpfaff.org

Giorgos Keramidas <keramida@ceid.upatras.gr> writes:
> Russell Shaw <rjshawN_o@s_pam.netspace.net.au> writes:
>> Paul Pluzhnikov wrote:
>>> Russell Shaw <rjshawN_o@s_pam.netspace.net.au> writes:
>>>>In a memory manager i wrote, i get corruption somewhere from
>>>>newly malloced (gnu malloc) memory overwriting existing heap variables.
>>> If the corruption is reproducible, the easiest way to catch it is
>>> with data breakpoint. On P3 and above, gdb implements data
>>> breakpoints via (hardware) debug registers, and they are very fast.
>>>
>>>>#define DATA_PTR_FROM_BLOB(blob)    ((void*)(blob) + offsetof(Blob, data))
>>>>#define BLOB_FROM_DATA_PTR(ptr)    ((void*)(ptr) - offsetof(Blob, data))
>>> Pointer arithmetic on 'void *' is non-portable. Quick, which of the
>>> two expressions below is true?
>>>    ((void *)p + 1) == ((char *)p + 1)
>>>    ((void *)p + 1) == ((int *)p + 1)
>>
>> My only assumption is that a pointer to a void assumes the basic increment
>> of the pointer is one byte, and that char is also one byte. To make it
>> portable, you let autoconf set some #defines. Therefore the first is true
>> because sizeof(int) is usually 4-bytes or more on PCs.
>
> You can't depend on that though.  It's 8 bytes here on a "modern"
> system, running FreeBSD/amd64

I think he knows that, which is why he wrote "4-bytes or more".

Strictly speaking, the only relevant requirements imposed by the C
language are sizeof(char)==1, sizeof(int)>=1 (and if sizeof(int)==1,
CHAR_BIT>=16) -- but I don't think gcc supports any such targets.

Assuming the gcc-specific semantics for artithmetic on void*, the
first expression above is true, and the second is false (unless
sizeof(int)==1).

Of course, if you use arithmetic on void*, you've left the realm of
portable standard C, and your code probably won't compile with
compilers other than gcc.  That's fine if you're willing to pay that
price.

-- 
Keith Thompson (The_Other_Keith) kst-u@mib.org  <http://www.ghoti.net/~kst>
San Diego Supercomputer Center             <*>  <http://users.sdsc.edu/~kst>
We must do something.  This is something.  Therefore, we must do this.

Russell Shaw <rjshawN_o@s_pam.netspace.net.au> writes:
> Ben Pfaff wrote:
>> Russell Shaw <rjshawN_o@s_pam.netspace.net.au> writes:
>>
>>>In a memory manager i wrote, i get corruption somewhere from
>>>newly malloced (gnu malloc) memory overwriting existing heap variables.
>>>
>>>typedef struct Blob {
>>>    int magic;
>>>    size_t payloadsize;
>>>    unsigned char data[0];
>>>    struct Blob *next, *prev; // overwritten by payload starting at data[0]
>>>} Blob;
>> You declared data[] as having 0 elements.  Thus, it overlaps with
>> next and prev.  (Is this really a surprise?)
>> ANSI C doesn't allow 0-element arrays.  You must be using some
>> compiler extension.
>
> I'm using gcc-4. I don't see why they should disallow empty array decs.,
> because it's good for marking a memory location used for variable size
> storage.

They can be useful, but they're non-standard, and any code that uses
them isn't portable.  (gcc warns about this with "-pedantic".)

If the C standard supported zero-length arrays, I would have no
objection at all.  As it is, it's up to you to decide whether the
functionality is worth the loss of portability.

-- 
Keith Thompson (The_Other_Keith) kst-u@mib.org  <http://www.ghoti.net/~kst>
San Diego Supercomputer Center             <*>  <http://users.sdsc.edu/~kst>
We must do something.  This is something.  Therefore, we must do this.

"Russell Shaw" <rjshawN_o@s_pam.netspace.net.au> wrote in message 
news:ooqm43-2o4.ln1@main.anatron.com.au...
> Hi,
> I Have:
> #include<stddef.h>
> #define DATA_PTR_FROM_BLOB(blob)    ((void*)(blob) + offsetof(Blob, data))

Me puzzled:

int main(int argc, char* argv[])
{
    return (int)((void *)argv[0] + 10);
}

MSVC says "error C2036: 'void *' : unknown size' while gcc accepts this 
happily. Is it valid or not?

--
Roger

Roger Willcocks wrote:

> Me puzzled:
> 
> int main(int argc, char* argv[])
> {
>     return (int)((void *)argv[0] + 10);
> }
> 
> MSVC says "error C2036: 'void *' : unknown size' while gcc accepts this 
> happily. Is it valid or not?

Pointer arithmetic doesn't make any sense on a void *, so it's invalid.

-- 
Erik Max Francis && max@alcyone.com && http://www.alcyone.com/max/
San Jose, CA, USA && 37 20 N 121 53 W && AIM erikmaxfrancis
   You could have another fate / You could be in another place
   -- Anggun

Erik Max Francis <max@alcyone.com> writes:
> Roger Willcocks wrote:
>> Me puzzled:
>> int main(int argc, char* argv[])
>> {
>>     return (int)((void *)argv[0] + 10);
>> }
>> MSVC says "error C2036: 'void *' : unknown size' while gcc accepts
>> this happily. Is it valid or not?
>
> Pointer arithmetic doesn't make any sense on a void *, so it's invalid.

GCC *does* warn about it, too:

flame:/tmp$ gcc -ansi -pedantic foo.c
foo.c: In function `main':
foo.c:3: warning: pointer of type `void *' used in arithmetic
foo.c:3: warning: cast from pointer to integer of different size
flame:/tmp$ 

Steve O'Hara-Smith wrote:
> On Tue, 15 Nov 2005 19:36:38 +1100
> Russell Shaw <rjshawN_o@s_pam.netspace.net.au> wrote:
> 
>>Steve O'Hara-Smith wrote:
>>
>>>On Tue, 15 Nov 2005 03:31:52 +1100
>>>Russell Shaw <rjshawN_o@s_pam.netspace.net.au> wrote:
>>>
>>>
>>>>typedef struct Blob {
>>>>    int magic;
>>>>    size_t payloadsize;
>>>>    unsigned char data[0];
>>>>    struct Blob *next, *prev; // overwritten by payload starting at data[0]
>>>
>>>	Well yes and if the payload size is greater than the size of the
>>>two pointers something else is going to get overwritten and you have no
>>>idea what that something else will be.
>>
>>Blob is actually the head of a variable size memory chunk, with an arbitrary
>>size payload starting at "data".
> 
> 	It didn't look like it in the code fragment you posted viz:
> 
>    struct Blob ablob;
>    ...
>    void *dataptr = DATA_PTR_FROM_BLOB(ablob);
>    ...
>    Blob *blobheader = BLOB_FROM_DATA_PTR(dataptr);
> 
> 	Now if you had something like:
> 
>    struct Blob *ablob = malloc (ENOUGH_SPACE)
> ...
> 
> 	you might get somewhere - but I can't see why you want to overwrite
> the next and prev pointers, as others have suggested the variable length
> array should be at the end (and for portability declared as data[] or 
> data[1]).

static Blob*
blob_malloc(size_t size)
{
     Blob *blob = malloc(sizeof(Blob) + size);
     blob->payloadsize = size;      /* data-size following blob */
     return blob;
}

blob_malloc(size) would be 8 bytes larger if the space occupied by
the next/prev pointers weren't over written (32-bit pointers).
After doing a blob_malloc(), next/prev pointers are no longer needed.

"Roger Willcocks" <rkww@rops.org> writes:
> "Russell Shaw" <rjshawN_o@s_pam.netspace.net.au> wrote in message 
> news:ooqm43-2o4.ln1@main.anatron.com.au...
>> Hi,
>> I Have:
>> #include<stddef.h>
>> #define DATA_PTR_FROM_BLOB(blob)    ((void*)(blob) + offsetof(Blob, data))
>
> Me puzzled:
>
> int main(int argc, char* argv[])
> {
>     return (int)((void *)argv[0] + 10);
> }
>
> MSVC says "error C2036: 'void *' : unknown size' while gcc accepts this 
> happily. Is it valid or not?

Standard C does not support pointer arithmetic.  gcc does support it
as a language extension.  (In my opinion it's a bad idea, but it is a
feature of the nonstandard "GNU C" language whether I like it or not.)

Try "gcc -pedantic".

-- 
Keith Thompson (The_Other_Keith) kst-u@mib.org  <http://www.ghoti.net/~kst>
San Diego Supercomputer Center             <*>  <http://users.sdsc.edu/~kst>
We must do something.  This is something.  Therefore, we must do this.

Giorgos Keramidas wrote:
> Russell Shaw <rjshawN_o@s_pam.netspace.net.au> writes:
> 
>>Paul Pluzhnikov wrote:
>>
>>>Russell Shaw <rjshawN_o@s_pam.netspace.net.au> writes:
>>>
>>>>In a memory manager i wrote, i get corruption somewhere from
>>>>newly malloced (gnu malloc) memory overwriting existing heap variables.
>>>
>>>If the corruption is reproducible, the easiest way to catch it is
>>>with data breakpoint. On P3 and above, gdb implements data
>>>breakpoints via (hardware) debug registers, and they are very fast.
>>>
>>>
>>>>#define DATA_PTR_FROM_BLOB(blob)    ((void*)(blob) + offsetof(Blob, data))
>>>>#define BLOB_FROM_DATA_PTR(ptr)    ((void*)(ptr) - offsetof(Blob, data))
>>>
>>>Pointer arithmetic on 'void *' is non-portable. Quick, which of the
>>>two expressions below is true?
>>>   ((void *)p + 1) == ((char *)p + 1)
>>>   ((void *)p + 1) == ((int *)p + 1)
>>
>>My only assumption is that a pointer to a void assumes the basic increment
>>of the pointer is one byte, and that char is also one byte. To make it
>>portable, you let autoconf set some #defines. Therefore the first is true
>>because sizeof(int) is usually 4-bytes or more on PCs.
> 
> You can't depend on that though.  It's 8 bytes here on a "modern"
> system, running FreeBSD/amd64

That's what Autoconf is for. You can adjust your code to adapt
to the install environment.

Keith Thompson wrote:
 
> Standard C does not support pointer arithmetic.

There's not enough words in that sentence.

-- 
pete

Russell Shaw wrote:
> Steve O'Hara-Smith wrote:
> 
>> On Tue, 15 Nov 2005 19:36:38 +1100
>> Russell Shaw <rjshawN_o@s_pam.netspace.net.au> wrote:
>>
>>> Steve O'Hara-Smith wrote:
>>>
>>>> On Tue, 15 Nov 2005 03:31:52 +1100
>>>> Russell Shaw <rjshawN_o@s_pam.netspace.net.au> wrote:
>>>>
>>>>
>>>>> typedef struct Blob {
>>>>>    int magic;
>>>>>    size_t payloadsize;
>>>>>    unsigned char data[0];
>>>>>    struct Blob *next, *prev; // overwritten by payload starting at 
>>>>> data[0]
>>>>
>>>>
>>>>     Well yes and if the payload size is greater than the size of the
>>>> two pointers something else is going to get overwritten and you have no
>>>> idea what that something else will be.
>>>
>>>
>>> Blob is actually the head of a variable size memory chunk, with an 
>>> arbitrary
>>> size payload starting at "data".
>>
>>
>>     It didn't look like it in the code fragment you posted viz:
>>
>>    struct Blob ablob;
>>    ...
>>    void *dataptr = DATA_PTR_FROM_BLOB(ablob);
>>    ...
>>    Blob *blobheader = BLOB_FROM_DATA_PTR(dataptr);
>>
>>     Now if you had something like:
>>
>>    struct Blob *ablob = malloc (ENOUGH_SPACE)
>> ...
>>
>>     you might get somewhere - but I can't see why you want to overwrite
>> the next and prev pointers, as others have suggested the variable length
>> array should be at the end (and for portability declared as data[] or 
>> data[1]).
> 
> 
> static Blob*
> blob_malloc(size_t size)
> {
>     Blob *blob = malloc(sizeof(Blob) + size);
>     blob->payloadsize = size;      /* data-size following blob */
>     return blob;
> }
> 
> blob_malloc(size) would be 8 bytes larger if the space occupied by
> the next/prev pointers weren't over written (32-bit pointers).
> After doing a blob_malloc(), next/prev pointers are no longer needed.

Correction:

blob_malloc(size) would be 8 bytes larger if the space occupied by
the next/prev pointers weren't over written (32-bit pointers).
After doing a blob_malloc(), the next/prev pointers are used to put
the blob into a linked list. After delivery to the user app, the
next/prev pointers are no longer needed.

pete <pfiland@mindspring.com> writes:
> Keith Thompson wrote:
>  
>> Standard C does not support pointer arithmetic.
>
> There's not enough words in that sentence.

D'oh!

Standard C does not support pointer arithmetic on type void*.

-- 
Keith Thompson (The_Other_Keith) kst-u@mib.org  <http://www.ghoti.net/~kst>
San Diego Supercomputer Center             <*>  <http://users.sdsc.edu/~kst>
We must do something.  This is something.  Therefore, we must do this.

David Schwartz wrote:

> 
> "Chris Dollin" <kers@hpl.hp.com> wrote in message
> news:dlcap2$r7o$1@malatesta.hpl.hp.com...
> 
>> Russell Shaw wrote:
> 
>>> When Blob is taken out of storage for use, user data starts at "data",
>>> overwriting next/prev pointers. Those pointers are only used when the
>>> blob is back in the pool, so 8 bytes are gained for free during use.
> 
>> Was there something wrong with using a union to express this?
> 
>     You will take a performance hit if you use a union. That may or may
>     not
> matter.

A /performance/ hit? Why?

-- 
Chris "another virtual machine" Dollin
time is just space under pressure. 

"Roger Willcocks" <rkww@rops.org> writes:

>"Russell Shaw" <rjshawN_o@s_pam.netspace.net.au> wrote in message 
>news:ooqm43-2o4.ln1@main.anatron.com.au...
>> Hi,
>> I Have:
>> #include<stddef.h>
>> #define DATA_PTR_FROM_BLOB(blob)    ((void*)(blob) + offsetof(Blob, data))

>Me puzzled:

>int main(int argc, char* argv[])
>{
>    return (int)((void *)argv[0] + 10);
>}

>MSVC says "error C2036: 'void *' : unknown size' while gcc accepts this 
>happily. Is it valid or not?

It is invalid; gcc is broken.

Casper
-- 
Expressed in this posting are my opinions.  They are in no way related
to opinions held by my employer, Sun Microsystems.
Statements on Sun products included here are not gospel and may
be fiction rather than truth.

Giorgos Keramidas <keramida@ceid.upatras.gr> writes:

>Erik Max Francis <max@alcyone.com> writes:
>> Roger Willcocks wrote:
>>> Me puzzled:
>>> int main(int argc, char* argv[])
>>> {
>>>     return (int)((void *)argv[0] + 10);
>>> }
>>> MSVC says "error C2036: 'void *' : unknown size' while gcc accepts
>>> this happily. Is it valid or not?
>>
>> Pointer arithmetic doesn't make any sense on a void *, so it's invalid.

>GCC *does* warn about it, too:

>flame:/tmp$ gcc -ansi -pedantic foo.c
>foo.c: In function `main':
>foo.c:3: warning: pointer of type `void *' used in arithmetic
>foo.c:3: warning: cast from pointer to integer of different size


It shouldn't accept it, though.  This type of "extensions" makes for
programmers writing non-portable code.

"embrace and extend".

Casper
-- 
Expressed in this posting are my opinions.  They are in no way related
to opinions held by my employer, Sun Microsystems.
Statements on Sun products included here are not gospel and may
be fiction rather than truth.

On Wed, 16 Nov 2005 13:31:33 +1100
Russell Shaw <rjshawN_o@s_pam.netspace.net.au> wrote:

> Russell Shaw wrote:

> > 
> > static Blob*
> > blob_malloc(size_t size)
> > {
> >     Blob *blob = malloc(sizeof(Blob) + size);
> >     blob->payloadsize = size;      /* data-size following blob */
> >     return blob;
> > }
> > 
> > blob_malloc(size) would be 8 bytes larger if the space occupied by
> > the next/prev pointers weren't over written (32-bit pointers).
> > After doing a blob_malloc(), next/prev pointers are no longer needed.

	Ah I grok it now - but shouldn't the malloc be for 
sizeof(Blob) + size - 8 or the payloadsize be size + 8 ? Or better
rather than 8, 2*sizeof(Blob *).

	I would also be inclined to use a union to express this rather
than a zero length array.

-- 
C:>WIN                                      |   Directable Mirror Arrays
The computer obeys and wins.                | A better way to focus the sun
You lose and Bill collects.                 |    licences available see
                                            |    http://www.sohara.org/

Steve O'Hara-Smith wrote:
> On Wed, 16 Nov 2005 13:31:33 +1100
> Russell Shaw <rjshawN_o@s_pam.netspace.net.au> wrote:
> 
>>Russell Shaw wrote:
> 
>>>static Blob*
>>>blob_malloc(size_t size)
>>>{
>>>    Blob *blob = malloc(sizeof(Blob) + size);
>>>    blob->payloadsize = size;      /* data-size following blob */
>>>    return blob;
>>>}
>>>
>>>blob_malloc(size) would be 8 bytes larger if the space occupied by
>>>the next/prev pointers weren't over written (32-bit pointers).
>>>After doing a blob_malloc(), next/prev pointers are no longer needed.
> 
> 	Ah I grok it now - but shouldn't the malloc be for 
> sizeof(Blob) + size - 8 or the payloadsize be size + 8 ? Or better
> rather than 8, 2*sizeof(Blob *).

What really happens that is not shown, is that malloc(sizeof(Blob))
is done when 8 bytes or less are wanted. For more bytes,
malloc(sizeof(Blob) + size - 2*sizoef(void*)) is done.

> 	I would also be inclined to use a union to express this rather
> than a zero length array.

On Wed, 16 Nov 2005 22:49:24 +1100
Russell Shaw <rjshawN_o@s_pam.netspace.net.au> wrote:

> Steve O'Hara-Smith wrote:

> > 	Ah I grok it now - but shouldn't the malloc be for 
> > sizeof(Blob) + size - 8 or the payloadsize be size + 8 ? Or better
> > rather than 8, 2*sizeof(Blob *).
> 
> What really happens that is not shown, is that malloc(sizeof(Blob))
> is done when 8 bytes or less are wanted. For more bytes,
> malloc(sizeof(Blob) + size - 2*sizoef(void*)) is done.

	Y'know it would have caused less confusion if you'd posted
snippets of the real code originally. Granted we probably wouldn't
have seen the realloc problem but at least we wouldn't have chased
so many phantoms.

-- 
C:>WIN                                      |   Directable Mirror Arrays
The computer obeys and wins.                | A better way to focus the sun
You lose and Bill collects.                 |    licences available see
                                            |    http://www.sohara.org/

Steve O'Hara-Smith wrote:
> On Wed, 16 Nov 2005 22:49:24 +1100
> Russell Shaw <rjshawN_o@s_pam.netspace.net.au> wrote:
> 
>>Steve O'Hara-Smith wrote:
> 
>>>	Ah I grok it now - but shouldn't the malloc be for 
>>>sizeof(Blob) + size - 8 or the payloadsize be size + 8 ? Or better
>>>rather than 8, 2*sizeof(Blob *).
>>
>>What really happens that is not shown, is that malloc(sizeof(Blob))
>>is done when 8 bytes or less are wanted. For more bytes,
>>malloc(sizeof(Blob) + size - 2*sizoef(void*)) is done.
> 
> 	Y'know it would have caused less confusion if you'd posted
> snippets of the real code originally. Granted we probably wouldn't
> have seen the realloc problem but at least we wouldn't have chased
> so many phantoms.

It would have been interesting, but distracting noise, because it
was very unlikely the bug was in there. It also needs more testing,
and optimizing. It may be released one day anyway.

In article <5vir43-v45.ln1@main.anatron.com.au>,
 Russell Shaw <rjshawN_o@s_pam.netspace.net.au> wrote:

> Steve O'Hara-Smith wrote:
> > 
> > 	Ah I grok it now - but shouldn't the malloc be for 
> > sizeof(Blob) + size - 8 or the payloadsize be size + 8 ? Or better
> > rather than 8, 2*sizeof(Blob *).
>
> What really happens that is not shown, is that malloc(sizeof(Blob))
> is done when 8 bytes or less are wanted. For more bytes,
> malloc(sizeof(Blob) + size - 2*sizoef(void*)) is done.

I'd use (offsetof(Blob,data)+size), and test to make sure it isn't less 
than sizeof(Blob).  It's simpler, and doesn't rely on knowing the number 
and types of the temporary fields following data[], so it'd be easier to 
add/remove temporary fields if you ever want to.

Elsewhere you noted that the GCC documentation says flexible array members 
must appear as the last member of a struct.  I assume after you found that, 
you changed your struct to use a 1-element array?

Getting back to your original post, I'd avoid using non-portable void 
pointer arithmetic by changing the macros to:

#define DATA_PTR_FROM_BLOB(blob)    ((void*) ((blob)->data))

#define BLOB_FROM_DATA_PTR(ptr)     ((void*) ((char*)(ptr) \
                                       - offsetof(Blob, data)))

Is there any chance the application might use data[] to store anything 
other than an array of char?  If so, you should change the structure 
definition to ensure data[] is properly aligned for any data type.

Casper H.S. Dik <Casper.Dik@Sun.COM> writes:
> Giorgos Keramidas <keramida@ceid.upatras.gr> writes:
>>Erik Max Francis <max@alcyone.com> writes:
>>> Roger Willcocks wrote:
>>>> Me puzzled:
>>>> int main(int argc, char* argv[])
>>>> {
>>>>     return (int)((void *)argv[0] + 10);
>>>> }
>>>> MSVC says "error C2036: 'void *' : unknown size' while gcc accepts
>>>> this happily. Is it valid or not?
>>>
>>> Pointer arithmetic doesn't make any sense on a void *, so it's invalid.
>
>>GCC *does* warn about it, too:
>
>>flame:/tmp$ gcc -ansi -pedantic foo.c
>>foo.c: In function `main':
>>foo.c:3: warning: pointer of type `void *' used in arithmetic
>>foo.c:3: warning: cast from pointer to integer of different size
>
> It shouldn't accept it, though.  This type of "extensions" makes for
> programmers writing non-portable code.
>
> "embrace and extend".

In -pedantic mode this is a warning.  Using -Werror makes all warnings
fatal too, so it *is* possible to write portable code with GCC that
doesn't use the 'extensions'.  It's just the defaults that are broken.

Despite being a very good set of tools, the Sun Studio 10 compiler may
accept code that is not 100% portable when used without the -Xc
option.  I don't that makes the compiler broken in any way though.

Cautious & careful programmers can still use both compilers to write
perfectly portable code.

> It shouldn't accept it, though.  This type of "extensions" makes for
> programmers writing non-portable code.

If your goal is to write portable code, then use the -ansi and
-pedantic cmdline arguments. The compiler can't intuit how you want to
use it.

In the end, you can't rely on the compiler to help you write portable
code. That has to be your job. It can point out some of the more
egregious problems, but the way the C standard is defined, it is
simply impossible for one C compiler to prevent you from using all
potentially non-portable constructs.

b

Wayne C. Morris wrote:
> In article <5vir43-v45.ln1@main.anatron.com.au>,
>  Russell Shaw <rjshawN_o@s_pam.netspace.net.au> wrote:
> 
>>Steve O'Hara-Smith wrote:
>>
>>>	Ah I grok it now - but shouldn't the malloc be for 
>>>sizeof(Blob) + size - 8 or the payloadsize be size + 8 ? Or better
>>>rather than 8, 2*sizeof(Blob *).
>>
>>What really happens that is not shown, is that malloc(sizeof(Blob))
>>is done when 8 bytes or less are wanted. For more bytes,
>>malloc(sizeof(Blob) + size - 2*sizoef(void*)) is done.
> 
> I'd use (offsetof(Blob,data)+size), and test to make sure it isn't less 
> than sizeof(Blob).  It's simpler, and doesn't rely on knowing the number 
> and types of the temporary fields following data[], so it'd be easier to 
> add/remove temporary fields if you ever want to.
> 
> Elsewhere you noted that the GCC documentation says flexible array members 
> must appear as the last member of a struct.  I assume after you found that, 
> you changed your struct to use a 1-element array?

No;)
I find the lack of zero-length arrays mathematically displeasing;)

When memory allocations are handled algorithmically, it is a good
idea to have blob_malloc(0) return a valid memory object. In the algorithm,
some blobs may be shortened to 0, while other zero-length ones are
lengthend. Requiring data[1] means that blob_malloc(0) will always be
larger than neccessary.

If portability to another compiler is required, i'll think of the specific
problem then. I prefer to think of the lack of zero-length arrays more as
a bug that only exists for backward compatability. It will all work with
data[1], but it's just so "incomplete";)

> Getting back to your original post, I'd avoid using non-portable void 
> pointer arithmetic by changing the macros to:
> 
> #define DATA_PTR_FROM_BLOB(blob)    ((void*) ((blob)->data))
> 
> #define BLOB_FROM_DATA_PTR(ptr)     ((void*) ((char*)(ptr) \
>                                        - offsetof(Blob, data)))
> 
> Is there any chance the application might use data[] to store anything 
> other than an array of char?  If so, you should change the structure 
> definition to ensure data[] is properly aligned for any data type.

It will store all kinds of junk. GCC has alignment attributes i could use.

In article <ns4t43-dl1.ln1@main.anatron.com.au>,
 Russell Shaw <rjshawN_o@s_pam.netspace.net.au> wrote:

> > Elsewhere you noted that the GCC documentation says flexible array members 
> > must appear as the last member of a struct.  I assume after you found that, 
> > you changed your struct to use a 1-element array?
> 
> No;)
[snip]
> Requiring data[1] means that blob_malloc(0) will always be
> larger than neccessary.

Not if you use a union to ensure 'data' and 'next' start at the same offset 
within the structure, like so:

   union {
      unsigned char data[1];
      struct {
         // overwritten by payload starting at data[0]
         struct Blob *next, *prev;
      } p;
   } u;

Or you could make 'data' a nested structure containing 'next' and 'prev':

      struct {
         // overwritten by payload starting at 'data'
         struct Blob *next, *prev;
      } data;

Or you could eliminate 'data' entirely, and use 'next' in its place:

#define DATA_PTR_FROM_BLOB(blob)    ((void*) &(blob)->next)

#define BLOB_FROM_DATA_PTR(ptr)     ((void*) ((char*)(ptr) \
                                       - offsetof(Blob, next)))

Or you could move the pointers into a separate 'BlobLinks' structure, and 
access it via ((BlobLinks*) &blob->data[0]).  This would allow you to have 
your zero-length array at the end of the structure, and make it easy to 
test whether the requested allocation size is smaller than 'BlobLinks'.