C word

[legacy]

char mystring[128];

C will let you write code that tries to access mystring[300] and, if you write to such a location, your program will often crash or hang

For my lib_tokenizer I created a dedicated library called "lib_safestring", whose data is  structured this way:

Code: [Select]

typedef struct
{
    p_char_t p_context;
    uint16_t position_curr;
    uint16_t len;
    uint16_t size;
} safestring_t;

So a string is really a "object_string", with its methods to access it's data structure.

It adds more function-calls, but you are sure your algorithm is "string-safe" and not "off-by-one" regarding accessing strings.

Code: [Select]

safestring_ans_t safestring_context_assign
(
    p_safestring_t p_safestring,
    p_char_t p_context
)


Code: [Select]

p_safestring_t my_safestring;
char_t msg[]="hAllo world";
safestring_ans_t is_ok;

is_ok = safestring_context_assign(my_safestring, msg);

a single char can only be accessed via

Code: [Select]

safestring_ans_t safestring_geth_ch
(
    p_safestring_t p_safestring,
    uint16_t position
)

If your string is of the size M, and you try to access M+n, this function will panic, so you will be aware of the bug.


Besides, there are new methods (well "functions") to
- append char to the string
- reverse the string
- get a substring
- search in the string
- compare two safestrings
- compare a stafestring with an array of char
- hash the string
- test if the string is empty
- empty the string
- etc

You need to destroy a safestring once you no more need it, this it in order to release resources.

[Nominal Animal]

But you know, ignoring feature requests is not exactly the same as ignoring obvious bugs

Obviously! I included that anecdote to illustrate how getting a discussion going is the hard part.  If you do already have a bug fix at hand, just posting it to gcc-bugs, gcc-patches, or to the bugzilla entry is not enough; you need to find a dev with commit access and get them interested/involved, too.  You might even have to skate by a couple who are slightly too aggressive in pruning unnecessary requests and bug reports, though.

It is a social issue, not a technical one, and is rather common.  For example, the bugs in drivers/md/md-bitmap.c:md_bitmap_status(), fs/proc/task_mmu.c:show_map_vma(), fs/proc/task_mmu.c:show_numa_map(), fs/proc/task_nommu.c:nommu_vma_show(), and elsewhere in the Linux kernel -- that the set of escaped characters given as the third parameter for the seq_file_path() function should always be either empty, or include a backslash ("\\") to ensure all possible file paths are unambiguously represented --, still exist (just checked) and have actually even proliferated (copied to new places in the kernel), even though I reported these with a patch to LKML in 2016.  I got completely ignored, because I am a nobody.  (The bug allows a process to disguise the true path to its executable, as the incorrect escaping means paths with e.g. "\\012" or "\n" in them are both represented as the former.  It is especially nasty when the real full absolute path is longer than PAGE_SIZE in the escaped form, but shorter in the latter form, as it is darned hard to automatically detect then.)

Because this is detrimental to the community, the Linux kernel community grew kernel-janitors and kernel-newbies to avoid that.  I do not know of any similar effort with GCC.  I really should repost those bugs to kernel-janitors, but since being ignored is one of my personal buttons, I haven't.  It is one of my personality flaws, I know .

[Nominal Animal]

For my lib_tokenizer I created a dedicated library called "lib_safestring", whose data is  structured this way

Why uint16_t, and not size_t?
Or, if you need the ability to shrink the memory footprint by limiting the string length, a SLEN_T macro type, with
    #ifndef  SLEN_T
    #define  SLEN_T  size_t
    #endif
just before the structure definition?

Please don't be a Bill and tell me "65535 bytes is long enough for everyone"! 

That said, I prefer the C99 flexible array member idiom, with slightly Pascal-y string definition:

Code: [Select]

typedef struct {
    size_t  size;  /* Number of bytes allocated for the data */
    size_t  used;  /* Number of bytes used; i.e., length; not including trailing nul char */
    char    data[];
} mystring;and usually don't make a difference between a NULL mystring pointer, and pointer to a mystring with used==0.

[SiliconWizard]

Just for the record - my bug report got accepted and confirmed in just under 3 hours from the moment I submitted it to the moment it got confirmed. So, I'm pleasantly surprised here, especially since it was already week-end time.

Sure they asked for a minimal test case, which I had actually done to make sure I had correctly pinpointed the bug. I provided it, and then they confirmed the problem very quickly.

Now I don't know how long it will take for it to get fixed, and if it ever does, but at least they recognized the bug very promptly, admitting that the misalignment check was sort of borked.

[SiliconWizard]

Why uint16_t, and not size_t?
Or, if you need the ability to shrink the memory footprint by limiting the string length, a SLEN_T macro type, with
    #ifndef  SLEN_T
    #define  SLEN_T  size_t
    #endif
just before the structure definition?

Agree.

That said, I prefer the C99 flexible array member idiom, with slightly Pascal-y string definition:

Code: [Select]

typedef struct {
    size_t  size;  /* Number of bytes allocated for the data */
    size_t  used;  /* Number of bytes used; i.e., length; not including trailing nul char */
    char    data[];
} mystring;

This construct can be handy. Before it got standardized, it was often already possible as an extension in many compilers.
It avoids having to allocate TWO objects for just ONE. (Like, if you want to dynamically allocate a "string", in legacy's approach you have to issue TWO allocations (one for the wrapping structure, one for the string buffer itself...) which is not that great performance-wise (multiplying small dynamic allocations is usually a bad idea with most allocators.) (And of course I'm already expecting a reply that he doesn't do dynamic allocations anyway, or that they do a very specific kind of allocations that are OK with this, etc. )

And before extensions, the usual way of doing this was to declare just a one-item array for the last member, such as "char data[1];". When allocating the "dynamic" object, you would just take this extra byte into account for the allocation. The downside with that old approach was that any static checking for the indexes would yield warnings all over the place (but old compilers rarely had very clever static checkers anyway...), whereas with data[], static checkers won't care about indexes (which is, OTOH, not that much more useful, but you gotta know what you're doing in C...)

As to legacy's code, as I got it, we have to understand they seem to be heavily relying on specific analysis tools, and not on really standard stuff, making the usual (some of which I gave) advice and good practices relatively non-relevant. (So in that aspect, we tend not to talk about the same thing exactly, as it seems legacy doesn't actually really care about standard C per se..., thus possible misunderstandings.)

[legacy]

Please don't be a Bill and tell me "65535 bytes is long enough for everyone"! 

There is an hidden motivation for this: 16bit is the max I can allocate for a string-object, this due to how the object-collector does its job under the hood.

One can replace the size as he/she whises, but size_t is nod defined in any of my environment, usually because it confuses HOOD-ICEs during debugging sessions.

[SiliconWizard]

char mystring[128];

C will let you write code that tries to access mystring[300] and, if you write to such a location, your program will often crash or hang. Unfortunately, solving such issues is never as simple as finding a line with an obvious out of bounds index such as I just gave.

This is a very common case of buffer overflow. Sure C is known to allow this much more easily than some other languages, but you still have to realize that buffer overflows can also happen in other languages - even some that claim otherwise.

As a tip, again, use static analyzers as much as you can when writing C. They won't catch everything of course, but are still a very big plus as opposed to doing nothing at all, or expecting to catch everything "obvious" by eye. Some are very expensive, but even the free ones can be immensely helpful. Just run one on some of your code base, and see how many potential problems it can spot. You may be in for a surprise.

As an example - yes it's trivial, but static analyzers can do much better than this:

Code: [Select]

void Test(void)
{
char DamnBuffer[128];
int i;

for (i = 0; i < 1000; i++)
DamnBuffer[i] = i;
}

gcc -Wall will yield: "warning: iteration 128 invokes undefined behavior" ( 7 |   DamnBuffer = i; )
cppcheck will yield: "error: Array 'DamnBuffer[128]' accessed at index 999, which is out of bounds." (note that cppcheck gives you the last index out of bounds, gcc the first, but it works in any case. If you replace the test with i <= 128, it will catch it as well.)

Surprisingly, clang-check (which is not that bad) doesn't catch anything here! Maybe it's an option thing? I'm not too familiar with it yet...

gcc, clang and cppcheck are all free. There are many other tools, some expensive, but the free ones can already get you significantly ahead. Try them.

And of course, there can be many cases of potential buffer overflows that can happen at run-time and that are almost impossible to spot with static analysis. Some tools exist for dynamic code analysis, but they are shit expensive... The additional thought about this, is that in many languages that have inherent "protection" against buffer overflows, attempting one may just yield an exception. Sure it's sometimes better than possible unexpected code execution, but exceptions, depending on how the programmer handles them, can still crash the program, or make it quit. It may be "safer" than random execution in some cases, not so much if the application must be guaranteed to run at all times. Also, some higher-level languages rely on heavy runtimes that, themselves, can have buffer overflows in some cases... (the more complex they are, and often the higher the probability you'll run into an issue with the runtime, on top of possible issues in your own code...)

Just saying that beware of silver bullets. When used as such, they may just yield as frustrating results as C will. Know your tools and use them properly, be them C or whatever else...

[magic]

And of course, there can be many cases of potential buffer overflows that can happen at run-time and that are almost impossible to spot with static analysis. Some tools exist for dynamic code analysis, but they are shit expensive...

valgrind is free but you aren't going to run it on an MCU.

[legacy]

Code: [Select]

struct header
{
    size_t len;
    unsigned char *data;
};

   struct header *p;
   p = malloc(sizeof(*p) + len + 1 );
   p->data = (unsigned char*) (p + 1 );  // memory after p[0] is used for data

vs this approach

[magic]

It is a social issue, not a technical one, and is rather common.  For example, the bugs in drivers/md/md-bitmap.c:md_bitmap_status(), fs/proc/task_mmu.c:show_map_vma(), fs/proc/task_mmu.c:show_numa_map(), fs/proc/task_nommu.c:nommu_vma_show(), and elsewhere in the Linux kernel -- that the set of escaped characters given as the third parameter for the seq_file_path() function should always be either empty, or include a backslash ("\\") to ensure all possible file paths are unambiguously represented --, still exist (just checked) and have actually even proliferated (copied to new places in the kernel), even though I reported these with a patch to LKML in 2016.

I have a suspicion that nobody reads that linux-kernel behemoth anymore. I would rather address the maintainer of seq_file_path directly + whatever is the relevant mailing list and argue that this function should always escape \ because otherwise things become ambiguous. (Also, how exactly am I supposed to escape \0 if the list is null-terminated? ) But that's gonna touch other subsystems and perhaps some userspace users so have fun with that. Yeah, it sucks.

[PlainName]

Code: [Select]

struct header
{
    size_t len;
    unsigned char *data;
};

   struct header *p;
   p = malloc(sizeof(*p) + len + 1 );
   p->data = (unsigned char*) (p + 1 );  // memory after p[0] is used for data


I think p->data might potentially point to the wrong place if it assumes sizeof(size_t) == sizeof(void *).

[SiliconWizard]

Code: [Select]

struct header
{
    size_t len;
    unsigned char *data;
};

   struct header *p;
   p = malloc(sizeof(*p) + len + 1 );
   p->data = (unsigned char*) (p + 1 );  // memory after p[0] is used for data


I think p->data might potentially point to the wrong place if it assumes sizeof(size_t) == sizeof(void *).

Nope. It's pointing to (p + 1), which, if you know your pointer arithmetic right, is right AFTER the whole struct header, p being a pointer to struct header. The malloc allocates the whole struct header ( sizeof(*p) ) PLUS the required additional buffer.

The only thing that makes me shiver, and not in a good way, is that no check of the return value of malloc() is made. If it runs out of memory, it will just write at address 0, which is rarely a good thing.

[Nominal Animal]

Using GCC, pointers seem to generate more efficient code than indexing on x86-64 (the difference is small, and I last checked this on GCC 4.9, so take it with a pinch of salt), so the structure I've sometimes used for file or socket input parsers/chunkers is

Code: [Select]

typedef struct {
    unsigned char *next;  /* First buffered unread character */
    unsigned char *ends;  /* End of buffered data */
    unsigned char *data;  /* Dynamically allocated buffer */
    size_t         size;  /* Size of the dynamically allocated buffer */
    int            fd;    /* POSIX.1 file or socket descriptor */
    unsigned int   errs;  /* Error events, bitmask */
} inbuffer;
#define  INBUFFER_INIT  { NULL, NULL, NULL, 0, -1, 0 }
The buffer size is dynamically managed by an inbuffer_need(inbuffer *, size_t) function, which ensures that there are at least the specified number of bytes buffered, unless end-of-input is encountered (which is one of the events in the errs bitmask).  The same function obviously reads from the input stream when necessary, and before reallocating the buffer, moves existing data so that next==data.  This means that after consuming leading whitespace, the parser can do an inbuffer_need(&buf, MAX_TOKEN_LENGTH+1) call, and be assured that the entire token is in the buffer, starting at buf.next.

Data from the buffer is consumed using two helper functions, inbuffer_next(inbuffer *) and inbuffer_skip(inbuffer *, size_t):

Code: [Select]

static inline int  inbuffer_next(inbuffer *ib)
{
    if (!ib)
        return -1;
    else
    if (ib->next < ib->ends)
        return *(ib->next++);
    else
        return inbuffer_next_slow(ib);
}

static inline size_t  inbuffer_skip(inbuffer *ib, size_t n)
{
    if (!ib)
        return 0;
    else
    if (ib->next + n <= ib->ends) {
        ib->next += n;
        return n;
    } else
        return inbuffer_skip_slow(ib, n);
}
These use two internal "slow" helper functions,

Code: [Select]

enum {
    INBUFFER_EOF = 1<<0,
    INBUFFER_ENOMEM = 1<<1,
};

static int  inbuffer_next_slow(inbuffer *ib)
{
    if (ib->errs) {
        return -1;
    }

    if (ib->next < ib->ends) {
        if (ib->next > ib->data) {
            const size_t  have = ib->ends - ib->data;
            memmove(ib->data, ib->next, have);
            ib->next = ib->data;
            ib->ends = ib->data + have;
        }
    } else {
        ib->next = ib->ends = ib->data;
    }

    if (ib->ends - ib->data >= ib->size) {
        /* Omitted: increase ib->size, realloc ib->data. */
    }

    /* Omitted: Read up to (ib->data + ib->size - ib->ends) bytes,
       increment ib->ends by the number of bytes read. */

    /* If successful, return *(ib->next++), otherwise -1. */
}

static size_t  inbuffer_skip_slow(inbuffer *ib, size_t n)
{
    size_t  skipped;

    if (ib->ends > ib->next) {
        skipped = ib->ends - ib->next;
        ib->next = ib->ends = ib->data;
    } else
        skipped = 0;

    /* Omitted: Skip up to (n - skipped) bytes of input. */

    /* Return the actual number of bytes skipped;
        this is either n, or smaller (in case of end of input). */
}
Depending on the C library version, this can be much faster than standard C fgetc() input.

When parsing binary data structures, like PNG chunks, you do a inbuffer_need(&png, 8); to ensure png.next points to the 4-byte big-endian length and 4-byte chunk type; then inbuffer_need(&png, 12 + length); to read the entire chunk in memory (assuming acceptable wrt. memory use).  To move to the next chunk, you do inbuffer_skip(&png, 12 + length);  I use accessor functions to obtain the 32-bit values from png.next + offset in big-endian byte order (cast and shift each unsigned char, then OR them togeter; usually generates pretty efficient code).

Funniest thing is, I still haven't found a really good way to make the reallocation and low-level read() chunk size policy "automatic".  Such policies are hard!  It is always a tradeoff between efficiency and excessive memory use.  The st_blksize field in the struct stat for the descriptor is a good start, but sometimes you want to use minimum amount of memory even if it means more system calls (and thus slower program); sometimes you know the user will have lots of RAM available anyway for the data to be parsed and there being lots of it, it is better to waste some memory but be as fast as possible.  I can't even leave the decision to the programmer (even if it is myself), because they cannot be arsed to think about how to find that out (from the user); much less implement e.g. suitable compile-time defaults and command-line override options.  I am leaning towards adding minsize, maxsize, and a function pointer to a resize() size policy function to the structure, with compile-time defaults.

[PlainName]

PLUS the required additional buffer

That's the size_t  len, isn't it?

[hamster_nz]

Code: [Select]

struct header
{
    size_t len;
    unsigned char *data;
};

   struct header *p;
   p = malloc(sizeof(*p) + len + 1 );
   p->data = (unsigned char*) (p + 1 );  // memory after p[0] is used for data

vs this approach

I'm slightly wondering why you have the "unsigned char *data;" rather than "char data[1];" where you don't have the pointer, but can do much the same by allocating extra data:

Code: [Select]

struct str {
   int  len;
   char data[1];
};

struct str *str_new(char *text) {
   int len = strlen(text);
   struct str *s;

   s = (struct str *)malloc(sizeof(struct str)+len);
   if(s == NULL) return NULL;

   s->len = len;
   strcpy(s->data,text);

   return s;
}

I' would much rather have a "char data[0]", so the allocation becomes much more like the C standard pattern of adding an extra byte for the terminator,:

Code: [Select]

struct str {
   int  len;
   char data[0];
};

  ...
   s = (struct str *)malloc(sizeof(struct str)+len+1);
  ...

It avoids padding the structure which adds extra bytes to the allocation but a zero-element array throws warnings (at least with GCC).

[SiliconWizard]

I'm slightly wondering why you have the "unsigned char *data;" rather than "char data[1];" where you don't have the pointer, but can do much the same by allocating extra data:

I think you're going through the same mental process as I did... you have to realize that legacy uses specific tools to check their code, and I guess these tools would be completely baffled by the use of flexible array members... (legacy will probably confirm this...) So again, judging the code he posts with our standard C approach doesn't really work. (And even though it's always interesting to see how others work and what tools they use, I admit this can be confusing here to many that read legacy's posts and don't know or understand this fact.)

[SiliconWizard]

PLUS the required additional buffer

That's the size_t  len, isn't it?

I'm not sure I'm following you, and even less what sizeof(size_t) would have anything to do with the correctness of his code?
He was just posting a short (and apparently incomplete) code piece. 'len' in the "p = malloc(sizeof(*p) + len + 1 );" statement is a variable or parameter that is not shown in the code he posted. It's certainly not the member of the 'struct header', that would be unitialized here anyway. What exactly dd you have in mind?

Here is a possible modified version which I think shows correctly and more completely what he meant:

Code: [Select]

struct header
{
    size_t len;
    unsigned char *data;
};

struct header * AllocateString(size_t len)
{
   struct header *p;

   p = malloc(sizeof(*p) + len + 1 );   // or "malloc(sizeof(struct header) + len + 1)": exactly the same, but I prefer the former version for code maintenance if you ever change the base type of p
   if (p == NULL)   // basic checks that should not be omitted IMO
      return NULL;
   
   p->len = len;
   p->data = (unsigned char*) (p + 1 );

   return p;
}


[Nominal Animal]

I would rather address the maintainer of seq_file_path directly + whatever is the relevant mailing list

Yup, kernel-janitors, what git blame gives for the relevant lines (to ping users of seq_file_path() about the issue), plus Al Viro since he's the maintainer for fs/seq_file.c per MAINTAINERS.

argue that this function should always escape \ because otherwise things become ambiguous.

No, because seq_file_path() is also used without any escaping at all.  That is, the backslash-escaping is optional, and only enabled if the escape string is non-empty.

Also, how exactly am I supposed to escape \0 if the list is null-terminated?

Nul is never escaped.  All strings in the Linux kernel are nul-terminated, and this particular interface is used to provide information in kernel-provided human-readable pseudofiles, as a single string; neither the content nor the paths have embedded nuls or escaped nuls.  This particular interface provides paths with characters escaped to make it easier to parse these files automatically; in things like the second field (in parentheses) in /proc/PID/stat and such.  The unescaped form is used for the /proc/PID/exe pseudo-symlink (and the bug allows a crafty process to execute a binary that fudges that to point to the incorrect file).

Not pushing for these bugs to be fixes really is kinda my fault, because I should have persisted, and pinged the relevant authors and kernel-janitors.  My point here is that for us not-very-social people, scaling the social hurdles is still quite a lot of work, and one should be prepared for that: it is normal and not personal.  These are communities, after all.

It was so much easier when being Politically Correct was not a requirement.  Nowadays, instead of asking a question like "okay, but why should we trust you? who the hell are you anyway?", it is much safer to just ignore submissions by nobodies, instead of react to them.  Many devs do not even read posts by people they don't know for that reason (unless CC'd directly).  The exception occurs when the dev-with-commit-access happens to take an interest in the subject, and immediately sees the technical merit in the post, and does not need to ask such questions.  Which has happened to me too before, and fortunately happened with SiliconWizard as mentioned here.

[PlainName]

I'm not sure I'm following you, and even less what sizeof(size_t) would have anything to do with the correctness of his code?

Code: [Select]

p = malloc(sizeof(*p) + len + 1 )
p points to an array of memory which is the size of the header plus data length plus one extra for a null. OK so far.

Code: [Select]

p->data = (unsigned char*) (p + 1 )
The data member is now made to point to itself. p, remember, points to the start of the header, so that code says the data member is at the start plus the number of bytes in a pointer. The assumption here is that  it will be four bytes, and len is four bytes, so now p->data points to itself.

My suggestion is that size_t is not necessarily the same as the size of a pointer, so on a system where sizeof(size_t) != sizeof(void *) the code will be wrong. It's likely that a pointer would be smaller than size_t in that case (x86 segmented mode, perhaps?), thus p->data would point to the middle of p->len.

[westfw]

p->data = (unsigned char*) (p + 1 )[/pre]The data member is now made to point to itself.

No.  Remember that (p+1) add the length of the structure being pointed to to the actual value in p.  (not 1.  Not "the size of a pointer.")
Thus the careful use of parenthesis: "(unsigned char *)p + 1" would do something different.So p ends up pointing to the section of memory just past the "header" (which was allocated.)

[PlainName]

Ah! Easy mistake to make 

[SiliconWizard]

I'm not sure I'm following you, and even less what sizeof(size_t) would have anything to do with the correctness of his code?

Code: [Select]

p = malloc(sizeof(*p) + len + 1 )
p points to an array of memory which is the size of the header plus data length plus one extra for a null. OK so far.

Code: [Select]

p->data = (unsigned char*) (p + 1 )
The data member is now made to point to itself. p, remember, points to the start of the header, so that code says the data member is at the start plus the number of bytes in a pointer. The assumption here is that  it will be four bytes, and len is four bytes, so now p->data points to itself.

No! That's not how pointer arithmetic works!
p + 1 will point to exactly the first byte after the whole header struct, and the data member will NOT point to itself! To remember that, remember 'p+1' is EXACTLY equivalent to &p[1].

For a pointer to a given type:

BaseType *p;

Byte-wise, this is how things work:

(char *)(p + 1) is equal to ((char *) p) + sizeof(BaseType)

Again, to remember pointer arithmetic, think "arrays".

And some strict rules (safety-related) actually recommend against using pointer arithmetic (to avoid fuck-ups) and only use the array-equivalent approach, which is less confusing to many.

So in his example:
"p->data = (unsigned char*) &p[1];" for instance.

But I can assure you it's exactly the same.

[Nominal Animal]

For a pointer to a given type:

    BaseType *p;

Byte-wise, this is how things work:

    (char *)(p + 1) == ((char *) p) + sizeof(BaseType)

Yes!  And, since sizeof is a C operator that does not evaluate its argument expression except for its type, sizeof (Basetype) == sizeof *p, and thus
    (char *)(p + 1) == ((char *)p) + sizeof *p

Again, incrementing or decrementing a pointer by one, changes the memory address the pointer points to by the size of the type it points to.

Also, it may come as a surprise to some, but if you write e.g. sizeof(p++), it evaluates to the size of p, but p is not incremented. This is why I usually add a space after sizeof, to remind myself and others that it's not a function, but a special operator.

[Nusa]

For a pointer to a given type:

    BaseType *p;

Byte-wise, this is how things work:

    (char *)(p + 1) == ((char *) p) + sizeof(BaseType)

Yes!  And, since sizeof is a C operator that does not evaluate its argument expression except for its type, sizeof (Basetype) == sizeof *p, and thus
    (char *)(p + 1) == ((char *)p) + sizeof *p

Again, incrementing or decrementing a pointer by one, changes the memory address the pointer points to by the size of the type it points to.

Also, it may come as a surprise to some, but if you write e.g. sizeof(p++), it evaluates to the size of p, but p is not incremented. This is why I usually add a space after sizeof, to remind myself and others that it's not a function, but a special operator.

Truth. But as long as we're doing confusing C code: Try sizeof(int[x++]). x does get incremented. Do you know why?

[Nominal Animal]

Try sizeof(int[x++]). x does get incremented. Do you know why?

Yes. The int[expression] part describes a type, an array of ints, and the expression part must be evaluated to determine the number of elements in that array.  The entire expression evaluates to the size of that array.

Bugger me if I know whether that is according to the C standard or not (i.e. whether the increment should be visible outside the expression or not); I gave up language-lawyerism a long time ago.  It is such a corner case I would not dare assume all C compilers get it right.  Seeing any kind of increment, decrement, or assignment in a sizeof expression is a red flag to me: something's afoot, and that expression must be fixed.