7 ms·

Yes the code is bad but does not need to be completely rewritten. It should interpret the data as an array uint8_t (no casting to structure/integer pointers!),

• ben0x539 8y ago
  I don't think I disagree, but could you articulate how strict aliasing makes this code wrong? Maybe I'm not looking at the right code, but it's not immediately obvious from the post to me.
  • ambrop7 8y ago
    Example explanation: https://stackoverflow.com/a/99010/1020667 https://stackoverflow.com/a/99010/1020667 (feel free to look for better ones).
    • ben0x539 8y ago
      That code looks somewhat wonky and seems either legal to me, or illegal for non-aliasing reasons. In principle, taking a pointer to a blob of bytes and casting it to a pointer to your struct should be sound if you can assume that the blob of bytes was originally a valid object of that struct type. I can see how you might get it wrong if that's not in fact how the blob was originally formed or if you get some alignment details wrong, but I can't immediately think of any aliasing-specific problems.
      • ambrop7 8y ago
        The problem is that it is "potentially unsafe" depending on what OTHER code does. For example, if some other code happens to manipulate the same bytes as an array of uint16_t (but not char/uint8_t) it's wrong (like in that example). Similarly if some other code manipulates the same bytes as some other struct type, it's also wrong. Yes it is possible to do it this way and not have undefined behavior, but you are leaving trap doors open that you don't even know about.
      • int_19h 8y ago
        C strict aliasing rules doesn't allow you to cast a pointer to a char array to an arbitrary type T, regardless of whether it has correct alignment and contains a valid representation of T. It allows you to memcpy from a char array to a T variable (assuming valid representation). And yes, it does actually produce broken code at runtime when optimizations are enabled on some compilers, unless you explicitly opt out from optimizations that depend on strict aliasing.
        • paavoova 8y ago
          Nonetheless, it's quite commonly used in popular code. E.g. I was looking at some dynamic string libraries, and a popular one (sds) casts a struct header from a char * and modifies its values. This code is used in Redis.
          • int_19h 8y ago
            And if you look at its build scripts, I bet there's -fno-strict-aliasing there. All popular compilers let you opt out of these kinds of optimizations. But it's no longer standard C at that point - and it's specifically because of strict aliasing rules (which were being discussed in this thread). The bugs you get when you break the rules without opting out of standard compliance are not theoretical, either. Just google for "strict aliasing bug" to see numerous horror stories.
        • clarry 8y ago
          In fact the C aliasing rules don't say a single thing about casting. You are free to cast at will. What matters, as far as aliasing is concerned, is the effective type of an object for access through an lvalue. And the effective type is.. it depends. But it is not the same as the type of the pointee of whatever pointer you might or might not have. Note that arrays are not lvalues, so the effective type of an object is never an array of char or any other array type. You can, however, access any object with a character type, and that does not change the effective type of said object. The standard explicitly permits this! That means I could allocate some memory, copy an object in there, make a pointer-to-array-of-char, pass the pointer onwards, and let the next guy in row cast this pointer-to-array-of-char into a pointer-to-the-type-of-the object I previously copied, perfectly legal. And if I didn't copy an object, it is still perfectly legal. A random piece of memory has no effective type unless it's gained one due to a previous access through a non-character type.
• jstimpfle 8y ago
  As an array of uint8_t? I'm pretty sure that's wrong, and that it should be char instead. Usual OS network APIs are char based, and the fact that the network is technically about "octets" shouldn't matter behind those APIs. Or is my thinking wrong?
  • ambrop7 8y ago
    On virtually all modern computers chars are 8-bit and casting the address to uint8_t* is perfectly fine (due to specific provisions in the standards that I am too lazy to find). And you need unsigned so that expressions like x + (y << 16) do the right thing. With that in mind, I use uint8_t as opposed to unsigned char to emphasize that it is 8-bit and also for less typing :)
    • saagarjha 8y ago
      > On virtually all modern computers chars are 8-bit and casting the address to uint8_t* is perfectly fine (due to specific provisions in the standards that I am too lazy to find). Are you sure the provision doesn't say "char *"?
      • ambrop7 8y ago
        Yeah that may be true, see the comment from amluto.
  • alxlaz 8y ago
    uint8_t is guaranteed to always be an octet, and stdint types (uint8_t, int32_t, uint32_t etc.) are also the recommended types to use when you want to make the size and signedness of the data clear. An array of uint8_t is definitely correct, and it's also the right thing to do in 2018. An array of char is something you should only use to store human-readable character in array form. Many OS network APIs are "char based" because the C standard requires that sizeof char be exactly one byte, so it used to be a convenient shorthand, but that's not the case anymore.
    • 8y ago
• microcolonel 8y ago
  > The byte order of the computer doesn't matter much at all except to compiler writers and the like This statement is to generic to mean anything helpful. I think the point our friend Rob Pike is making is that you could write general purpose code which accomplishes the task, and on a sufficiently advanced compiler, the swap may be eliminated where applicable; but when the point is made this way, it surely just pisses off people who know better. C programmers are not (quite) children, you don't need to eradicate nuance to keep people reading.
• camgunz 8y ago
  Yeah casting to struct pointers is a bright red flag. It also rarely saves you anything since packed structs are slow, and you still have to validate the contents. You're right, these streams should be interpreted as uint8_t to avoid undefined behavior when shifting, masking, and ORing, and anyone doing this should tuck it behind some abstraction layer. It should never bleed into parsing.
  • webkike 8y ago
    My second year at google, I fixed undefined behavior that essentially appeared in every google binary because a core library did this without care for the alignment of the memory in question. Thank god with PODs you can just memcpy
  • Gibbon1 8y ago
    > It also rarely saves you anything since packed structs are slow Packed structs are slow on 1990's RISC processors. Packed structs are not slow on 2010 era super scalar processors.
    • camgunz 8y ago
      Fair, but people do the packed struct thing because they're either lazy (don't want to memcpy individual fields) or they're trying to avoid a perf hit from memcpy. That perf hit is on the scale of using packed structs, except you only pay it once. So it's really not worth it.
      • Gibbon1 8y ago
        The problem is you don't have a decent way in the language to specify data structures layout in memory. That's the C/C++ standards committees fault. Problem is worse because people that design network standards refuse to switch to little endian like everyone else did 30 years ago.
      • usefulcat 8y ago
        I've written a good deal of very high performance code that does a great deal of parsing such 'packed structs', and I intentionally do not use memcpy, for performance reasons. For one thing, note that many compilers never inline calls to memcpy--it's always a function call. In this kind of code, that alone is sufficient reason to avoid using it for accessing individual fields.
• moefh 8y ago
  Interestingly (and pedantically), the code from the otherwise excellent "Byte order fallacy" article technically contains undefined behavior in C on machines where int is 32 bits wide (which is almost everything nowadays). Consider this example of extracting a 32-bit int encoded as little endian: #include <stdio.h> int main(void) { unsigned char data[4] = { 0, 0, 0, 128 }; unsigned int i; // the line of code from the article: i = (data[0]<<0) | (data[1]<<8) | (data[2]<<16) | (data[3]<<24); printf("%x\n", i); } Compiling this with either gcc or clang using "-fsanitize=undefined" triggers this runtime error: test.c:9:61: runtime error: left shift of 128 by 24 places cannot be represented in type 'int' That happens because, even though the bytes are unsigned as the article requires (using uint8_t instead of unsigned char has the same problem), they are promoted to (signed) int before the left shift as part of automatic integer promotion. When the left shift by 24 happens, it puts an 1 into the sign bit of the int, which is undefined behavior. Later the int is converted to unsigned (using implementation-defined behavior, which luckily is sane on all popular compilers), but by then it's already too late. The solution is to change the code slightly: i = (data[0]<<0) | (data[1]<<8) | (data[2]<<16) | ((unsigned)data[3]<<24); This prevents the promotion from unsigned char (or uint8_t) to int by explicitly casting it to an unsigned int, which can then be safely shifted left by 24.
  • ambrop7 8y ago
    You are correct. I didn't want to go into these details to not confuse people even more :) But I think it is better to cast all the bytes to the final type (e.g. uint32) before shifting, not unknown types like unsigned int.
  • d21d3q 8y ago
    Can't reproduce: https://onlinegdb.com/HyighU72Q https://onlinegdb.com/HyighU72Q
    • moefh 8y ago
      I'm not familiar with that site, I don't see any way of passing arguments to the compiler. To reproduce you have to compile with "-fsanitize=undefined", as in: gcc -fsanitize=undefined -o test test.c ./test (clang also works with the exact same command line)