Page MenuHomePhabricator

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

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

Details

Summary

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

rdar://problem/51615164

Diff Detail

Repository
rL LLVM

Event Timeline

vsapsai created this revision.Tue, Jun 25, 1:37 PM
rtrieu added inline comments.Tue, Jun 25, 6:45 PM
clang/lib/AST/ODRHash.cpp
73 ↗(On Diff #206522)

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.

75 ↗(On Diff #206522)

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.Tue, Jun 25, 7:29 PM
clang/lib/AST/ODRHash.cpp
73 ↗(On Diff #206522)

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.Wed, Jun 26, 2:35 PM
clang/lib/AST/ODRHash.cpp
73 ↗(On Diff #206522)

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.Thu, Jun 27, 8:25 PM
  • Address review comments.
Herald added a project: Restricted Project. · View Herald TranscriptThu, Jun 27, 8:25 PM
vsapsai marked 2 inline comments as done.Thu, Jun 27, 8:26 PM
rtrieu accepted this revision.Thu, Jun 27, 8:51 PM

LGTM

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