UBSan: check size/type mismatch of global variables declarations across translation units

[llvmbot]

Bugzilla Link	21498
Version	trunk
OS	All
Reporter	LLVM Bugzilla Contributor
CC	@eugenis,@kcc,@zygoloid

Extended Description

C99 states that declaring same global variable with incompatible types in different units of translation is undefined behavior (6.7.5.2 item 6). So e.g. having int a[10]; in tmp.c and int a[100]; in tmp2.c would be a UB. We should be able to check this easily by registering all referenced globals in .c in libubsan via callbacks at program start. Libubsan could than find size/type mismatches and report warning.

[llvmbot]

This approach we will only detect mismatches against libraries that are available at compile time.

True; whether this is appropriate will depend on how we want to handle symbol interposition. However, the linker-checked approach has a major benefit: we may be able to turn it on by default.

Do you mean a dynamic linker here?

No: if we want to detect issues across DSO boundaries, we would need to preserve the relevant data after static linking, which would lead to bloat that is probably unacceptable as a default behavior.

I think the startup overheads could be avoided in default case (when checks are turned off). The additional code/data pages won't be loaded to memory.

The majority of ODR violations we see are between different .so libraries, i.e. the static linker is not involved.

Whether these are ODR violations depends on whether we think that the ELF interposition rules are able to override the C++ ODR rules. This seems very unclear. (Also, one possible reason for this is because the code you're looking at makes very heavy use of .so's.)

This may be true for size difference but I'd argue that type changes (e.g. int vs. float, data vs. code) are always a sign of bug in code.

There seem to be two different classes of bugs we're talking about here:

1) A global is declared with one type, and defined with a different, incompatible type (the feature requested in this bug). This can happen within a single DSO, and depends subtly on C's "compatible type" rules. (This can also happen across DSOs.)

2) A global is defined twice (the feature provided by ASan). This cannot happen within a single DSO, because the static linker would reject, and is in effect turning off the ELF symbol interposition feature.

First of all thanks for making the separation clear. I can't agree with "turning off the ELF symbol interposition feature" though - the interposition would still work under this Asan check provided that it respects types.

[llvmbot]

One more thing: asan already have something very similar. In many cases ubsan is used in the same process as asan. We should avoid building two different but very similar checkers that may end up together in the same process.

Given that 1) this is a UB, not a memory error 2) this does not need heavy ASan machinery I'd propose to have this check in UBSan.

[ec04fc15-fa35-46f2-80e1-5d271f2ef708]

This approach we will only detect mismatches against libraries that are available at compile time.

True; whether this is appropriate will depend on how we want to handle symbol interposition. However, the linker-checked approach has a major benefit: we may be able to turn it on by default.

Do you mean a dynamic linker here?

No: if we want to detect issues across DSO boundaries, we would need to preserve the relevant data after static linking, which would lead to bloat that is probably unacceptable as a default behavior.

The majority of ODR violations we see are between different .so libraries, i.e. the static linker is not involved.

Whether these are ODR violations depends on whether we think that the ELF interposition rules are able to override the C++ ODR rules. This seems very unclear. (Also, one possible reason for this is because the code you're looking at makes very heavy use of .so's.)

There seem to be two different classes of bugs we're talking about here:

1) A global is declared with one type, and defined with a different, incompatible type (the feature requested in this bug). This can happen within a single DSO, and depends subtly on C's "compatible type" rules. (This can also happen across DSOs.)

2) A global is defined twice (the feature provided by ASan). This cannot happen within a single DSO, because the static linker would reject, and is in effect turning off the ELF symbol interposition feature.

[kcc]

One more thing: asan already have something very similar. In many cases ubsan is used in the same process as asan. We should avoid building two different but very similar checkers that may end up together in the same process.

[kcc]

This approach we will only detect mismatches against libraries that are available at compile time.

True; whether this is appropriate will depend on how we want to handle symbol interposition. However, the linker-checked approach has a major benefit: we may be able to turn it on by default.

Do you mean a dynamic linker here? The majority of ODR violations we see are between different .so libraries, i.e. the static linker is not involved.

[ec04fc15-fa35-46f2-80e1-5d271f2ef708]

This approach we will only detect mismatches against libraries that are available at compile time.

True; whether this is appropriate will depend on how we want to handle symbol interposition. However, the linker-checked approach has a major benefit: we may be able to turn it on by default.

[llvmbot]

It's not entirely clear how this should interact with globals in DSOs, however: while the C and C++ standards have rather specific rules about redefinitions, the ELF dynamic linking rules say something different, and explicitly support interposition.

Indeed, AFAIK ODR/DSO semantics is not well defined. GCC folks argue about this every now and then and I think there is still no general consensus (I may be wrong though). E.g. sizeof() does not respect interposition and also C++ symbols with value linkage are treated specially (compiler assumes that they must be equivalent across DSOs).

In any case there are real-world bugs due to accidental name clashes in shared libraries (see e.g. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51146).

What about making the cross-DSO part runtime tunable? I can easily imagine developers who would be interested in strictest checks possible. Really, overriding function with an int would almost always signal a bug.

That said, I'm not entirely convinced that ubsan is the right layer to be providing this check. This seems like something that we should ideally enforce at link time, not on program startup.

This approach we will only detect mismatches against libraries that are available at compile time. If we then run program against different library version or preloaded library, we will not be able to detect mismatches.

Also we won't be able to detect mismatches in dynamically loaded shlibs (plugins).

[ec04fc15-fa35-46f2-80e1-5d271f2ef708]

Historically, ubsan's runtime deliberately didn't have any "run-on-startup" code; it was supposed to be a portable utility library that didn't do any intrusive things like shadow memory or hooking system calls or whatever. The main motivation for that was to allow libubsan to be linked to a DSO that was dynamically loaded into uninstrumented code. Since we've already abandoned that goal, I don't think there's any further impediment here.

It seems simple enough to support this: inject an attribute((constructor)) function into each TU that tells ubsan about the globals in that TU. It's not entirely clear how this should interact with globals in DSOs, however: while the C and C++ standards have rather specific rules about redefinitions, the ELF dynamic linking rules say something different, and explicitly support interposition.

That said, I'm not entirely convinced that ubsan is the right layer to be providing this check. This seems like something that we should ideally enforce at link time, not on program startup. Perhaps we could add an extra section to the .o files containing the relevant information, and teach the linker to check them?

[llvmbot]

+Richard

sounds like a viable feature.

[llvmbot]

Off the top of my head I don't see any way to implement this w/o shadow memory (i.e. in ubsan) as efficiently

Well, in each TU we could generate callbacks to register declarations inside UBSan runtime. Runtime could then store them in some hashtable and use it to verify whether all calls report matching info for each symbol. Having a hashtable is an overhead but it could be dropped once program initialization is done. Ctors would eat some memory though.

[kcc]

Mismatch between declaration and definition? Hmmmm. That's different indeed. Please either rename this bug or file a separate one.

The current ODR violation detector in asan relies on shadow memory. Off the top of my head I don't see any way to implement this w/o shadow memory (i.e. in ubsan) as efficiently

[llvmbot]

For Asan this only detects duplicate definitions, I think we can also detect mismatch in declarations (e.g. one module declares extern int a[10] wheres a is really defined as int a[10]). As for Asan, this really sounds like an UBSan feature.

[kcc]

This feature is present in asan: https://code.google.com/p/address-sanitizer/wiki/OneDefinitionRuleViolation