New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

undefined behaviour when emulating offsetof #1450

Closed
xyzzyz opened this Issue Apr 25, 2016 · 16 comments

Comments

Projects
None yet
7 participants
@xyzzyz
Contributor

xyzzyz commented Apr 25, 2016

(context: https://bugs.chromium.org/p/chromium/issues/detail?id=605933)

The UBSan detects undefined behaviours in protobuf code which are using reinterpret_cast on invalid pointers to emulate offsetof() macro. These are:

  1. GOOGLE_PROTOBUF_GENERATED_MESSAGE_FIELD_OFFSET macro that tries to emulate __builtin_offsetof and makes are reinterpret_casts on an invalid pointer (16):
    https://code.google.com/p/chromium/codesearch#chromium/src/third_party/protobuf/src/google/protobuf/generated_message_reflection.h&q=GOOGLE_PROTOBUF_GENERATED_MESSAGE_FIELD_OFFSET&sq=package:chromium&type=cs&l=590

  2. ZR_HELPER that appears in the generated files:

define ZR_HELPER_(f) reinterpret_cast<char*>(\

&reinterpret_cast<BaseLayerProperties*>(16)->f)

Some issue: invalid reinterpret_cast.

The first issue might be fixed by defining GOOGLE_PROTOBUF_GENERATED_MESSAGE_FIELD_OFFSET as __builtin_offsetof and suppressing -Winvalid-offsetof.

For the second, the for the generated code should be in third_party/protobuf/src/google/protobuf/compiler/cpp/cpp_message.cc:

The following macro needs to be changed:

  • const char* macros =
  •  "#define ZR_HELPER_(f) __builtin_offsetof($classname$, f)\n\n"
    
  •  "#define ZR_(first, last) do {\\n"
    
  •  "  ::memset(&first, 0,\\n"
    
  •  "           ZR_HELPER_(last) - ZR_HELPER_(first) + sizeof(last));\\n"
    
  •  "} while (0)\n\n";
    

Note that using __builtiin_offsetof() on non-POD types is still undefined behaviour, but seems like it is better suited for the job, and less likely to break due to compiler optimizing it out.

@gkrasin

This comment has been minimized.

gkrasin commented Apr 25, 2016

To clarify:

-- Using __builtin_offsetof() on non-POD types is undefined behavior, but it's equivalent to the reinterpret_cast scheme used in the macros above. So, using __builtin_offsetof() is not worse than the current state of things.

-- Making reinterpret_cast on naked invalid pointers prevents dynamic tools, like UBSan vptr ([1]) or LLVM CFI ([2]) to work correctly. In Chrome, we aim to enable LLVM CFI for the official Chrome binaries (on Linux first), which should have a measurable positive effect on the overall Chrome security.

  1. http://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#availablle-checks
  2. http://clang.llvm.org/docs/ControlFlowIntegrity.html
  3. https://www.chromium.org/developers/testing/control-flow-integrity

My proposal would be to use __builtin_offsetof() for the compilers which support it (like, Clang or GCC) , and suppress the corresponding warning (-Winvalid-offsetof) with a pragma. It's probably okay to keep the current scheme for the compilers we don't know much about, just in case if they implement things significantly different and also to ensure that the code would compile without warnings.

@xfxyjwf xfxyjwf added bug c++ labels Apr 25, 2016

@xfxyjwf

This comment has been minimized.

Contributor

xfxyjwf commented Apr 25, 2016

Am I right that we don't have a way to fix the undefined behavior and the solution is just to suppress warnings/errors in the compiler/build tool? If so, instead of using __builtin_offsetof which will trigger a compiler warning, can we add something specific for UBSan or LLVM CFI to let them not complain?

@xfxyjwf

This comment has been minimized.

Contributor

xfxyjwf commented Apr 25, 2016

Note that the GOOGLE_PROTOBUF_GENERATED_MESSAGE_FIELD_OFFSET macro was introduced because offsetof() will trigger a GCC compiler warning:

// Returns the offset of the given field within the given aggregate type.
// This is equivalent to the ANSI C offsetof() macro.  However, according
// to the C++ standard, offsetof() only works on POD types, and GCC
// enforces this requirement with a warning.  In practice, this rule is
// unnecessarily strict; there is probably no compiler or platform on
// which the offsets of the direct fields of a class are non-constant.
// Fields inherited from superclasses *can* have non-constant offsets,
// but that's not what this macro will be used for.

If we are not actually fixing the undefined behavior anyway, I would prefer an approach that will not add back the warning since the warning will likely cause problems to the Google internal build of protobuf (where we don't have control over the compiler flags and warnings are treated as errors).

@gkrasin

This comment has been minimized.

gkrasin commented Apr 25, 2016

Well, the current scheme has two undefined behaviors:

  1. It relies on the specific layout of the fields within a non-POD type (just like __builtin_offsetof)
  2. It makes a reinterpret_cast on a naked pointer 0x10

So, we can get rid of at least of one of them and keep only the necessary evil in the code.

@gkrasin

This comment has been minimized.

gkrasin commented Apr 25, 2016

At least in Clang, the following works:

#define GOOGLE_PROTOBUF_GENERATED_MESSAGE_FIELD_OFFSET(TYPE, FIELD)    \
  _Pragma("clang diagnostic push")                                     \
  _Pragma("clang diagnostic ignored \"-Winvalid-offsetof\"")           \
  __builtin_offsetof(TYPE, FIELD)                                      \
  _Pragma("clang diagnostic pop")
#else

It turns off the warning, but only for a single line. That should work for GCC too, I believe.

@gkrasin

This comment has been minimized.

gkrasin commented Apr 25, 2016

Sorry, missed this message:

Am I right that we don't have a way to fix the undefined behavior and the solution is just to suppress warnings/errors in the compiler/build tool? If so, instead of using __builtin_offsetof which will trigger a compiler warning, can we add something specific for UBSan or LLVM CFI to let them not complain?

Both UBSan and LLVM CFI rely on reading the vtable pointers to determine if the cast is vaild. In case of UBSan, it should be not hard to just suppress a report for reinterpret_cast(0x10). It's imperfect, as it would make it harder to find bugs in other programs, but it's doable.

For CFI, it's a harder sell, as it's not a bug-finding tool, it's a security hardening approach and invalid reinterpret_cast would defeat the defense. If we allow the cast, we'll also need to prove that the value is not then used to call virtual functions, and running such escape analysis is prohibitively complex in both the time to implement and time to run. It will never be perfect too.

@xfxyjwf

This comment has been minimized.

Contributor

xfxyjwf commented Apr 25, 2016

@gkrasin I was not suggesting changing UBSan or LLVM CFI. I'm thinking whether we can add some annotations to protobuf code so UBSan/LLVM CFI will either see a different code path or simply ignore the problem for that specific part of code. For example, we added some code in protobuf for ASan:
https://github.com/google/protobuf/blob/814685ca2cd9280ca401e1842fd6311440921a0a/src/google/protobuf/arena.cc#L141

If that is not possible, I think it's also acceptable to change it back to offsetof() providing we can suppress the warning and verify it works well in Google internal build.

@gkrasin

This comment has been minimized.

gkrasin commented Apr 25, 2016

Yes, maybe. Let me take a look / ask around.

@pcc

This comment has been minimized.

Contributor

pcc commented Apr 25, 2016

@xfxyjwf either of those approaches would work, but I would slightly prefer the approach of using offsetof(), as the suppression would be more localised. You can suppress the warning by using pragmas as Ivan mentioned.

If you do decide to go with the first approach, you can use the no_sanitize http://clang.llvm.org/docs/AttributeReference.html#no-sanitize-clang-no-sanitize attribute on each function that uses the macro.

@gkrasin

This comment has been minimized.

gkrasin commented Apr 25, 2016

@pcc no sanitize attribute for each function that uses a macro seems unpractical to me (based on the exploration of proto3 code). It's also unnecessary coarse compared to the use of __builtin_offsetof and enabling / disabling pragmas like I do above, where a single line is excluded from the compiler diagnostics.

@pcc

This comment has been minimized.

Contributor

pcc commented Apr 25, 2016

@gkrasin we're in agreement then.

@xfxyjwf

This comment has been minimized.

Contributor

xfxyjwf commented Apr 26, 2016

If using offsetof is preferred, can someone help send me a CL to update our internal code base first? Just want to make sure the change will work with our internal compiler toolchain.

@krasin

This comment has been minimized.

krasin commented Apr 26, 2016

Sure!

@krasin

This comment has been minimized.

krasin commented Apr 28, 2016

The fix is submitted into the internal Google repo, and it is expected to reach github in 2-3 weeks, if everything goes smooth.

Chromium applied a temporary patch, see https://crbug.com/607751

@xfxyjwf xfxyjwf closed this Mar 7, 2017

@abigailbunyan

This comment has been minimized.

abigailbunyan commented Apr 3, 2017

(For anybody else doing some software archaeology here: the fix made it to this repository in cf14183, which made it into released versions v3.0.0-beta-3 and upwards.)

@joshkel

This comment has been minimized.

Contributor

joshkel commented Apr 23, 2017

This is a total hack, but in case anyone wants to get Protocol Buffers 2.6.1 working in UBSan without modifying the source (e.g., so that you can continue to use Ubuntu LTS's packaging of protobuf), I put together a gist with instructions how.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment