11 ms·

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 conte

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.
    - usefulcat 8y ago
      > many compilers never inline calls to memcpy Sigh. Of course I say that, and then try to reproduce it on godbolt, and can't, at least not with a trivial example. So maybe that problem is not as widespread as I thought.
    - camgunz 8y ago
      Yeah I'm completely ok with reading into a packed struct. I think you may as well and indeed in most cases it's more explicit and succinct. I think using that internally leads to problems, so generally I advocate (if you're going to do this) using those as "packets" or whatever that are just PODs. You can validate those, then build higher level, internal structs from them. Generally I find whenever you see packed structs anywhere but the "I got this from the network/filesystem" level, the devs aren't considering taint analysis, which is a red flag to me. You shouldn't be dealing with external data outside a specific, well-defined layer.

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.
  - usefulcat 8y ago
    > many compilers never inline calls to memcpy Sigh. Of course I say that, and then try to reproduce it on godbolt, and can't, at least not with a trivial example. So maybe that problem is not as widespread as I thought.
  - camgunz 8y ago
    Yeah I'm completely ok with reading into a packed struct. I think you may as well and indeed in most cases it's more explicit and succinct. I think using that internally leads to problems, so generally I advocate (if you're going to do this) using those as "packets" or whatever that are just PODs. You can validate those, then build higher level, internal structs from them. Generally I find whenever you see packed structs anywhere but the "I got this from the network/filesystem" level, the devs aren't considering taint analysis, which is a red flag to me. You shouldn't be dealing with external data outside a specific, well-defined layer.