Hacker News Reader

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!

>> some structures would be packed differently in different compilation units.

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

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.

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

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?

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.

Yeah, this is technically undefined.

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.

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.

> Their contents are undefined

No, their contents has an unspecified value.

Ok, this will sound stupid then, but is pointer aliasing using casts undefined?

Not stupid–pointer aliasing using a cast is undefined if the types do not match, unless the type you are converting to is a void * , char * , or unsigned char *.

> 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.

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.

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.

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.

sizeof will include padding, but there is no need for memset to actually perform a write that cannot be consistently observed.

Are you saying memset does not have to write the entire struct? I understand it can be optimized out, but thought it if it was actually executed it had to set all of the specified bytes.

memset must write to the bytes that underly the members of the struct. What it does to padding bytes is not of consequence to you, as you cannot observe it as far as I understand. (That is, if you tried to read it out it would not necessarily be the value you had memset, or even be consistent across reads.)

memset definitely writes to padding bytes, as noted by several other folks here. you observe these padding bytes during serialization and deserialization (disk, network, memory mapped buffer, etc.)

Note this is important enough that compiler folks consider it a bug if it doesn't work correctly. see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92486 for a recent example involving memset and memcpy.

As mentioned in the bug report, the value of padding bytes is unspecified by the C standard. Actually, according to the linked Defect Report mentioned in the bug, there is a bit of discussion on whether the functions you mentioned should be entirely undefined to call or if some exceptions should be carved out. If the functions are legal to call, it is very likely that the results will be some arbitrary reification of the unspecified value.

There is some discussion of that, but it is wrong. There are many comments there, so you should read the whole bug report. The value of the padding bytes must be what is specified in memset.

Reading padding bytes is UB, so the compiler could choose to optimize that memset call so that the padding bytes aren't touched.

memset is absolutely required to zero out padding bytes. Padding is part of the structure size, and memset hast to zero out exactly as many bytes as it is told.

memseting a structure to zero is commonly done in programs that send structure outside of the process (like passing it to communication or storage-related system calls) because the padding can leak sensitive information. That better work!

If the size of a structure didn't include the padding, then pointer arithmetic on structures wouldn't work correctly, and arrays of them would be broken/impossible. Arrays are the reason for the padding; given a struct foo * p, we need p + 1 to be properly aligned (for the sake of accessing all the members of * (p + 1). So struct foo cannot have a size like 5, if it contains a member of type int or anything else with alignment requirements.

memset is not required to do anything if the side effects are not observable; this is the entire reason why memset_s needed to be added (and also why __attribute__((packed)) is recommended for anything that is being sent directly over the wire, if this kind of construction cannot be avoided). Compilers often inline and unroll small, constant-size memsets anyways, and since it is not possible to observe a consistent padding in C in a standards-compliant way to my knowledge, an optimizing compiler should be able to legally leave those bits alone.

Well, the contents of the padding bits is legally observable (with a memcpy for example) so the compiler can't assume they are unovservable.

As far as I understand, padding bits are indeterminate until you observe them via a memcpy into a buffer, at which point they will collapse (only in the buffer) to some arbitrary but now-constant value. Is this correct?

I think you are right in the general case. Definitely padding bits are not preserved during copies, but I think that at least in some cases they might be preserved, at least I think that if you memset+memcpy, the memcpy is guaranteed to see the zeros (what if you want to end the lifetime of the object and simply reuse the storage as an array of chars?). But there is divergence between C and C++ and also an area quite in flux (the notion of lifetimes of objects and memory locations gets tweaked every standard in C++, not sure where we are at now).

edit: interestingly, Biriba is yet another game in the canasta family, my theory is that different groups were originally playing these differents variants, but when they gained knowledge of the more popular variant (burraco), they started playing it but kept calling it with the original name. I guess that up until the internet era, these games were mostly passed via oral knowledge in casual groups.

edit2: macchiavelli [1] is another very fun game of the same family, but a lot more puzzle solving oriented.

[1] https://en.wikipedia.org/wiki/Machiavelli_(Italian_card_game...

I looked it up for C, and as far as I can tell this has not standardized. (The Defect Report I read, which seems to have the latest opinion, literally calls such things "wobbly values". I'm not joking.) The TL;DR of Defect Report #451 seems to be that this is needs some work from the standard. In the case of an automatic struct or whatever (so you can't pull the bytes out from under it and reuse it) I think the consensus seems to be tending towards Heisenburg-ish values that have values that can change between reads, and certain library functions will be legal to call on them so that they "collapse" the state in the view that you've just created. For your case, I would have the question of whether two objects (e.g. a char array and a struct carved from that array) can share the same memory; in this case I would intuitively tend to agree with you that the padding bits are visible from the longer-lived object, kind of like an ad-hoc union. But I am not a licensed language lawyer, so I am not quite sure on that point.

I appreciate your edits, by the way ;)

Thanks for hunting down the details.

And it seem that I should double check which comment I'm editing before submitting :)

memset_s is a pointless misinvention. I can't think of any situation in which I would use memset, and not want it to be what memset_s is claimed to be.

Which is to say that memset_s should just be called memset; there is no need for memset to be doing stupid things so that people must use memset_s.

I have no plans to use memset_s (ever), or to upstream any fix that involves using it unless the author provides a repro test case, and a proof that the problem can't be fixed with compiler options that make the problem go away with memset.

In the worst imaginable scenario, I will #define memset memset_s everywhere (after the inclusion of <string.h> of course, not before, and an #undef memset).

(The #undef memset may be enough, in fact, if the only problem is that memset is #define'd to some compiler built-in that doesn't properly implement classic C90 memset in all cases.)

Most of the Annex K * _s functions are pretty stupid, to be honest, but this is one that is actually useful. It's not that memset is doing stupid things; rather it's being smart and skipping work that it's not required to do.

memset has always been required to set N bytes starting at a given address to zero. That's the requirement. Been that way since it appeared in AT&T Unix and beat BSD's bzero to the ANSI C punch.

There is no need for a broken memset that fails to set some of the bytes, so that a fixed one under a different name has to be used in its place.

If you're using memset such that it's okay for memset not to set some of the bytes, and you'd like them not to be set if that makes things faster, then you shouldn't be using memset. You're using a hammer to drive a screw: wrong tool.

C has perfectly good initialization and assignment for structures.

End of story; I'm going to walk away pretend I never read this subthread, re-joining the hordes of C programmers using memset in the normal way, adding to the countless lines of code that do it that way and are never going to be changed.

This bullshit will be backpedaled out of the standard eventually, you just wait.

…and assigning to a variable is supposed to put some bytes at a certain address, yes. Except compilers will skip doing so if you never read from the variable again, and the same thing happens here: the compiler can "know" that it has no need to actually do the assignment, as writing to padding bytes is not required to actually do anything and trying to observe the value is currently the subject of debate but somewhere between "undefined" and "whatever you get out of it has nothing to do with what you think went in".

Agree. memset should be secure by default. That means it should not be optimized away by the compiler. And second the secure variant should flush the content, so that cache attacks can not read secrets which were memset'ed, but still in the cache on broken CPU's (Intel).

sloooooooooooooooow. No, really, do you want a full flush every time you do a memset? That would be horrendous for performance.

"And second the secure variant should flush ..."

Probably means the extra-secure variant, more secure than the regular non-optimized-away memset that really clears the memory, but without the flush.

Yeah, that's a more reasonable interpretation.

I've been duped into participating in misleading thread without brushing up on this. The problem is that memset can be entirely optimized away when it's a dead store, which is reasonable:

  {
    struct foo x;
    // sensitive calculation with x
    memset(&x, 0, sizeof x);
  }

Basic liveness analysis (compiler technique from the 1970's if not older) tells us that the object has no "next use" at the point where it is being written by memcpy. That's a dead store that can be eliminated. The object is about to become toast. This is a problem for sensitive code (e.g. crypto).

I've been discussing only this case:

  {
    struct foo x;
    memset(&x, 0, sizeof x);
    // init x
    syscall(&x, sizeof x);
  }

Here, the memset cannot be optimized away. So we can only have some academic discussion about how part of the memset could be optimized away: that part which flosses the structure padding between the members and at the end.

That's a stupid and dangerous optimization that threatens a whole lot of code in the wild.

A good defense against this sort of time-wasting nonsense is "I'm not fixing anything without a repro test case; have a nice day".

As I was discussing with 'gpderetta, it's complicated: https://news.ycombinator.com/item?id=23002423. Depending on how the followup to the DR I mentioned goes, your call is somewhere between "illegal as this invokes undefined behavior" and "sending structs across the wire better not be an infoleak". We'll see where the standards people take this. (Personally, I'm of the opinion that reading padding should always give an unspecified value–that is, a read can give an arbitrary, possibly inconsistent but valid value for that byte, but copies of it would have a fixed value. Code in the wild that has historically not cared for standards compliance anyways, e.g. Linux, should get a GCC flag for "make my padding bits what I want them to be".)

Wrong answer. This is brittle and relies on the memory always being initialized correctly and will be prone to all sorts of issues in the wild. Better is just either templatize on the key type or store the size on the map.

The answer is wrong more because it relies on padding bytes having consistent values than a lack of templating.

To be clear, there are obviously different/better ways to handle this in C++. We're talking about K&R C here.

Right I mentioned "store size of the key on the map" as the second option for K&R/Ansi C.

memset, sure, then you copy the struct (return it or pass it by value) and the compiler won't bother copying the padding bytes. Or worse, an optimizing compiler will see that you're writing to padding bytes and helpfully no-op it.

A compiler that optimizes away memset is not one that could be used for targeting anything that sits on a network.

That immediately rules out GCC and Clang…

I don't think so, unless we're mistaking memset for __builtin_memset.

GCC treats the two identically. See https://github.com/gcc-mirror/gcc/blob/229c0ef777161ec5adfb7...

I see; because the sixth argument (BOTH_P) in that macro call is true.

First of all, there is no way the compiler is going to optimize a call to memset(&foo, 0, sizeof(foo)) when &foo is being interpreted as a void pointer. That doesn't even make sense.

Second of all, in a generic C interface keys are likely to be treated as void pointer and almost certainly are going to be moved around with memcpy etc. rather than returned/passed by value since doing so would make the interface non-generic.

The HN markdown ate your stars.

Thanks for letting me know, should be better now! :-)

Basically, put a space after the star that you want to render verbatim, and don't try to do that in a region of text that is already italicized with stars.

E.g. "... being interpreted as a void * type."

The compiler knows what memset does. One of the earliest steps of optimization is replacing calls to well-known functions with intrinsics. Try it! https://godbolt.org/z/QUREQi

Yes, it's true that generic C code will type-erase the key type. However it just takes a little refactoring in specific code to move the struct initialization across a call boundary from where it is passed to the generic code.

The compiler knows that memset clobbers an object, and can classify that as a dead store.

I'm skeptical about compilers optimizing memset not to cover padding between structure members.

Firstly, that would introduce security holes into a heck of a lot more existing code compared to code that uses a dead-store memset to wipe sensitive crypto.

Secondly, it wouldn't run any faster. Gaps in a structure and at the end exist in order to eliminate misalignment. Before most padding, there is a member that ends on a misaligned address. It's slower to update just that member, and leave the padding alone, than to clobber the padding.

For instance if we have a { char a; int b; char c; } structure, we gain nothing by zeroing just one byte of a, b and c.

In some compiler for an 8 bit system, this reasoning is likely false; I will worry about it when porting to that. Very little existing code will fit; you're coding from scratch for such things.

> First of all, there is no way the compiler is going to optimize a call to memset(&foo, 0, sizeof(foo)) when &foo is being interpreted as a void pointer. That doesn't even make sense.

that is really dangerous to assume. memset is a compiler built-in in every relevant C++ compiler and the compiler definitely knows the type of the object that is behind your void* and knows if you're being nasty.

e.g. look at this code : https://gcc.godbolt.org/z/xh9BXs

it's UB, and the compiler knows it and inserts an "invalid opcode" instruction even if you try to hide a memset behind a void*-taking function

-Ofast makes non-compliant programs anyways, so it's not the best example.

I had -Ofast in there but you can check that it's the same with -O2

One solution consists of using memset to initialize the structure to zero, and using memcpy to copy it instead of structure assignment. (Problem: pass-by-value in function calls won't use memcpy; abstractly, it uses member-for-member assignment which is not required to copy padding. A function that wants to calculate the correct has has to prepare a blank object with memset, then individually assign the fields into it from the incoming object.)

Another solution, more along the lines of what I was thinking, is simply to associate the hash table with a hashing function which processes the type as a structure, hashing the members individually rather than as a pad of memory.

C++ templates refine this by adding the ability to deduce the hashing function statically, and possibly inline it, which we could do with some preprocessing in C, along the lines of how those TAILQ macros from BSD work for linked lists.

> Another solution, more along the lines of what I was thinking, is simply to associate the hash table with a hashing function which processes the type as a structure, hashing the members individually rather than as a pad of memory.

This solution is probably what I would go for in most cases as well. The most compelling reasons I can think of for going the other way would be if there were a desire to use a specific hashing function/algorithm on all keys regardless of type or if there were a desire to have keys of different types in the same map.

That's true but missing the point.

The point is that C++20 adds a way to catch yourself before you make the exact type of mistake the grandparent comment talks about.

https://en.cppreference.com/w/cpp/language/constraints