This is an archive of the discontinued LLVM Phabricator instance.

Check i < FD->getNumParams() before querying
ClosedPublic

Authored by Violet on Mar 31 2019, 8:53 PM.

Details

Summary

As was already stated in a previous comment, the parameter isn't
necessarily referring to one of the DeclContext's parameter. We
should check the index is within the range to avoid out-of-boundary
access.

Event Timeline

Violet created this revision.Mar 31 2019, 8:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2019, 8:53 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Violet updated this revision to Diff 193041.Mar 31 2019, 9:31 PM

Wrong bug number

Violet updated this revision to Diff 193045.Mar 31 2019, 10:59 PM

a more straightforward testcase

lebedev.ri added inline comments.
clang/test/SemaCXX/PR41139.cpp
4

Add a comment what this test does?
('just checking that the crash does not happen again')

clang/test/SemaCXX/cxx1y-generic-lambdas.cpp
946

Does this have to be in that namespace?
Can you put it into a new namespace referencing this review?

Violet updated this revision to Diff 194152.Apr 8 2019, 8:37 AM

Address reviewer comment

Violet marked an inline comment as done.
lebedev.ri added inline comments.Apr 8 2019, 8:41 AM
clang/lib/Sema/SemaTemplateInstantiate.cpp
2895

Have you analyzed how we get here?
Maybe the caller code is faulty?

Violet marked 2 inline comments as done.Apr 8 2019, 9:02 AM
Violet added inline comments.
clang/lib/Sema/SemaTemplateInstantiate.cpp
2895

It's not the caller's fault. The cause is that a function pointer or a function typedef isn't a declcontext, so even if the getDeclContext() of its parameter returns a function, it has nothing to do with its own ParmVarDecl. That's why we check if the corresponding ParmVarDecl is the same one.

lebedev.ri resigned from this revision.Apr 9 2019, 5:05 AM

I won't be able to properly review this, sorry.
While i'm sure this fix silences the assert, i don't know what that means for the original caller.
Does it fail? Does it retry in some other scope? Should it be more picky about the original scope in the first place?

Violet added a comment.Apr 9 2019, 5:27 AM

I won't be able to properly review this, sorry.

Can you suggest someone with familiarity for this part of code? The reviewers I randomly added by git blame doesn't seem to be reviewing.

While i'm sure this fix silences the assert, i don't know what that means for the original caller.
Does it fail? Does it retry in some other scope? Should it be more picky about the original scope in the first place?

This small function is just to get the canonical ParmValDecl for a given one. If the check doesn't pass, it simply mean there is no canonical one (it's obvious for a function pointer), so we just use the passed one. There is nothing wrong here.

gribozavr accepted this revision.Apr 10 2019, 2:16 AM

This fix makes sense to me, however, Richard Smith @rsmith is the best person to review this patch.

This revision is now accepted and ready to land.Apr 10 2019, 2:16 AM

Thanks. Can you land it for me? I'm a newcommer without landing privilege.

I'd be happy to land it, but I do want @rsmith to take a look.

rsmith accepted this revision.Apr 10 2019, 12:23 PM
This revision was automatically updated to reflect the committed changes.