Re: [PATCH] D23852: [SemaObjC] Fix crash while parsing type arguments and protocols
bruno closed this revision. bruno added a comment. Thanks Doug! Committed r281383 https://reviews.llvm.org/D23852 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23852: [SemaObjC] Fix crash while parsing type arguments and protocols
doug.gregor accepted this revision. doug.gregor added a comment. This revision is now accepted and ready to land. 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. https://reviews.llvm.org/D23852 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23852: [SemaObjC] Fix crash while parsing type arguments and protocols
bruno added a comment. Ping! https://reviews.llvm.org/D23852 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23852: [SemaObjC] Fix crash while parsing type arguments and protocols
bruno added a comment. Ping! In https://reviews.llvm.org/D23852#529308, @aaron.ballman wrote: > In https://reviews.llvm.org/D23852#525291, @doug.gregor wrote: > > > 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. https://reviews.llvm.org/D23852 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23852: [SemaObjC] Fix crash while parsing type arguments and protocols
aaron.ballman added a subscriber: aaron.ballman. aaron.ballman added a comment. In https://reviews.llvm.org/D23852#525291, @doug.gregor wrote: > 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. https://reviews.llvm.org/D23852 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23852: [SemaObjC] Fix crash while parsing type arguments and protocols
bruno added a comment. Ping! https://reviews.llvm.org/D23852 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23852: [SemaObjC] Fix crash while parsing type arguments and protocols
bruno updated the summary for this revision.
bruno updated this revision to Diff 69456.
bruno added a comment.
Updated patch and changed approach after Doug's comment.
https://reviews.llvm.org/D23852
Files:
lib/Parse/ParseDecl.cpp
lib/Parse/ParseObjc.cpp
lib/Parse/Parser.cpp
test/SemaObjC/crash-on-type-args-protocols.m
Index: test/SemaObjC/crash-on-type-args-protocols.m
===
--- /dev/null
+++ test/SemaObjC/crash-on-type-args-protocols.m
@@ -0,0 +1,40 @@
+// RUN: %clang_cc1 -DFIRST -fsyntax-only -verify %s
+// RUN: %clang_cc1 -DSECOND -fsyntax-only -verify %s
+// RUN: %clang_cc1 -DTHIRD -fsyntax-only -verify %s
+// RUN: %clang_cc1 -DFOURTH -fsyntax-only -verify %s
+
+@protocol P;
+@interface NSObject
+@end
+@protocol X
+@end
+@interface X : NSObject
+@end
+
+@class A;
+
+#ifdef FIRST
+id F1(id<[P> v) { // expected-error {{expected a type}} // expected-error {{use of undeclared identifier 'P'}} // expected-error {{use of undeclared identifier 'v'}} // expected-note {{to match this '('}}
+ return 0;
+}
+#endif
+
+#ifdef SECOND
+id F2(id v) { // expected-error {{unknown type name 'P'}} // expected-error {{unexpected interface name 'X': expected expression}} // expected-error {{use of undeclared identifier 'v'}} // expected-note {{to match this '('}}
+ return 0;
+}
+#endif
+
+#ifdef THIRD
+id F3(id v) { // expected-error {{unknown type name 'P'}} // expected-error {{expected expression}} // expected-error {{use of undeclared identifier 'v'}} // expected-note {{to match this '('}}
+ return 0;
+}
+#endif
+
+#ifdef FOURTH
+id F4(id v { // expected-error {{unknown type name 'P'}} // expected-error {{expected ')'}} // expected-note {{to match this '('}} // expected-note {{to match this '('}}
+ return 0;
+}
+#endif
+
+// expected-error {{expected '>'}} // expected-error {{expected parameter declarator}} // expected-error {{expected ')'}} // expected-error {{expected function body after function declarator}}
Index: lib/Parse/Parser.cpp
===
--- lib/Parse/Parser.cpp
+++ lib/Parse/Parser.cpp
@@ -1513,6 +1513,8 @@
NewEndLoc);
if (NewType.isUsable())
Ty = NewType.get();
+ else if (Tok.is(tok::eof)) // Nothing to do here, bail out...
+return ANK_Error;
}
Tok.setKind(tok::annot_typename);
@@ -1744,6 +1746,8 @@
NewEndLoc);
if (NewType.isUsable())
Ty = NewType.get();
+else if (Tok.is(tok::eof)) // Nothing to do here, bail out...
+ return false;
}
// This is a typename. Replace the current token in-place with an
Index: lib/Parse/ParseObjc.cpp
===
--- lib/Parse/ParseObjc.cpp
+++ lib/Parse/ParseObjc.cpp
@@ -344,9 +344,11 @@
protocols, protocolLocs, EndProtoLoc,
/*consumeLastToken=*/true,
/*warnOnIncompleteProtocols=*/true);
+ if (Tok.is(tok::eof))
+return nullptr;
}
}
-
+
// Next, we need to check for any protocol references.
if (LAngleLoc.isValid()) {
if (!ProtocolIdents.empty()) {
@@ -1814,6 +1816,8 @@
protocolRAngleLoc,
consumeLastToken,
/*warnOnIncompleteProtocols=*/false);
+ if (Tok.is(tok::eof)) // Nothing else to do here...
+return;
// An Objective-C object pointer followed by type arguments
// can then be followed again by a set of protocol references, e.g.,
@@ -1862,6 +1866,9 @@
protocols, protocolLocs,
protocolRAngleLoc, consumeLastToken);
+ if (Tok.is(tok::eof))
+return true; // Invalid type result.
+
// Compute the location of the last token.
if (consumeLastToken)
endLoc = PrevTokLocation;
Index: lib/Parse/ParseDecl.cpp
===
--- lib/Parse/ParseDecl.cpp
+++ lib/Parse/ParseDecl.cpp
@@ -5861,7 +5861,8 @@
// To handle this, we check to see if the token after the first
// identifier is a "," or ")". Only then do we parse it as an
// identifier list.
- && (NextToken().is(tok::comma) || NextToken().is(tok::r_paren));
+ && (!Tok.is(tok::eof) &&
+ (NextToken().is(tok::comma) || NextToken().is(tok::r_paren)));
}
/// ParseFunctionDeclaratorIdentifierList - While parsing a function declarator
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D23852: [SemaObjC] Fix crash while parsing type arguments and protocols
doug.gregor added a 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? https://reviews.llvm.org/D23852 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D23852: [SemaObjC] Fix crash while parsing type arguments and protocols
bruno created this revision.
bruno added a reviewer: doug.gregor.
bruno added subscribers: cfe-commits, manmanren.
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 '>'.
ParseTypeName() can parse a variety combination of names, making this
case complicated to handle by looking tokens ahead. Fix the issue by
doing temptive parsing in the remaining type arg list, and revert the
token state in case tok::eof is reached.
rdar://problem/25063557
https://reviews.llvm.org/D23852
Files:
lib/Parse/ParseObjc.cpp
test/SemaObjC/crash-on-type-args-protocols.m
Index: test/SemaObjC/crash-on-type-args-protocols.m
===
--- /dev/null
+++ test/SemaObjC/crash-on-type-args-protocols.m
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -DFIRST -fsyntax-only -verify %s
+// RUN: %clang_cc1 -DSECOND -fsyntax-only -verify %s
+// RUN: %clang_cc1 -DTHIRD -fsyntax-only -verify %s
+
+@protocol P;
+@interface NSObject
+@end
+@protocol X
+@end
+@interface X : NSObject
+@end
+
+@class A;
+
+#ifdef FIRST
+id F1(id<[P> v) { // expected-error {{expected a type}} // expected-error
{{use of undeclared identifier 'P'}} // expected-error {{use of undeclared
identifier 'v'}} // expected-error {{expected '>'}} // expected-error
{{expected ')'}} // expected-note {{to match this '('}}
+ return 0;
+}
+
+id F2(id v) { // expected-error {{unknown type name 'P'}} //
expected-error {{unexpected interface name 'X': expected expression}} //
expected-error {{use of undeclared identifier 'v'}} // expected-error
{{expected '>'}} // expected-error {{unexpected interface name 'X': expected
expression}} // expected-error {{use of undeclared identifier 'v'}} //
expected-note {{to match this '('}}
+ return 0;
+}
+#endif
+
+#ifdef SECOND
+id F3(id v) { // expected-error {{unknown type name 'P'}} //
expected-error {{expected expression}} // expected-error {{use of undeclared
identifier 'v'}} // expected-error {{expected '>'}} // expected-error
{{expected expression}} // expected-error {{use of undeclared identifier 'v'}}
// expected-note {{to match this '('}}
+ return 0;
+}
+#endif
+
+#ifdef THIRD
+id F4(id v { // expected-error {{unknown type name 'P'}} //
expected-error {{expected ')'}} // expected-error {{expected '>'}} //
expected-error {{expected ')'}} // expected-note {{to match this '('}} //
expected-note {{to match this '('}} // expected-note {{to match this '('}}
+ return 0;
+}
+#endif
+
+ // expected-error {{expected ')'}} // expected-error {{expected function body
after function declarator}}
Index: lib/Parse/ParseObjc.cpp
===
--- lib/Parse/ParseObjc.cpp
+++ lib/Parse/ParseObjc.cpp
@@ -1740,6 +1740,10 @@
}
}
+ // ParseTypeName() can advance the token stream up to tok::eof when there's
+ // an umatched token (e.g. "["). Restore the state in case this happens.
+ TentativeParsingAction TPA(*this);
+
// Continue parsing type-names.
do {
Token CurTypeTok = Tok;
@@ -1763,6 +1767,11 @@
}
} while (TryConsumeToken(tok::comma));
+ if (Tok.is(tok::eof))
+TPA.Revert();
+ else
+TPA.Commit();
+
// Diagnose the mix between type args and protocols.
if (foundProtocolId && foundValidTypeId)
Actions.DiagnoseTypeArgsAndProtocols(foundProtocolId, foundProtocolSrcLoc,
Index: test/SemaObjC/crash-on-type-args-protocols.m
===
--- /dev/null
+++ test/SemaObjC/crash-on-type-args-protocols.m
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -DFIRST -fsyntax-only -verify %s
+// RUN: %clang_cc1 -DSECOND -fsyntax-only -verify %s
+// RUN: %clang_cc1 -DTHIRD -fsyntax-only -verify %s
+
+@protocol P;
+@interface NSObject
+@end
+@protocol X
+@end
+@interface X : NSObject
+@end
+
+@class A;
+
+#ifdef FIRST
+id F1(id<[P> v) { // expected-error {{expected a type}} // expected-error {{use of undeclared identifier 'P'}} // expected-error {{use of undeclared identifier 'v'}} // expected-error {{expected '>'}} // expected-error {{expected ')'}} // expected-note {{to match this '('}}
+ return 0;
+}
+
+id F2(id v) { // expected-error {{unknown type name 'P'}} // expected-error {{unexpected interface name 'X': expected expression}} // expected-error {{use of undeclared identifier 'v'}} // expected-error {{expected '>'}} // expected-error {{unexpected interface name 'X': expected expression}} // expected-error {{use of undeclared identifier 'v'}} // expected-note {{to match this '('}}
+ return 0;
+}
+#endif
+
+#ifdef SECOND
+id F3(id v) { // expected-error {{unknown type name 'P'}} // expected-error {{expected expression}}
