This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Fix null pointer dereference in readability-identifier-naming
ClosedPublic

Authored by markstegeman on May 24 2019, 9:36 AM.

Details

Summary

readability-identifier-naming causes a null pointer dereference when checking an identifier introduced by a structured binding whose right hand side is an undeclared identifier.

Running the check on a file that is just the following results in a crash:

auto [left] = right;

Diff Detail

Repository
rL LLVM

Event Timeline

markstegeman created this revision.May 24 2019, 9:36 AM
markstegeman created this object with visibility "markstegeman (Mark Stegeman)".
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2019, 9:36 AM

I haven't yet been able to figure out how to properly add a test for this bug, since the expected result is a failure to compile while the current test/clang-tidy/readability-identifier-naming.cpp must compile to pass.

Any suggestions?

markstegeman edited the summary of this revision. (Show Details)May 24 2019, 9:42 AM
markstegeman changed the visibility from "markstegeman (Mark Stegeman)" to "Public (No Login Required)".
markstegeman edited the summary of this revision. (Show Details)May 24 2019, 9:43 AM

I haven't yet been able to figure out how to properly add a test for this bug, since the expected result is a failure to compile while the current test/clang-tidy/readability-identifier-naming.cpp must compile to pass.

Any suggestions?

The way we usually handle this is to create a separate test file that exhibits the previously crashing code with a comment saying the test case used to crash. IIRC, you can use a CHECK line to check that you get the expected error.

Added a test.

hokein added inline comments.May 27 2019, 1:41 AM
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
804 ↗(On Diff #201401)

We are risky of dereferencing a nullptr, as the TypePtr can be null now.

madsravn added inline comments.
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
804 ↗(On Diff #201401)

Are we not checking for null in the line where TypePtr gets declared? Or am I misunderstanding?

aaron.ballman accepted this revision.May 27 2019, 7:34 AM

LGTM!

clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
804 ↗(On Diff #201401)

I don't see a null pointer dereference here for TypePtr; I think the code is fine as-is.

This revision is now accepted and ready to land.May 27 2019, 7:34 AM

Great. Can someone commit this for me?

hokein accepted this revision.May 28 2019, 4:05 AM
hokein added inline comments.
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
804 ↗(On Diff #201401)

Ah, I missed the if statement, it is guarded by the if statement.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 28 2019, 4:51 AM