This is an archive of the discontinued LLVM Phabricator instance.

Differential D10634

Loss of Sign Checker
Needs ReviewPublic

Authored by soumitra on Jun 22 2015, 10:21 PM.

Details

Reviewers
danielmarjamaki
Summary

Diagnose the case when a negative value is assigned to an unsigned variable.

Unfortunately, there are no existing diagnostics for such assignments
that cause the loss of the sign, potentially resulting in nasty silent runtime failures.

Diff Detail

Event Timeline

soumitra updated this revision to Diff 28096.Jun 22 2015, 10:21 PM
soumitra retitled this revision from to Loss of Sign Checker.
soumitra updated this object.
soumitra edited the test plan for this revision. (Show Details)
soumitra added a subscriber: Unknown Object (MLST).
danielmarjamaki added a subscriber: danielmarjamaki.Jun 23 2015, 1:57 AM

I recommend a more specific name than LossOfSignChecker unless you want it to have more warnings later. Right now, a name such as LossOfSignInAssignmentChecker.cpp would be better imho.

Sure there can be issues with loss of sign. If we can warn about mistakes that is awesome!

However personally I think it seems noisy to warn about:

unsigned char uc1 = -1;

As I see it, it is quite clear that the developer wants to assign -1 to uc1 and that a cast is expected. On a normal machine the uc1 would get the value 255 and this is likely expected.

danielmarjamaki added a comment.Jun 23 2015, 2:06 AM

Have you tried this checker on some real projects to see if the results are good?

soumitra added a comment.Jun 23 2015, 3:03 AM
In D10634#192481, @danielmarjamaki wrote:

I recommend a more specific name than LossOfSignChecker unless you want it to have more warnings later. Right now, a name such as LossOfSignInAssignmentChecker.cpp would be better imho.

I don't have immediate plans to add more warnings on this, but there definitely are many more cases where we can lose the sign and potentially cause runtime failures.

That said, I can rename it as you suggest since I do not have immediate plans to address more cases.

Sure there can be issues with loss of sign. If we can warn about mistakes that is awesome!

However personally I think it seems noisy to warn about:

unsigned char uc1 = -1;

As I see it, it is quite clear that the developer wants to assign -1 to uc1 and that a cast is expected. On a normal machine the uc1 would get the value 255 and this is likely expected.

Sure, but wouldn't it be better to expect an explicit cast if the developer really meant what he wrote?

In D10634#192482, @danielmarjamaki wrote:

Have you tried this checker on some real projects to see if the results are good?

No, not really.

However, I hit this issue when moving a large software from x86 to AArch64, where unfortunately, the 'sign' of a plain char changes, causing us to lose days of effort in narrowing down the issue.

For example, the following representative program that works perfectly on x86 would fail silently on AArch64:

 1  #include <stdio.h>
 2
 3  const char data[] = { -1 };
 4
 5  int main() {
 6     int value = data[0];
 7     if (value != -1) {
 8        printf("failed! value = %d\n", value);
 9        return 1;
10     }
11     return 0;
12  }

Do you think narrowing down the checker to the above singular case would be more appropriate? I would still think that while the 'unsigned char' case you point out may be a grey area, an "integer" assignment with the wrong sign would certainly not be a good programming practice.

danielmarjamaki added a comment.Jun 23 2015, 4:18 AM
In D10634#192520, @soumitra wrote:
In D10634#192481, @danielmarjamaki wrote:

I recommend a more specific name than LossOfSignChecker unless you want it to have more warnings later. Right now, a name such as LossOfSignInAssignmentChecker.cpp would be better imho.

I don't have immediate plans to add more warnings on this, but there definitely are many more cases where we can lose the sign and potentially cause runtime failures.

That said, I can rename it as you suggest since I do not have immediate plans to address more cases.

ok. I guess more warnings can be added later. So a generic name for the class is not a bad idea.

I'd recommend that you change the id in the td like:

def LossOfSignChecker : Checker<"LossOfSignAssign">,

And then also rename the testfile to LossOfSignAssign.c.

Sure there can be issues with loss of sign. If we can warn about mistakes that is awesome!

However personally I think it seems noisy to warn about:

unsigned char uc1 = -1;

As I see it, it is quite clear that the developer wants to assign -1 to uc1 and that a cast is expected. On a normal machine the uc1 would get the value 255 and this is likely expected.

Sure, but wouldn't it be better to expect an explicit cast if the developer really meant what he wrote?

Well.. I am not sure what the Clang users thinks about this. Maybe they would accept changing their code. Personally I'd prefer:

unsigned char = ~0U;

It would be nice to see some actual warnings and see if there are possible mistakes.

In D10634#192482, @danielmarjamaki wrote:

Have you tried this checker on some real projects to see if the results are good?

No, not really.

ok. I can run it on some projects.. I have a testscript I run.

However, I hit this issue when moving a large software from x86 to AArch64, where unfortunately, the 'sign' of a plain char changes, causing us to lose days of effort in narrowing down the issue.

For example, the following representative program that works perfectly on x86 would fail silently on AArch64:

 1  #include <stdio.h>
 2
 3  const char data[] = { -1 };
 4
 5  int main() {
 6     int value = data[0];
 7     if (value != -1) {
 8        printf("failed! value = %d\n", value);
 9        return 1;
10     }
11     return 0;
12  }

Nice example.

Do you think narrowing down the checker to the above singular case would be more appropriate? I would still think that while the 'unsigned char' case you point out may be a grey area, an "integer" assignment with the wrong sign would certainly not be a good programming practice.

I would definitely feel better about this then.

But I will try your current patch on some open source projects.. and then we will have some actual results to look at.

Feel free to run your patch on some projects too.

danielmarjamaki added a comment.Jun 23 2015, 6:25 AM

hmm.. I get stack dumps.

Please review the code around line 151 in LossOfSignChecker.cpp

To reproduce a dump on a linux box, you can use such commands:
$ wget ftp://ftp.sunet.se/pub/Linux/distributions/Debian/debian/pool/main/a/a52dec/a52dec_0.7.4.orig.tar.gz
$ tar xzvf a52dec_0.7.4.orig.tar.gz
$ cd a52dec-0.7.4
$ ./configure
$ ~/llvm/tools/clang/tools/scan-build/scan-build --use-analyzer=/home/$USER/llvm/build/Debug+Asserts/bin/clang -enable-checker alpha.core.LossOfSign make

Stack dump:

#0 0x46e3574 llvm::sys::PrintStackTrace(llvm::raw_ostream&) /home/danielm/llvm/lib/Support/Unix/Signals.inc:437:0
#1 0x46e3889 PrintStackTraceSignalHandler(void*) /home/danielm/llvm/lib/Support/Unix/Signals.inc:495:0
#2 0x46e238a SignalHandler(int) /home/danielm/llvm/lib/Support/Unix/Signals.inc:210:0
#3 0x2ab6b09cc340 restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x10340)
#4 0x16c8922 clang::Expr::getType() const /home/danielm/llvm/tools/clang/lib/Serialization/../../include/clang/AST/Expr.h:125:0
#5 0x2257417 (anonymous namespace)::LossOfSignChecker::checkASTDecl(clang::VarDecl const*, clang::ento::AnalysisManager&, clang::ento::BugReporter&) const /home/danielm/llvm/tools/clang/lib/StaticAnalyzer/Checkers/LossOfSignChecker.cpp:151:0
#6 0x2257a9a void clang::ento::check::ASTDecl<clang::VarDecl>::_checkDecl<(anonymous namespace)::LossOfSignChecker>(void*, clang::Decl const*, clang::ento::AnalysisManager&, clang::ento::BugReporter&) /home/danielm/llvm/tools/clang/lib/StaticAnalyzer/Checkers/../../../in
clude/clang/StaticAnalyzer/Core/Checker.h:34:0
#7 0x23b57d7 clang::ento::CheckerFn<void (clang::Decl const*, clang::ento::AnalysisManager&, clang::ento::BugReporter&)>::operator()(clang::Decl const*, clang::ento::AnalysisManager&, clang::ento::BugReporter&) const /home/danielm/llvm/tools/clang/lib/StaticAnalyzer/Core
/../../../include/clang/StaticAnalyzer/Core/CheckerManager.h:59:0
#8 0x23b1d84 clang::ento::CheckerManager::runCheckersOnASTDecl(clang::Decl const*, clang::ento::AnalysisManager&, clang::ento::BugReporter&) /home/danielm/llvm/tools/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp:76:0
#9 0x216369f (anonymous namespace)::AnalysisConsumer::VisitDecl(clang::Decl*) /home/danielm/llvm/tools/clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:342:0
#10 0x2191929 clang::DataRecursiveASTVisitor<(anonymous namespace)::AnalysisConsumer>::WalkUpFromDecl(clang::Decl*) /home/danielm/llvm/tools/clang/lib/StaticAnalyzer/Frontend/../../../include/clang/AST/DataRecursiveASTVisitor.h:400:0
#11 0x219e05d clang::DataRecursiveASTVisitor<(anonymous namespace)::AnalysisConsumer>::WalkUpFromNamedDecl(clang::NamedDecl*) /home/danielm/llvm/build/tools/clang/lib/StaticAnalyzer/Frontend/../../../include/clang/AST/DeclNodes.inc:89:0
#12 0x21a0081 clang::DataRecursiveASTVisitor<(anonymous namespace)::AnalysisConsumer>::WalkUpFromValueDecl(clang::ValueDecl*) /home/danielm/llvm/build/tools/clang/lib/StaticAnalyzer/Frontend/../../../include/clang/AST/DeclNodes.inc:323:0
#13 0x219fe29 clang::DataRecursiveASTVisitor<(anonymous namespace)::AnalysisConsumer>::WalkUpFromDeclaratorDecl(clang::DeclaratorDecl*) /home/danielm/llvm/build/tools/clang/lib/StaticAnalyzer/Frontend/../../../include/clang/AST/DeclNodes.inc:327:0
#14 0x2185585 clang::DataRecursiveASTVisitor<(anonymous namespace)::AnalysisConsumer>::WalkUpFromVarDecl(clang::VarDecl*) /home/danielm/llvm/build/tools/clang/lib/StaticAnalyzer/Frontend/../../../include/clang/AST/DeclNodes.inc:397:0
#15 0x2171612 clang::DataRecursiveASTVisitor<(anonymous namespace)::AnalysisConsumer>::TraverseVarDecl(clang::VarDecl*) /home/danielm/llvm/tools/clang/lib/StaticAnalyzer/Frontend/../../../include/clang/AST/DataRecursiveASTVisitor.h:1787:0
#16 0x2167947 clang::DataRecursiveASTVisitor<(anonymous namespace)::AnalysisConsumer>::TraverseDecl(clang::Decl*) /home/danielm/llvm/build/tools/clang/lib/StaticAnalyzer/Frontend/../../../include/clang/AST/DeclNodes.inc:397:0
#17 0x2164065 (anonymous namespace)::AnalysisConsumer::HandleTranslationUnit(clang::ASTContext&) /home/danielm/llvm/tools/clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:537:0
#18 0x19fa91c clang::ParseAST(clang::Sema&, bool, bool) /home/danielm/llvm/tools/clang/lib/Parse/ParseAST.cpp:153:0
#19 0x13e5ebe clang::ASTFrontendAction::ExecuteAction() /home/danielm/llvm/tools/clang/lib/Frontend/FrontendAction.cpp:538:0
#20 0x13e597b clang::FrontendAction::Execute() /home/danielm/llvm/tools/clang/lib/Frontend/FrontendAction.cpp:443:0
#21 0x13a763e clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) /home/danielm/llvm/tools/clang/lib/Frontend/CompilerInstance.cpp:819:0
#22 0x136733f clang::ExecuteCompilerInvocation(clang::CompilerInstance*) /home/danielm/llvm/tools/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:222:0
#23 0x1351746 cc1_main(llvm::ArrayRef<char const*>, char const*, void*) /home/danielm/llvm/tools/clang/tools/driver/cc1_main.cpp:112:0
#24 0x1360de3 ExecuteCC1Tool(llvm::ArrayRef<char const*>, llvm::StringRef) /home/danielm/llvm/tools/clang/tools/driver/driver.cpp:358:0
#25 0x13613f5 main /home/danielm/llvm/tools/clang/tools/driver/driver.cpp:404:0
#26 0x2ab6b1848ec5 libc_start_main /build/buildd/eglibc-2.19/csu/libc-start.c:321:0
#27 0x134fbe9 _start (/home/danielm/llvm/build/Debug+Asserts/bin-sign/clang+0x134fbe9)
Stack dump:
0. Program arguments: /home/danielm/llvm/build/Debug+Asserts/bin-sign/clang -cc1 -triple x86_64-unknown-linux-gnu -analyze -disable-free -main-file-name imdct.c -analyzer-store=region -analyzer-opt-analyze-nested-blocks -analyzer-eagerly-assume -analyzer-checker=cor
e -analyzer-checker=unix -analyzer-checker=deadcode -analyzer-checker=security.insecureAPI.UncheckedReturn -analyzer-checker=security.insecureAPI.getpw -analyzer-checker=security.insecureAPI.gets -analyzer-checker=security.insecureAPI.mktemp -analyzer-checker=security.in
secureAPI.mkstemp -analyzer-checker=security.insecureAPI.vfork -analyzer-output plist -w -mrelocation-model static -mthread-model posix -fmath-errno -masm-verbose -mconstructor-aliases -munwind-tables -fuse-init-array -target-cpu x86-64 -target-linker-version 2.24 -momit
-leaf-frame-pointer -dwarf-column-info -resource-dir /home/danielm/llvm/build/Debug+Asserts/bin-sign/../lib/clang/3.7.0 -D HAVE_CONFIG_H -I . -I . -I ../include -I ../include -I ../include -internal-isystem /usr/local/include -internal-isystem /home/danielm/llvm/build/De
bug+Asserts/bin-sign/../lib/clang/3.7.0/include -internal-externc-isystem /usr/include/x86_64-linux-gnu -internal-externc-isystem /include -internal-externc-isystem /usr/include -O3 -fdebug-compilation-dir /home/danielm/daca-clang-sign/a52dec-0.7.4/liba52 -ferror-limit 1
9 -fmessage-length 0 -mstackrealign -fobjc-runtime=gcc -fdiagnostics-show-option -vectorize-loops -vectorize-slp -analyzer-checker alpha.core.LossOfSign -analyzer-output=plist -o /home/danielm/daca-clang-signplist-output/2015-06-23-144232-17041-1/report-FjusPt.plist -x c imdct.c

danielmarjamaki added a comment.Jun 23 2015, 6:45 AM

I also get such stack dumps when I run 'make check-all' .. does everything work fine for you when you run all the tests?

soumitra updated this revision to Diff 28648.Jun 29 2015, 1:32 AM
soumitra edited the test plan for this revision. (Show Details)

I had missed a check for initializer on a variable in my previous revision, resulting in a stack dump. Fixed.
Renamed the id and the test file as per Daniel's suggestion to reflect assignment.
Validated with make check-all, which I had missed earlier.

soumitra edited the test plan for this revision. (Show Details)Jun 29 2015, 1:33 AM
soumitra edited the test plan for this revision. (Show Details)
danielmarjamaki added a comment.Jun 29 2015, 3:35 AM

I get a compiler error now. Either I do something wrong, or you did not update the Clang source code. Let me know if I do something wrong..

/home/danielm/llvm/tools/clang/lib/StaticAnalyzer/Checkers/LossOfSignChecker.cpp:63:61: error: no matching function for call to ‘clang::ento::CheckerContext::emitReport(clang::ento::BugReport*)’
     C.emitReport(new BugReport(*BT, BT->getDescription(), N));
soumitra updated this revision to Diff 28751.Jun 29 2015, 9:55 PM
soumitra edited the test plan for this revision. (Show Details)

Missed updating my working copy prior to creating the patch.
Updated, Rebuilt, and tested using check-all.

danielmarjamaki added a comment.Jul 1 2015, 2:07 AM

While the above is trying to assign a signed value to an unsigned variable, I am still not able to correctly establish if the value is negative for sure. The following is a reduced piece of code that still throws the diagnostic, but I am not sure what exactly am I missing in my check to avoid this. Note that the reported line of code seems to be unreachable:

Thank you!

The code is not unreachable as far as I see.. the function yylex() can be called from another source file for instance.

The warning is correct if we assume that the global variables have their default initialisation values. That is probably not the case in the real code.

For now I think the yylex warning can be ok. At least in the reduced code, we can't prove that rhs is never negative. If we want to fix these warnings later, I think that it must be solved in the Clang infrastructure. Then I would suggest we do that in a separate patch.

I will also run this checker on some projects..

lib/StaticAnalyzer/Checkers/LossOfSignChecker.cpp
11

I don't know see the connection against "ExprEngine".. so this comment looks confusing to me.. how about removing "a builtin check in ExprEngine"?

40

Please end all comments with a period ".".

41

the first character in function names should be lower case.

72

Variables should start with a uppercase letter. And abbreviations are preferred.

86

this should probably be a early return.

96

I assume you did not run clang-format on this?

clang-format -i LossOfSignChecker.cpp
105

as far as I see this return is redundant

109

as far as I see this return is redundant

129

Please use { on this outer if also. I'd say the { on the inner if is optional.

159

early return

soumitra updated this revision to Diff 28934.Jul 1 2015, 10:52 PM

Incorporated review comments by Daniel.
Updated revisions, rebuilt, retested using check-all.

danielmarjamaki added a comment.Jul 2 2015, 5:43 AM

Here are some warnings.. could you please look at some of them...

There are a few cases where RHS is a constant integer literal.. do you think there could be a bug in some of them?

ftp://ftp.sunet.se/pub/Linux/distributions/Debian/debian/pool/main/a/abcm2ps/abcm2ps_7.8.9.orig.tar.gz
clang-tidy abcm2ps-7.8.9/deco.c
deco.c:788:9: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

ps_x = -1;

ftp://ftp.sunet.se/pub/Linux/distributions/Debian/debian/pool/main/a/adolc/adolc_2.5.2.orig.tar.gz
clang-tidy ADOL-C-2.5.2/ADOL-C/src/taping.c
taping.c:914:12: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

number = lastTayP1 - ADOLC_CURRENT_TAPE_INFOS.tayBuffer;

ftp://ftp.sunet.se/pub/Linux/distributions/Debian/debian/pool/main/a/alsa-lib/alsa-lib_1.0.28.orig.tar.bz2
clang-tidy alsa-lib-1.0.28/src/pcm/pcm_dsnoop.c
pcm_dsnoop.c:60:20: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

snd_pcm_uframes_t ptr1 = -2LL /* invalid value */, ptr2;

ftp://ftp.sunet.se/pub/Linux/distributions/Debian/debian/pool/main/a/anubis/anubis_4.1.1+dfsg1.orig.tar.gz
clang-tidy anubis-4.1.1+dfsg1/src/rcfile-lex.c
rcfile-lex.c:1482:3: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

rc_yy_size_t new_size = (rc_yy_n_chars) + number_to_move + ((rc_yy_n_chars) >> 1);

ftp://ftp.sunet.se/pub/Linux/distributions/Debian/debian/pool/main/a/atheme-services/atheme-services_7.0.7.orig.tar.bz2
clang-tidy atheme-services-7.0.7/modules/groupserv/main/database.c
database.c:9:21: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]
static unsigned int loading_gdbv = -1;

clang-tidy atheme-services-7.0.7/libathemecore/reslib.c
reslib.c:1093:13: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

n = labellen(cp - 1); /* XXX */

clang-tidy atheme-services-7.0.7/libmowgli-2/src/libmowgli/dns/dns_evloop_reslib.c
dns_evloop_reslib.c:1011:8: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

n = labellen(cp - 1);   /* XXX */

clang-tidy avfs-1.0.1/src/sysdeps.c
sysdeps.c:251:5: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

static avuid_t myuid = -1;
^

sysdeps.c:251:5: note: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior

static avuid_t myuid = -1;
^

sysdeps.c:252:5: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

static avuid_t mygid = -1;
^

sysdeps.c:252:5: note: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior

static avuid_t mygid = -1;
^

sysdeps.c:251:5: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

static avuid_t myuid = -1;
^

sysdeps.c:251:5: note: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior

static avuid_t myuid = -1;
^

sysdeps.c:252:5: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

static avuid_t mygid = -1;
^

sysdeps.c:252:5: note: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior

static avuid_t mygid = -1;
^

clang-tidy avfs-1.0.1/zlib/deflate.c
deflate.c:1033:11: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

n = read_buf(s->strm, s->window + s->strstart + s->lookahead, more);
lib/StaticAnalyzer/Checkers/Checkers.td
91

Please put this in alphabetical order somewhat.

lib/StaticAnalyzer/Checkers/LossOfSignChecker.cpp
72

I looked in the llvm coding style guide. It does not say that abbreviations are preferred as far as I see. Ignore my comment about that.

88

Comments should start with uppercase

93

you missed this variable, use uppercase for the first character.

soumitra updated this revision to Diff 29059.Jul 5 2015, 8:26 PM

Incorporated further review comments by Daniel.
Working copy updated, built, and tested using check-all.

Investigating individual diagnostics pointed out by Daniel.

soumitra marked 14 inline comments as done.Jul 5 2015, 8:29 PM
soumitra added a comment.Jul 6 2015, 12:56 AM
In D10634#198447, @danielmarjamaki wrote:

ftp://ftp.sunet.se/pub/Linux/distributions/Debian/debian/pool/main/a/abcm2ps/abcm2ps_7.8.9.orig.tar.gz
clang-tidy abcm2ps-7.8.9/deco.c
deco.c:788:9: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

ps_x = -1;

Only a temporary assignment, before being assigned to "signed char" ps_func, and hence safe.

ftp://ftp.sunet.se/pub/Linux/distributions/Debian/debian/pool/main/a/adolc/adolc_2.5.2.orig.tar.gz
clang-tidy ADOL-C-2.5.2/ADOL-C/src/taping.c
taping.c:914:12: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

number = lastTayP1 - ADOLC_CURRENT_TAPE_INFOS.tayBuffer;

This one indeed looks suspicious, since "number" is subsequently used to compute "chunks", which is being used as a loop control guard initialized with 0.

ftp://ftp.sunet.se/pub/Linux/distributions/Debian/debian/pool/main/a/alsa-lib/alsa-lib_1.0.28.orig.tar.bz2
clang-tidy alsa-lib-1.0.28/src/pcm/pcm_dsnoop.c
pcm_dsnoop.c:60:20: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

snd_pcm_uframes_t ptr1 = -2LL /* invalid value */, ptr2;

This can be potentially problematic due to the subsequent equality check with "ptr2".

ftp://ftp.sunet.se/pub/Linux/distributions/Debian/debian/pool/main/a/anubis/anubis_4.1.1+dfsg1.orig.tar.gz
clang-tidy anubis-4.1.1+dfsg1/src/rcfile-lex.c
rcfile-lex.c:1482:3: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

rc_yy_size_t new_size = (rc_yy_n_chars) + number_to_move + ((rc_yy_n_chars) >> 1);

Here, "new_size" is the target size of a realloc, which can go haywire if the assigned value was indeed negative.

ftp://ftp.sunet.se/pub/Linux/distributions/Debian/debian/pool/main/a/atheme-services/atheme-services_7.0.7.orig.tar.bz2
clang-tidy atheme-services-7.0.7/modules/groupserv/main/database.c
database.c:9:21: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]
static unsigned int loading_gdbv = -1;

This is probably problematic, since a subsequent check "loading_gdbv >= 4" (in function "db_h_grp") will always be true, unless modified by an intervening assignment.

clang-tidy atheme-services-7.0.7/libathemecore/reslib.c
reslib.c:1093:13: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

n = labellen(cp - 1); /* XXX */

This looks safe to me.

clang-tidy atheme-services-7.0.7/libmowgli-2/src/libmowgli/dns/dns_evloop_reslib.c
dns_evloop_reslib.c:1011:8: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

n = labellen(cp - 1);   /* XXX */

This looks safe to me.

clang-tidy avfs-1.0.1/src/sysdeps.c

Looks like you missed mentioning the source.
I am assuming http://sourceforge.net/projects/avf/

sysdeps.c:251:5: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

static avuid_t myuid = -1;
^

My understanding was that the above should cause the subsequent comparison with '-1' to fail, but it doesn't. Promotion?
It does fail in case of 'char' though.

sysdeps.c:251:5: note: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior

static avuid_t myuid = -1;
^

I thought I am only emitting a warning. Where is this "note" originating from? Can this duplicate diagnostic be suppressed?

sysdeps.c:252:5: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

static avuid_t mygid = -1;
^

sysdeps.c:252:5: note: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior

static avuid_t mygid = -1;
^

Same as above.

clang-tidy avfs-1.0.1/zlib/deflate.c
deflate.c:1033:11: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

n = read_buf(s->strm, s->window + s->strstart + s->lookahead, more);

Here, "n" is used to adjust the "lookahead", which is then compared with "MIN_MATCH". On assigning a negative value (in effect a huge positive value), the above comparison may fail.

soumitra added a reviewer: danielmarjamaki.Jul 6 2015, 12:57 AM
danielmarjamaki edited edge metadata.EditedJul 6 2015, 6:06 AM

Here is a bigger log with more warnings.
https://drive.google.com/file/d/0BykPmWrCOxt2aGtRNTY4eXQ1OU0/view

there are some cases where I don't think there should be a warning.. for instance some compound assignments:

args.c:990:9: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

tmp /= 10;
    ^

read.c:4940:13: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

value >>= 7;
      ^

I am not an expert but it seems strange to me that clang seems to think that 10 and 7 are negative. Can you investigate this?

soumitra added a comment.Jul 7 2015, 8:03 PM
In D10634#199554, @danielmarjamaki wrote:

Here is a bigger log with more warnings.
https://drive.google.com/file/d/0BykPmWrCOxt2aGtRNTY4eXQ1OU0/view

there are some cases where I don't think there should be a warning.. for instance some compound assignments:

args.c:990:9: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

tmp /= 10;
    ^

read.c:4940:13: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

value >>= 7;
      ^

I am not an expert but it seems strange to me that clang seems to think that 10 and 7 are negative. Can you investigate this?

Here's a short test case that shows the problem:

1  void safe_mul(unsigned long x) {
2    unsigned long multiplier = 1000000000000000000; 
3    while (x > 0) {
4      multiplier /= 10;
5    }
6  }

I may be wrong here, but I believe that the warning is due to an overflow during constant folding of the initializer, which causes the RHS to be treated as negative.

danielmarjamaki added a comment.Jul 9 2015, 1:29 AM

Here's a short test case that shows the problem:

Thanks!

I believe a fix for this problem would be a good thing. So that when this patch is added we don't get these false positives.

It should be a separate patch.

aadg added a subscriber: aadg.Jul 9 2015, 3:05 PM
soumitra added a comment.Jul 9 2015, 11:18 PM
In D10634#201632, @danielmarjamaki wrote:

Here's a short test case that shows the problem:

Thanks!

I believe a fix for this problem would be a good thing. So that when this patch is added we don't get these false positives.

It should be a separate patch.

Sure, let me try to take a shot at it.

zaks.anna added a subscriber: zaks.anna.Jul 28 2015, 1:45 PM

I have a couple of high level comments.

Why do you have checkASTDecl (which is a syntactic check) in addition to the checkBind (which is a path-sensitive check)? Does checkASTDecl find more issues? can those be found using a path sensitive callback?

I am leaning toward allowing explicit assignments to "-1", like in this case: "unsigned int j = -1". The tool is much more usable if there are few false positives.

Some of the results look like false positives. For example, this one from https://drive.google.com/file/d/0BykPmWrCOxt2aGtRNTY4eXQ1OU0/view:
fitscore.c:1077:21: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

namelen -= 9;  /* deleted the string 'HIERARCH ' */

It's hard to know what they are if we are only looking at the line where the issue is reported without seeing the pull path. Do we know that the value can be negative on that path?

test/Analysis/LossOfSignAssign.c
20

What happens if the return type is unsigned?

soumitra added a comment.Aug 3 2015, 9:49 PM
In D10634#213835, @zaks.anna wrote:

Why do you have checkASTDecl (which is a syntactic check) in addition to the checkBind (which is a path-sensitive check)? Does checkASTDecl find more issues? can those be found using a path sensitive callback?

Yes, it seems that the global assignments can only be caught using the ASTDecl checker (probably because they do not have an associated "path"?) Please do let me know in case I am wrong in my understanding.

I am leaning toward allowing explicit assignments to "-1", like in this case: "unsigned int j = -1". The tool is much more usable if there are few false positives.

This is exactly what I started off with, albeit with a plain 'char' instead of 'unsigned int'. We were hitting a runtime issue while porting a large piece of software to AArch64 since the "signedness" of plain char changes across x86 and AArch64, and a negative value was used as a initializer.

Some of the results look like false positives. For example, this one from https://drive.google.com/file/d/0BykPmWrCOxt2aGtRNTY4eXQ1OU0/view:
fitscore.c:1077:21: warning: assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior [clang-analyzer-alpha.core.LossOfSignAssign]

namelen -= 9;  /* deleted the string 'HIERARCH ' */

I don't see the above in the link you posted. Can you please point me to the source so that I can look into this?
From similar cases that Daniel pointed out earlier, it seems that the variable would possibly take a negative value (e.g. if namelen was initialized to 0) and hence the checker flags it off.

It's hard to know what they are if we are only looking at the line where the issue is reported without seeing the pull path. Do we know that the value can be negative on that path?

What I am trying to do is check if the RHS of an unsigned initialization is negative and if so, flag off that assignment.
Do you see anything incorrect in my approach, or a better way to achieve the same?

test/Analysis/LossOfSignAssign.c
20

Do you think this would be something good to catch? I somehow assumed that this might be more noisy and less useful than the assignment cases?

danielmarjamaki added a comment.Aug 3 2015, 10:56 PM
In D10634#217433, @soumitra wrote:
In D10634#213835, @zaks.anna wrote:

I am leaning toward allowing explicit assignments to "-1", like in this case: "unsigned int j = -1". The tool is much more usable if there are few false positives.

This is exactly what I started off with, albeit with a plain 'char' instead of 'unsigned int'. We were hitting a runtime issue while porting a large piece of software to AArch64 since the "signedness" of plain char changes across x86 and AArch64, and a negative value was used as a initializer.

I am also still skeptic about this. Ideally there should only be warnings when there is a mistake.

In your example code you showed previously there were portability problems because the signedness changed. A warning for that is ok in my opinion.

If the code says 'unsigned int j = -1;' then there is no such portability problem.

soumitra added a comment.Aug 3 2015, 11:03 PM
In D10634#217438, @danielmarjamaki wrote:
In D10634#217433, @soumitra wrote:
In D10634#213835, @zaks.anna wrote:

I am leaning toward allowing explicit assignments to "-1", like in this case: "unsigned int j = -1". The tool is much more usable if there are few false positives.

This is exactly what I started off with, albeit with a plain 'char' instead of 'unsigned int'. We were hitting a runtime issue while porting a large piece of software to AArch64 since the "signedness" of plain char changes across x86 and AArch64, and a negative value was used as a initializer.

I am also still skeptic about this. Ideally there should only be warnings when there is a mistake.

In your example code you showed previously there were portability problems because the signedness changed. A warning for that is ok in my opinion.

If the code says 'unsigned int j = -1;' then there is no such portability problem.

Ah! Now I get it!!

So, the case I have is assignment of a negative value to a 'plain char'. Is there a way I can check for a 'plain char' in the checker as against a generic 'unsigned' integral variable?

danielmarjamaki added a comment.Aug 3 2015, 11:20 PM
In D10634#217441, @soumitra wrote:
In D10634#217438, @danielmarjamaki wrote:
In D10634#217433, @soumitra wrote:
In D10634#213835, @zaks.anna wrote:

I am leaning toward allowing explicit assignments to "-1", like in this case: "unsigned int j = -1". The tool is much more usable if there are few false positives.

This is exactly what I started off with, albeit with a plain 'char' instead of 'unsigned int'. We were hitting a runtime issue while porting a large piece of software to AArch64 since the "signedness" of plain char changes across x86 and AArch64, and a negative value was used as a initializer.

I am also still skeptic about this. Ideally there should only be warnings when there is a mistake.

In your example code you showed previously there were portability problems because the signedness changed. A warning for that is ok in my opinion.

If the code says 'unsigned int j = -1;' then there is no such portability problem.

Ah! Now I get it!!

So, the case I have is assignment of a negative value to a 'plain char'. Is there a way I can check for a 'plain char' in the checker as against a generic 'unsigned' integral variable?

Great. I don't know how off the top of my head. But it should be possible.

Revision Contents

PathSize
lib/
StaticAnalyzer/
Checkers/
1 line
4 lines
177 lines
test/
Analysis/
20 lines

Diff 28934

lib/StaticAnalyzer/Checkers/CMakeLists.txt

Context not available.
IdenticalExprChecker.cpp IdenticalExprChecker.cpp
IvarInvalidationChecker.cpp IvarInvalidationChecker.cpp
LLVMConventionsChecker.cpp LLVMConventionsChecker.cpp
LossOfSignChecker.cpp
MacOSKeychainAPIChecker.cpp MacOSKeychainAPIChecker.cpp
MacOSXAPIChecker.cpp MacOSXAPIChecker.cpp
MallocChecker.cpp MallocChecker.cpp
Context not available.

lib/StaticAnalyzer/Checkers/Checkers.td

Context not available.
let ParentPackage = CoreAlpha in { let ParentPackage = CoreAlpha in {
def LossOfSignChecker : Checker<"LossOfSignAssign">,
danielmarjamakiUnsubmitted
Done
ReplyInline Actions

Please put this in alphabetical order somewhat.

danielmarjamaki: Please put this in alphabetical order somewhat.
HelpText<"Warn about (possible) loss of sign in assignments and initializations">,
DescFile<"LossOfSignChecker.cpp">;
def BoolAssignmentChecker : Checker<"BoolAssignment">, def BoolAssignmentChecker : Checker<"BoolAssignment">,
HelpText<"Warn about assigning non-{0,1} values to Boolean variables">, HelpText<"Warn about assigning non-{0,1} values to Boolean variables">,
DescFile<"BoolAssignmentChecker.cpp">; DescFile<"BoolAssignmentChecker.cpp">;
Context not available.

lib/StaticAnalyzer/Checkers/LossOfSignChecker.cpp

//== LossOfSignChecker.cpp - Loss of sign checker -----*- C/C++ -*--==//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//
//
// This defines LossOfSignChecker, which performs checks for assignment of
// signed negative values to unsigned variables.
danielmarjamakiUnsubmitted
Done
ReplyInline Actions

I don't know see the connection against "ExprEngine".. so this comment looks confusing to me.. how about removing "a builtin check in ExprEngine"?

danielmarjamaki: I don't know see the connection against "ExprEngine".. so this comment looks confusing to me..
//
//===----------------------------------------------------------------------===//
#include "ClangSACheckers.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
#include "clang/StaticAnalyzer/Core/CheckerManager.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
using namespace clang;
using namespace ento;
namespace {
class LossOfSignChecker : public Checker<check::Bind, check::ASTDecl<VarDecl>> {
mutable std::unique_ptr<BuiltinBug> BT;
public:
void checkBind(SVal Loc, SVal Val, const Stmt *S, CheckerContext &C) const;
void checkASTDecl(const VarDecl *VD, AnalysisManager &mgr,
BugReporter &BR) const;
void doCheck(const VarDecl *VD, const Expr *RHS, SVal Val,
CheckerContext &C) const;
void emitReport(ProgramStateRef St, CheckerContext &C) const;
};
} // end anonymous namespace.
// Strip off all intervening operations to get to the real RHS.
static const Expr *getAbsoluteRHS(const Expr *Ex) {
while (Ex) {
danielmarjamakiUnsubmitted
Done
ReplyInline Actions

Please end all comments with a period ".".

danielmarjamaki: Please end all comments with a period ".".
Ex = Ex->IgnoreParenImpCasts();
danielmarjamakiUnsubmitted
Done
ReplyInline Actions

the first character in function names should be lower case.

danielmarjamaki: the first character in function names should be lower case.
if (const BinaryOperator *BO = dyn_cast<BinaryOperator>(Ex)) {
if (BO->getOpcode() == BO_Assign || BO->getOpcode() == BO_Comma) {
Ex = BO->getRHS();
continue;
}
}
break;
}
return Ex;
}
// Report diagnostic about loss of sign.
void LossOfSignChecker::emitReport(ProgramStateRef St,
CheckerContext &C) const {
if (ExplodedNode *N = C.addTransition(St)) {
if (!BT)
BT.reset(new BuiltinBug(this, "assigning negative value to "
"unsigned variable loses sign "
"and may cause undesired runtime "
"behavior"));
C.emitReport(llvm::make_unique<BugReport>(*BT, BT->getDescription(), N));
}
}
// Check if the said variable is unsigned and yet being assigned a negative
// value.
void LossOfSignChecker::doCheck(const VarDecl *VD, const Expr *RHS, SVal Val,
CheckerContext &C) const {
ProgramStateRef St = C.getState();
SValBuilder &SValBldr = C.getSValBuilder();
ConstraintManager &CM = C.getConstraintManager();
danielmarjamakiUnsubmitted
Done
ReplyInline Actions

Variables should start with a uppercase letter. And abbreviations are preferred.

danielmarjamaki: Variables should start with a uppercase letter. And abbreviations are preferred.
danielmarjamakiUnsubmitted
Done
ReplyInline Actions

I looked in the llvm coding style guide. It does not say that abbreviations are preferred as far as I see. Ignore my comment about that.

danielmarjamaki: I looked in the llvm coding style guide. It does not say that abbreviations are preferred as…
// Remove extraneous wrappers from the RHS value.
RHS = getAbsoluteRHS(RHS);
if (!RHS)
return;
// Catch the LHS and RHS types involved.
QualType VarTy = VD->getType();
QualType RHSTy = RHS->getType();
// Only look at binding of signed value type to unsigned variable type.
if (!VarTy->isUnsignedIntegerType() || !RHSTy->isSignedIntegerType())
return;
danielmarjamakiUnsubmitted
Done
ReplyInline Actions

this should probably be a early return.

danielmarjamaki: this should probably be a early return.
//
// check for negative value.
danielmarjamakiUnsubmitted
Done
ReplyInline Actions

Comments should start with uppercase

danielmarjamaki: Comments should start with uppercase
//
DefinedOrUnknownSVal Zero = SValBldr.makeZeroVal(RHSTy);
// Cast the RHS value to the correct (original) type that it appeared in.
SVal castVal = SValBldr.evalCast(Val, RHSTy, VarTy);
danielmarjamakiUnsubmitted
Done
ReplyInline Actions

you missed this variable, use uppercase for the first character.

danielmarjamaki: you missed this variable, use uppercase for the first character.
SVal LessThanZeroVal = SValBldr.evalBinOp(St, BO_LT, castVal, Zero, RHSTy);
if (Optional<DefinedSVal> LessThanZeroDVal =
danielmarjamakiUnsubmitted
Done
ReplyInline Actions

I assume you did not run clang-format on this?

clang-format -i LossOfSignChecker.cpp
danielmarjamaki: I assume you did not run clang-format on this? clang-format -i LossOfSignChecker.cpp
LessThanZeroVal.getAs<DefinedSVal>()) {
ProgramStateRef StatePos, StateNeg;
std::tie(StateNeg, StatePos) = CM.assumeDual(St, *LessThanZeroDVal);
if (StateNeg && !StatePos) {
// Binding of a negative value to a unsigned location.
emitReport(StateNeg, C);
}
}
danielmarjamakiUnsubmitted
Done
ReplyInline Actions

as far as I see this return is redundant

danielmarjamaki: as far as I see this return is redundant
}
void LossOfSignChecker::checkBind(SVal Loc, SVal Val, const Stmt *S,
CheckerContext &C) const {
danielmarjamakiUnsubmitted
Done
ReplyInline Actions

as far as I see this return is redundant

danielmarjamaki: as far as I see this return is redundant
// Skip statements in macros.
if (S->getLocStart().isMacroID())
return;
//
// "Walk" over the statement looking for possible loss of sign in assigning a
// known negative value to an unsigned location.
//
if (const BinaryOperator *B = dyn_cast<BinaryOperator>(S)) {
if (!B->isAssignmentOp()) {
return;
}
if (DeclRefExpr *DR = dyn_cast<DeclRefExpr>(B->getLHS())) {
if (VarDecl *VD = dyn_cast<VarDecl>(DR->getDecl()))
doCheck(VD, B->getRHS(), Val, C);
}
} else if (const DeclStmt *DS = dyn_cast<DeclStmt>(S)) {
// Iterate through the decls.
for (const auto *DI : DS->decls()) {
danielmarjamakiUnsubmitted
Done
ReplyInline Actions

Please use { on this outer if also. I'd say the { on the inner if is optional.

danielmarjamaki: Please use { on this outer if also. I'd say the { on the inner if is optional.
if (const auto *VD = dyn_cast<VarDecl>(DI))
doCheck(VD, VD->getInit(), Val, C);
}
}
}
void LossOfSignChecker::checkASTDecl(const VarDecl *VD, AnalysisManager &mgr,
BugReporter &BR) const {
// Only for globals.
if (VD->isLocalVarDeclOrParm())
return;
// Remove extraneous wrappers from the initializer.
const Expr *RHS = getAbsoluteRHS(VD->getInit());
if (!RHS)
return;
// Catch the LHS and RHS types involved.
QualType VarTy = VD->getType();
QualType RHSTy = RHS->getType();
// Only look at binding of signed value type to unsigned variable type.
if (!VarTy->isUnsignedIntegerType() || !RHSTy->isSignedIntegerType())
return;
//
// check for negative value.
//
llvm::APSInt Result;
if (RHS->EvaluateAsInt(Result, BR.getContext())) {
danielmarjamakiUnsubmitted
Done
ReplyInline Actions

early return

danielmarjamaki: early return
if (Result.isNegative()) {
// Initialization of unsigned variable with signed negative value.
SmallString<64> Buf;
llvm::raw_svector_ostream Os(Buf);
Os << "assigning negative value to unsigned variable loses sign "
"and may cause undesired runtime behavior";
PathDiagnosticLocation L =
PathDiagnosticLocation::create(VD, BR.getSourceManager());
BR.EmitBasicReport(VD, this, "Loss of sign on assignment", "Loss of Sign",
Os.str(), L);
}
}
}
void ento::registerLossOfSignChecker(CheckerManager &mgr) {
mgr.registerChecker<LossOfSignChecker>();
}

test/Analysis/LossOfSignAssign.c

// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.core.LossOfSignAssign -verify %s
unsigned char uc1 = -1; // expected-warning {{assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior}}
signed char sc1 = -1;
static unsigned int sui1 = -1; // expected-warning {{assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior}}
int t1 () {
static unsigned int sui2 = -1; // expected-warning {{assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior}}
int x = -10;
int i = -1;
unsigned int j = -1; // expected-warning {{assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior}}
i = x;
j = i; // expected-warning {{assigning negative value to unsigned variable loses sign and may cause undesired runtime behavior}}
j = (unsigned int)x; // explicit cast; don't warn
return i+j; // implicit conversion here too!
}
zaks.annaUnsubmitted
Not Done
ReplyInline Actions

What happens if the return type is unsigned?

zaks.anna: What happens if the return type is unsigned?
soumitraAuthorUnsubmitted
Not Done
ReplyInline Actions

Do you think this would be something good to catch? I somehow assumed that this might be more noisy and less useful than the assignment cases?

soumitra: Do you think this would be something good to catch? I somehow assumed that this might be more…