This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Fix a crash in bugprone-not-null-terminated-result check when `__STDC_WANT_LIB_EXT1__` is not a literal.
ClosedPublic

Authored by ArcsinX on Aug 7 2020, 7:05 AM.

Details

Summary

If __STDC_WANT_LIB_EXT1__ is not a literal (e.g. #define __STDC_WANT_LIB_EXT1__ ((unsigned)1)) bugprone-not-null-terminated-result check crashes.
Stack dump:

#0 0x0000000002185e6a llvm::sys::PrintStackTrace(llvm::raw_ostream&) (/llvm-project/build/bin/clang-tidy+0x2185e6a)
#1 0x0000000002183e8c llvm::sys::RunSignalHandlers() (/llvm-project/build/bin/clang-tidy+0x2183e8c)
#2 0x0000000002183ff3 SignalHandler(int) (/llvm-project/build/bin/clang-tidy+0x2183ff3)
#3 0x00007f08d91b1390 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x11390)
#4 0x00000000021338bb llvm::StringRef::getAsInteger(unsigned int, llvm::APInt&) const (/llvm-project/build/bin/clang-tidy+0x21338bb)
#5 0x000000000052051c clang::tidy::bugprone::NotNullTerminatedResultCheck::check(clang::ast_matchers::MatchFinder::MatchResult const&) (/llvm-project/build/bin/clang-tidy+0x52051c)

Diff Detail

Event Timeline

ArcsinX created this revision.Aug 7 2020, 7:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2020, 7:05 AM
ArcsinX requested review of this revision.Aug 7 2020, 7:05 AM
ArcsinX added a project: Restricted Project.
hokein accepted this revision.Aug 7 2020, 7:15 AM
hokein added inline comments.
clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
806

let's add the getLiteralData check here -- if (T.isLiteral() && T.getLiteralData())

clang-tools-extra/test/clang-tidy/checkers/bugprone-not-null-terminated-result-stdc-want-lib-ext1-not-a-literal.c
3

I think you probably need a // UNSUPPORTED: system-windows, as the bugprone-not-null-terminated-result-strlen.c does.

16

nit: add EOF.

This revision is now accepted and ready to land.Aug 7 2020, 7:15 AM
ArcsinX added inline comments.Aug 7 2020, 7:34 AM
clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
806

Could we also add clangd test here for that case (when T.isLiteral() == true and T.getLiteralData() == nullptr)?

ArcsinX added inline comments.Aug 7 2020, 7:42 AM
clang-tools-extra/test/clang-tidy/checkers/bugprone-not-null-terminated-result-stdc-want-lib-ext1-not-a-literal.c
3

Seems we don't need this.
Problem with bugprone-not-null-terminated-result-strlen.c test on Windows related with strncmp, according with comment.

// FIXME: Something wrong with the APInt un/signed conversion on Windows:
// in 'strncmp(str6, "string", 7);' it tries to inject '4294967302' as length.

// UNSUPPORTED: system-windows

E.g. bugprone-not-null-terminated-result-memcpy-safe also uses strlen

hokein added inline comments.Aug 9 2020, 11:54 PM
clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
806

I think it is fine to just change it here without a clangd test.

clang-tools-extra/test/clang-tidy/checkers/bugprone-not-null-terminated-result-stdc-want-lib-ext1-not-a-literal.c
3

I see, good catch.

ArcsinX updated this revision to Diff 284296.Aug 10 2020, 3:09 AM
  • Rebase
  • Add T.getLiteralData() check
  • Newline