-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[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
Conversation
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.
@swift-ci please smoke test and merge |
Doh, good catch! |
@@ -933,9 +933,9 @@ class MetadataReader { | |||
sizeof(flags))) | |||
return nullptr; | |||
|
|||
unsigned baseSize; | |||
unsigned baseSize = 0; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Also, is it possible to add a test case in the swift codebase that would exercise the code or does it already exist? |
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. |
This helps catching trivial mistakes & UB, e.g. uninitialized booleans, see, e.g. swiftlang#14400
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.