[PATCH] D10634: Loss of Sign Checker

[Soumitra Chatterjee]

In http://reviews.llvm.org/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.


http://reviews.llvm.org/D10634