Skip to content

[MetadataReader] Initialize fields to prevent UB. #14400

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

Merged
merged 1 commit into from
Feb 4, 2018

Conversation

dcci
Copy link
Member

@dcci dcci commented Feb 3, 2018

hasVtable is not initialized on all paths, so when we read it,
we end up reading a random value. On debug builds, this value
almost always happen to be zero, so we do the right thing. This
isn't necessarily true when the optimizations kick in.

lldb uses metadata reder to resolve types and as result of this
undefined behaviour sometimes gets wrong results back, causing
a bunch of tests to fail in release builds.

hasVtable is not initialized on all paths, so when we read it,
we end up reading a random value. On debug builds, this value
almost always happen to be zero, so we do the right thing. This
isn't necessarily true when the optimizations kick in.

lldb uses metadata reder to resolve types and as result of this
undefined behaviour sometimes gets wrong results back, causing
a bunch of tests to fail in release builds.
@dcci
Copy link
Member Author

dcci commented Feb 3, 2018

@swift-ci please smoke test and merge

@jckarter
Copy link
Contributor

jckarter commented Feb 3, 2018

Doh, good catch!

@@ -933,9 +933,9 @@ class MetadataReader {
sizeof(flags)))
return nullptr;

unsigned baseSize;
unsigned baseSize = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should get warnings from Clang if we miss an initialization in the switch below. (Not sure why we didn't see one for hasVTable…). Instead of pre-initializing the values here, does it work to initialize hasVTable = false; in the Module, Extension, and Anonymous branches? It looks like baseSize already gets initialized on every path, and if we add new cases in the future, it'd be nice to get the compiler warnings to ensure they're set intentionally for the new case.

Copy link
Member Author

@dcci dcci Feb 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it works, but I'm afraid this will show up again if somebody adds a new case to the switch.
Maybe an alternative would be having a three state boolean (and then add an assertion at the end of the switch/case to make sure it's initialized?

FWIW, this PR/problem is one of the reasons why I think languages without definite initialization semantics for types are particularly hard to work with).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's the weird thing—Clang does have definite initialization warnings for trivial types that should have fired here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, fun fact, it seems that some tests in swift/test happen to fail under ubsan with your commit, but the issue was hidden by other failures early in the build (the UBsan build is not clean). I think we can try to put some energy to get the UBsan build green for swift as a second line of defense (although, for simple types like bool or int, clang warnings should always fire)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, sigh, clang actually warns on this only if -Wuninitialized is passed (which is part of -Wall but not of the default set of warnings).

see, e.g.

int patatino(int banana) {
  int tinkywinky = 27;
  bool initializeMe;

  switch (banana) {
    case 0:
      break;
    case 1:
      break;
    case 2:
      //initializeMe = false;
      break;
    default:
      break;
  }

  if (initializeMe) {
    tinkywinky = 12;
  }

  return tinkywinky;
}
dcci@Davides-MacBook-Pro ~> clang++ patatino.cpp -c
dcci@Davides-MacBook-Pro ~> clang++ patatino.cpp -c -Wall
patatino.cpp:17:7: warning: variable 'initializeMe' is uninitialized when used
      here [-Wuninitialized]
  if (initializeMe) {
      ^~~~~~~~~~~~
patatino.cpp:3:20: note: initialize the variable 'initializeMe' to silence this
      warning
  bool initializeMe;
                   ^
                    = false
1 warning generated.

Should we consider enabling -Wuninitialized (or -Wall) for the swift codebase? IIRC the reason why warnings are part of -Wall but not of the default set is because they might produce false positive, but you could always annotate places to placate them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I think we have -Wuninitialized on for the compiler, but maybe not the runtime. I think we ought to have it on for both.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jckarter I'll be more than happy to submit a pull request to enable this tomorrow (and fix the failures, if any).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds great! Thanks again for tracking this down and fixing it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we enable not only -Wuninitialized but -Wall for the runtime?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AnnaZaks yes, it's reasonable, let me do it now.

@swift-ci swift-ci merged commit c345043 into swiftlang:master Feb 4, 2018
@AnnaZaks
Copy link
Contributor

AnnaZaks commented Feb 5, 2018

Also, is it possible to add a test case in the swift codebase that would exercise the code or does it already exist?

@jckarter
Copy link
Contributor

jckarter commented Feb 5, 2018

There are already tests for this code in the Swift codebase. Somehow they all managed to get lucky and not trip on the uninitialized variable here.

dcci pushed a commit to dcci/swift that referenced this pull request Feb 5, 2018
This helps catching trivial mistakes & UB, e.g. uninitialized
booleans, see, e.g. swiftlang#14400
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants