baloghadamsoftware requested changes to this revision. baloghadamsoftware added inline comments. This revision now requires changes to proceed. Herald added a subscriber: rnkovacs.
================ Comment at: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp:55 + // they can cause a not null-terminated string. In this case we store the + // string is being not null-terminated in the 'NullTerminationMap'. + // ---------------- Charusso wrote: > balazske wrote: > > The functions `gets`, `strcpy` return a null-terminated string according to > > standard, probably `sprintf` too, and the `fscanf` `%s` output is > > null-terminated too even if no length is specified (maybe it writes outside > > of the specified buffer but null terminated). > From where do you observe such information? I have not seen anything > `strcpy()`-specific in the standard. My observation clearly says that, if > there is not enough space for the data you *could* even write to overlapping > memory, or as people says, you could meet nasal demons. It is called > *undefined* behavior. From `cppreference.com`: `strcpy()` -- //Copies the null-terminated byte string pointed to by src, including the null terminator, to the character array whose first element is pointed to by dest.// `gets()` -- //https://en.cppreference.com/w/c/io/gets// `scanf`, `fscanf`, `sscan` format `%s` -- //If width specifier is used, matches up to width or until the first whitespace character, whichever appears first. Always stores a null character in addition to the characters matched (so the argument array must have room for at least width+1 characters)// Thus @balazske is completely right. All these functions add the null-terminator. Of course, if the string does not fit, it overwrites the memory after the buffer which is undefined behavior. But the null-terminator is definitely added. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp:100 +/// Returns the interesting region of \p V. +static const MemRegion *getRegion(SVal V) { + const MemRegion *MR = V.getAsRegion(); ---------------- Do we really need a separate utility function for that? ================ Comment at: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp:136 + llvm::raw_svector_ostream Out(Msg); + Out << '\'' << CallName << "' could write outside of '" << printPretty(Arg, C) + << '\''; ---------------- Create `SmallString`, use a stream to print into it, create another one in `printPretty`, convert it to `std::string` and then copy it again to the original `SmallString`? Why? ================ Comment at: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp:165 + if (EnableStr31cChecker) + createCallOverflowReport(Call, CallC, C); +} ---------------- `sprintf` not non-compliant generally. You should calculate the possible maximal length of the string and compare it to the size of the target region. ================ Comment at: clang/lib/StaticAnalyzer/Checkers/cert/StrChecker.cpp:180 + // FIXME: Handle multiple buffers. + if (FormatExpr->getString() != "%s") + return; ---------------- Checking for `%s` is not enough. `%2s` may also be a problem if the buffer is `1` character long. ================ Comment at: clang/test/Analysis/cert/str31-c.cpp:27 +namespace test_gets_good { +enum { BUFFERSIZE = 32 }; + ---------------- Why do we need a different buffer size here? And why `enum`? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70411/new/ https://reviews.llvm.org/D70411 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits