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.

Sign up for GitHub

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
Merged

[MetadataReader] Initialize fields to prevent UB. #14400

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions include/swift/Remote/MetadataReader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

unsigned genericHeaderSize = sizeof(GenericContextDescriptorHeader);
bool hasVTable;
bool hasVTable = false;
switch (auto kind = flags.getKind()) {
case ContextDescriptorKind::Module:
baseSize = sizeof(TargetModuleContextDescriptor<Runtime>);
Expand Down