9 ms·

I have done some win32 programming but haven't encountered struct packing or alignment being an issue, where does that pop up?

gambiting 6y ago
I work in video games, and very recently we had a sneaky bug in one of our AAA titles(that was already out!), where(in huge simplification) we had a struct that looked like: struct Obj { int foo; bool bar; } then we were storing those in a custom hashmap using these as keys, where the hashing function was basically hashing bits of each stored object, without any awareness of what's in the object. The bug was found when someone did something like: void DoSomething(Obj obj) { if(!map.has(obj)) map.add(obj, new Whatever()); map[obj].blah(); //CRASH null pointer exception } I was like.....well, if there is no key "obj" in the map, we insert one....and yet literally one line after it doesn't have a value for that key??? How can this be? Well, it can be because even though the struct looks like it takes 5 bytes, in reality it's 8 bytes because it's getting padded. So a naive hashing method that just looks at bits is hashing your 5 bytes of actual data + 3 bytes of garbage, which means that two "identical" objects are very unlikely to actually produce the same hash. C++20 now has a "hashable" concept to help with this, but it still requires the programmer to be aware of structure packing.
- flohofwoe 6y ago
  It's not just the different "under the hood size" that's a problem in this situation, but alignment-padding bytes added by the compiler inbetween struct members will have random junk data in them, the compiler will not zero-initialize padding bytes, unless you explicitely memset() the struct (and even then I wouldn't count on that the padding bytes aren't "tainted" later). E.g. if you initialize a C struct or C++ object the "usual way": Obj obj = { }; There will most definitely be junk in the padding bytes.
  - steerablesafe 6y ago
    Zeroing padding bytes is an interesting scenario. The C++ standard does mandate zeroing padding bytes in certain cases[1], but some compilers do not conform. It's especially a nasty issue when partially overlapping subobjects and RVO are added to the mix. There is a possible defect in the C++ standard here. [1] https://eel.is/c++draft/dcl.init#6.2 https://eel.is/c++draft/dcl.init#6.2
- saalweachter 6y ago
  In my first job, there was an interesting bug introduced by an un-terminated pragma-pack in a header file. Because not every structure was packed, depending on your include order, some structures would be packed differently in different compilation units. Except, for the only structures this happened for, the packed packing was coincidentally the same as the default packing ... when the program was compiled 32-bits. Attempting to switch from a 32-bit executable to a 64-bit executable resulted in mysterious segfaults as different compilation units disagreed on where different fields were. Fun times!
  - gambiting 6y ago
    >> some structures would be packed differently in different compilation units. <shivers violently>
  - karlding 6y ago
    This is why I always use static_assert [0] to assert on the struct size in projects using C11+, or use a macro to create my own (using the negative array size trick) to cause a compile error if I can't use C11 as a sanity check for these cases. This saves a lot of potential headaches. [0] https://en.cppreference.com/w/c/language/_Static_assert https://en.cppreference.com/w/c/language/_Static_assert
- kazinator 6y ago
  > C++20 now has a "hashable" concept to help with this I'm sorry, you can easily solve this problem in K&R C from 1979.
  - shawxe 6y ago
    Yeah, it's called memset. Not sure if this bug is supposed to be subtle or something, but if you require the padding bytes to be consistent then you need to consistently initialize your structs. Ignoring UB often leads to these sorts of bugs. EDIT: Based on some of the comments I'm getting here, it seems like some of you have never implemented a hashmap/generic interface in vanilla C and it shows. If you want a hashmap that is generic and you don't want to write/specify a hashing function for keys on map initialization, then what you're likely to end up doing is ensuring that keys are initialized consistently, providing key size on map initialization, and simply performing a hash on the key as though it were a buffer of bytes. Suggesting that this is somehow any more fragile than anything else in C or that templates should be used instead, which is an absurd comment since templates do not exist in C, is ridiculous. This comment is replying to a comment about how solutions to this problem have existed since K&R C--and they have. You don't need templates to not ignore UB, although if you are using C++ you can certainly use templates (and also take advantage of the stronger type system) to work around issues like this. The point is that avoiding undefined behavior in C/C++ hashmap implementations is not something that has only recently become possible. C solutions may be more fragile, but that doesn't stop them from being "correct" in that will yield correct behavior unless an error is made elsewhere. Code that makes assumptions about the values of padding without explicitly setting those values is NOT correct and for anyone who works in a language like C/C++ regularly, that should be obvious.
    - saagarjha 6y ago
      I don’t think memset is required to zero out padding bytes. The correct way to do this is to only access non-padding bytes.
      - shawxe 6y ago
        sizeof(struct foo) is required to return the actual size, including padding, so struct foo foo_inst; memset(&foo_inst, 0, sizeof(foo_inst)); will certainly result in all padding bytes being set to zero.
- bregma 6y ago
  If you rely on code containing undefined behaviour you're in for a world of butthurt sooner rather than later. There is no way in C++ to get at the padding bytes unless you're using undefined behaviour. How does the hash function work? Pointer aliasing using reinterpret_cast? Pointer aliasing using C-style casts? Typing punning through the old union switcheroo?
  - gambiting 6y ago
    I don't have the code in front of me, but something like int hash=0; for(int i=0;i<sizeof(obj);i++) hash += hashing_method(reintepret_cast<char*>(&obj)+i); return hash; Basically hashing each byte of the memory containing the object, regardless of what the object itself represents. We can argue whether that's a smart thing to do or not, but I wasn't in charge of implementing it - it's a relic from a codebase that's more than a decade old at this point. It's a simple hashing method that works with most types, but obviously dies horrendously in a case like this.
    - saagarjha 6y ago
      Yeah, this is technically undefined.
    - nly 6y ago
      This doesn't help you, because the contents of the padding bytes is not guaranteed to be anything in particular. Two structs containing identical field values can have different padding bytes. Reading the padding bytes is UB. Just pack the struct and be done with it.
  - Asooka 6y ago
    You can trivially get at the padding bytes, just cast to char pointer. Their contents are undefined and there is absolutely no guarantee that they will remain constant, but they're there. Generally you have several possibilities for how to escape this problem, but the simplest is to just add the padding and a static assert that the sizeof the struct is what you expect.
    - 6y ago
munchbunny 6y ago
It's abstracted behind the API's data types as long as you're using C++, but in order to work with the data types you often have to maneuver around the specific packing/alignment. The Windows ETW tracing API has several examples of specific "structure packing" phenomena: 1. Arranging fields to pack things that are shorter than 4 bytes along 4 byte boundaries. 2. Manually arranging data buffers in very specific layouts (this is less about the original post, more about a different interpretation of "packing"). Aside from what I mentioned above, more commonly you will run into this in interop scenarios, such as calling C/C++ API's from C#. Thankfully those scenarios are few and far between.

gambiting 6y ago

I work in video games, and very recently we had a sneaky bug in one of our AAA titles(that was already out!), where(in huge simplification) we had a struct that looked like: struct Obj { int foo; bool bar; } then we were storing those in a custom hashmap using these as keys, where the hashing function was basically hashing bits of each stored object, without any awareness of what's in the object. The bug was found when someone did something like: void DoSomething(Obj obj) { if(!map.has(obj)) map.add(obj, new Whatever()); map[obj].blah(); //CRASH null pointer exception } I was like.....well, if there is no key "obj" in the map, we insert one....and yet literally one line after it doesn't have a value for that key??? How can this be? Well, it can be because even though the struct looks like it takes 5 bytes, in reality it's 8 bytes because it's getting padded. So a naive hashing method that just looks at bits is hashing your 5 bytes of actual data + 3 bytes of garbage, which means that two "identical" objects are very unlikely to actually produce the same hash. C++20 now has a "hashable" concept to help with this, but it still requires the programmer to be aware of structure packing.

flohofwoe 6y ago
It's not just the different "under the hood size" that's a problem in this situation, but alignment-padding bytes added by the compiler inbetween struct members will have random junk data in them, the compiler will not zero-initialize padding bytes, unless you explicitely memset() the struct (and even then I wouldn't count on that the padding bytes aren't "tainted" later). E.g. if you initialize a C struct or C++ object the "usual way": Obj obj = { }; There will most definitely be junk in the padding bytes.
- steerablesafe 6y ago
  Zeroing padding bytes is an interesting scenario. The C++ standard does mandate zeroing padding bytes in certain cases[1], but some compilers do not conform. It's especially a nasty issue when partially overlapping subobjects and RVO are added to the mix. There is a possible defect in the C++ standard here. [1] https://eel.is/c++draft/dcl.init#6.2 https://eel.is/c++draft/dcl.init#6.2
saalweachter 6y ago
In my first job, there was an interesting bug introduced by an un-terminated pragma-pack in a header file. Because not every structure was packed, depending on your include order, some structures would be packed differently in different compilation units. Except, for the only structures this happened for, the packed packing was coincidentally the same as the default packing ... when the program was compiled 32-bits. Attempting to switch from a 32-bit executable to a 64-bit executable resulted in mysterious segfaults as different compilation units disagreed on where different fields were. Fun times!
- gambiting 6y ago
  >> some structures would be packed differently in different compilation units. <shivers violently>
- karlding 6y ago
  This is why I always use static_assert [0] to assert on the struct size in projects using C11+, or use a macro to create my own (using the negative array size trick) to cause a compile error if I can't use C11 as a sanity check for these cases. This saves a lot of potential headaches. [0] https://en.cppreference.com/w/c/language/_Static_assert https://en.cppreference.com/w/c/language/_Static_assert
kazinator 6y ago
> C++20 now has a "hashable" concept to help with this I'm sorry, you can easily solve this problem in K&R C from 1979.
- shawxe 6y ago
  Yeah, it's called memset. Not sure if this bug is supposed to be subtle or something, but if you require the padding bytes to be consistent then you need to consistently initialize your structs. Ignoring UB often leads to these sorts of bugs. EDIT: Based on some of the comments I'm getting here, it seems like some of you have never implemented a hashmap/generic interface in vanilla C and it shows. If you want a hashmap that is generic and you don't want to write/specify a hashing function for keys on map initialization, then what you're likely to end up doing is ensuring that keys are initialized consistently, providing key size on map initialization, and simply performing a hash on the key as though it were a buffer of bytes. Suggesting that this is somehow any more fragile than anything else in C or that templates should be used instead, which is an absurd comment since templates do not exist in C, is ridiculous. This comment is replying to a comment about how solutions to this problem have existed since K&R C--and they have. You don't need templates to not ignore UB, although if you are using C++ you can certainly use templates (and also take advantage of the stronger type system) to work around issues like this. The point is that avoiding undefined behavior in C/C++ hashmap implementations is not something that has only recently become possible. C solutions may be more fragile, but that doesn't stop them from being "correct" in that will yield correct behavior unless an error is made elsewhere. Code that makes assumptions about the values of padding without explicitly setting those values is NOT correct and for anyone who works in a language like C/C++ regularly, that should be obvious.
  - saagarjha 6y ago
    I don’t think memset is required to zero out padding bytes. The correct way to do this is to only access non-padding bytes.
    - shawxe 6y ago
      sizeof(struct foo) is required to return the actual size, including padding, so struct foo foo_inst; memset(&foo_inst, 0, sizeof(foo_inst)); will certainly result in all padding bytes being set to zero.
bregma 6y ago
If you rely on code containing undefined behaviour you're in for a world of butthurt sooner rather than later. There is no way in C++ to get at the padding bytes unless you're using undefined behaviour. How does the hash function work? Pointer aliasing using reinterpret_cast? Pointer aliasing using C-style casts? Typing punning through the old union switcheroo?
- gambiting 6y ago
  I don't have the code in front of me, but something like int hash=0; for(int i=0;i<sizeof(obj);i++) hash += hashing_method(reintepret_cast<char*>(&obj)+i); return hash; Basically hashing each byte of the memory containing the object, regardless of what the object itself represents. We can argue whether that's a smart thing to do or not, but I wasn't in charge of implementing it - it's a relic from a codebase that's more than a decade old at this point. It's a simple hashing method that works with most types, but obviously dies horrendously in a case like this.
  - saagarjha 6y ago
    Yeah, this is technically undefined.
  - nly 6y ago
    This doesn't help you, because the contents of the padding bytes is not guaranteed to be anything in particular. Two structs containing identical field values can have different padding bytes. Reading the padding bytes is UB. Just pack the struct and be done with it.
- Asooka 6y ago
  You can trivially get at the padding bytes, just cast to char pointer. Their contents are undefined and there is absolutely no guarantee that they will remain constant, but they're there. Generally you have several possibilities for how to escape this problem, but the simplest is to just add the padding and a static assert that the sizeof the struct is what you expect.
  - 6y ago

munchbunny 6y ago

It's abstracted behind the API's data types as long as you're using C++, but in order to work with the data types you often have to maneuver around the specific packing/alignment. The Windows ETW tracing API has several examples of specific "structure packing" phenomena: 1. Arranging fields to pack things that are shorter than 4 bytes along 4 byte boundaries. 2. Manually arranging data buffers in very specific layouts (this is less about the original post, more about a different interpretation of "packing"). Aside from what I mentioned above, more commonly you will run into this in interop scenarios, such as calling C/C++ API's from C#. Thankfully those scenarios are few and far between.