Page MenuHomePhabricator

[SemaObjC] Fix crash while parsing type arguments and protocols
ClosedPublic

Authored by bruno on Aug 24 2016, 1:56 PM.

Details

Summary

Fix a crash-on-invalid.

When parsing type arguments and protocols, ParseTypeName() tries to find
matching tokens for '[', '(', etc whenever they appear among potential
type names. If unmatched, ParseTypeName() yields a tok::eof token
stream. This leads to crashes since the parsing at this point is not
expected to go beyond the param list closing '>'.

rdar://problem/25063557

Diff Detail

Event Timeline

bruno updated this revision to Diff 69166.Aug 24 2016, 1:56 PM
bruno retitled this revision from to [SemaObjC] Fix crash while parsing type arguments and protocols.
bruno updated this object.
bruno added a reviewer: doug.gregor.
bruno added subscribers: cfe-commits, manmanren.
doug.gregor edited edge metadata.Aug 25 2016, 9:16 AM

This will work, but it's *really* unfortunate to put tentative parsing into this code path because tentative parsing is far from free, and specialized Objective-C types are getting pretty common in Objective-C headers. Why can't the callers be taught to handle EOF and bail out?

bruno updated this revision to Diff 69456.Aug 26 2016, 6:39 PM
bruno updated this object.
bruno edited edge metadata.

Updated patch and changed approach after Doug's comment.

This will work, but it's *really* unfortunate to put tentative parsing into this code path because tentative parsing is far from free, and specialized Objective-C types are getting pretty common in Objective-C headers. Why can't the callers be taught to handle EOF and bail out?

The unfortunate part of that is it makes all of the callers fragile -- we have to sprinkle "are we at EOF now" checks all over, or risk running into the same problem later. We don't typically do that (there are only 7 eof checks in the parser currently, this adds 6 more). I don't know which option is worse.

bruno added a comment.Sep 6 2016, 12:00 PM

Ping!

This will work, but it's *really* unfortunate to put tentative parsing into this code path because tentative parsing is far from free, and specialized Objective-C types are getting pretty common in Objective-C headers. Why can't the callers be taught to handle EOF and bail out?

The unfortunate part of that is it makes all of the callers fragile -- we have to sprinkle "are we at EOF now" checks all over, or risk running into the same problem later. We don't typically do that (there are only 7 eof checks in the parser currently, this adds 6 more). I don't know which option is worse.

Likewise, I'm not sure what's the best tradeoff. And there's probably more to come: some part of PR23057 is likely the need to properly handle EOFs.

doug.gregor accepted this revision.Sep 12 2016, 3:49 PM
doug.gregor edited edge metadata.

I don't love the fact that it makes callers fragile, but having to do tentative parsing for these otherwise-unambiguous cases is expensive (in compile time). We should only use tentative parsing when we have an actual ambiguity to resolve.

This revision is now accepted and ready to land.Sep 12 2016, 3:49 PM
bruno closed this revision.Sep 13 2016, 1:14 PM

Thanks Doug!

Committed r281383