Page MenuHomePhabricator

[ODRHash] Fix null pointer dereference for ObjC selectors with empty slots.
ClosedPublic

Authored by vsapsai on Jun 25 2019, 1:37 PM.

Details

Summary

Because Selector::getIdentifierInfoForSlot returns NULL if a slot has
no corresponding identifier, use Selector::getNameForSlot instead.

rdar://problem/51615164

Event Timeline

vsapsai created this revision.Jun 25 2019, 1:37 PM
rtrieu added inline comments.Jun 25 2019, 6:45 PM
clang/lib/AST/ODRHash.cpp
73–74

There's actually a second bug here as well. When processing an arbitrary number of elements, the number of elements needs to placed before the list. This line should be added before the for-loop:

ID.AddInteger(NumArgs);

This change isn't directly related to your original patch, so feel free to skip it.

76–80

For possibly null pointers, ODRHash uses a boolean before processing the pointer. The way to fix this is:

const IdentifierInfo *II = S.getIdentifierInfoForSlot(i);
AddBoolean(II)
if (II) {
  AddIdentifierInfo(II);
}
vsapsai added inline comments.Jun 25 2019, 7:29 PM
clang/lib/AST/ODRHash.cpp
73–74

Thanks for the review, Richard. What would be the way to test the suggested changes? I was mostly thinking about making sure we treat as different

  • -foo: and -foo::
  • -foo:bar:: and -foo::bar:

Does it sound reasonable? Maybe you have some test examples for C++ that I can adopt for Objective-C.

rtrieu added inline comments.Jun 26 2019, 2:35 PM
clang/lib/AST/ODRHash.cpp
73–74

Yes, checking things close to each other sounds reasonable. I did pretty much the same thing testing in C++, find things (usually types) that are close to others and making sure the ODRHash can tell them apart. The ObjC testing is a little sparse since I was focusing on the C++ part. Thanks for helping fill it out.

vsapsai updated this revision to Diff 206992.Jun 27 2019, 8:25 PM
  • Address review comments.
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2019, 8:25 PM
vsapsai marked 2 inline comments as done.Jun 27 2019, 8:26 PM
rtrieu accepted this revision.Jun 27 2019, 8:51 PM

LGTM

This revision is now accepted and ready to land.Jun 27 2019, 8:51 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2019, 10:44 AM