nimble-code / Cobra

An interactive (fast) static source code analyzer
139 stars 31 forks source link

`cobra_json.c`: 'sprintf' output between 3 and 523 bytes into a destination of size 512 #39

Closed yilmazdurmaz closed 2 years ago

yilmazdurmaz commented 2 years ago

I haven't used JSON output format but the following warning is coming up when trying to re-compile.

cobra_json.c: In function 'check_bvar':
cobra_json.c:65:23: error: 'sprintf' may write a terminating nul past the end of the destination [-Werror=format-overflow=]
   65 |  { sprintf(buf, "%s %d", c->txt, c->lnr);
      |                       ^
cobra_json.c:65:4: note: 'sprintf' output between 3 and 523 bytes into a destination of size 512
   65 |  { sprintf(buf, "%s %d", c->txt, c->lnr);
      |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

I tried to increase the buffer to 1024 and it seems buffer size does not matter and there are always extra 11 bytes added before line 65:

note: 'sprintf' output between 3 and 1035 bytes into a destination of size 1024

I can't see the cause (not enough level), thus for now I continue compiling with CFLAGS= ... -Wno-error=format-overflow to get around it.

nimble-code commented 2 years ago

There's a test on the length of the string pointed to by c->txt at the top of that function, but it doesn't account for the length of the " %d" field. I'm impressed that the compiler figured that out. I'll change the sprintf into an snprintf -- but just out of curiosity: which compiler are you using?

yilmazdurmaz commented 2 years ago

@nimble-code To compile, I use a very basic toolset: gcc, musl-dev, byacc, and make.

The system is a bit hard to spell: An Alpine Linux container in a Docker environment that uses WSL2 on Windows 10 :) I am not fully sure, but in theory, all Alpine Linux instances should behave the same.

nimble-code commented 2 years ago

which version of gcc?

nimble-code commented 2 years ago

the replacement for that sprintf line: { snprintf(buf, sizeof(buf), "%s %d", c->txt, c->lnr);

yilmazdurmaz commented 2 years ago

I couldnt read which part to send. here is the full result from gcc -v

Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-alpine-linux-musl/10.3.1/lto-wrapper
Target: x86_64-alpine-linux-musl
Configured with: /home/buildozer/aports/main/gcc/src/gcc-10.3.1_git20211027/configure --prefix=/usr --mandir=/usr/share/man --infodir=/usr/share/info --build=x86_64-alpine-linux-musl --host=x86_64-alpine-linux-musl --target=x86_64-alpine-linux-musl --with-pkgversion='Alpine 10.3.1_git20211027' --enable-checking=release --disable-fixed-point --disable-libstdcxx-pch --disable-multilib --disable-nls --disable-werror --disable-symvers --enable-__cxa_atexit --enable-default-pie --enable-default-ssp --enable-cloog-backend --enable-languages=c,c++,d,objc,go,fortran,ada --disable-libssp --disable-libmpx --disable-libmudflap --disable-libsanitizer --enable-shared --enable-threads --enable-tls --with-system-zlib --with-linker-hash-style=gnu
Thread model: posix
Supported LTO compression algorithms: zlib
gcc version 10.3.1 20211027 (Alpine 10.3.1_git20211027)
nimble-code commented 2 years ago

version 10.3.1 -- that explains -- I still use version 9.3.0 -- time to update! thanks for reporting this!

yilmazdurmaz commented 2 years ago

@nimble-code by the way, snprintf has another warning now. at least it now says "truncation may happen" with tha same 11 bytes extra

cobra_json.c: In function 'check_bvar':
cobra_json.c:66:37: error: 'snprintf' output may be truncated before the last format character [-Werror=format-truncation=]
   66 |  { snprintf(buf, sizeof(buf), "%s %d", c->txt, c->lnr);
      |                                     ^
cobra_json.c:66:4: note: 'snprintf' output between 3 and 523 bytes into a destination of size 512
   66 |  { snprintf(buf, sizeof(buf), "%s %d", c->txt, c->lnr);
      |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
nimble-code commented 2 years ago

sigh, okay -- i'll fix it so that it wont trigger warnings

nimble-code commented 2 years ago

I updated my Ubuntu installation (WSL) but somehow I still have only gcc version 9.3.0. Best I can think of is to do this update, but I can't test it:

static void check_bvar(Prim *c) { char buf[512];

if ((c->mark & (4|16)) == 0
||   strlen(c->txt) + 16 >= sizeof(buf)-1)
{   return;
}

if (c->mark & 4)    // matched
{   sprintf(buf, "->%d", c->lnr);
} else          // defined
{   snprintf(buf, sizeof(buf), "%s %d", c->txt, c->lnr);
}
yilmazdurmaz commented 2 years ago

I will try it now

yilmazdurmaz commented 2 years ago

@nimble-code

Ok, that changes in line 58 and 65 are doing fine but this now got an interesting turn (at least for me). I have kept the sprintf and changed only this line by adding +1 instead of +16 you have used and the warning is now gone.

at line 58:

    ||   strlen(c->txt) +1 >= sizeof(buf)-1)

I don't know what has happened there. maybe this +1 byte is now the placement for the "terminating nul" in the warning error.

I cannot tell if this +1 is enough, so it is your decision to change only this +1 or both lines you wrote.

nimble-code commented 2 years ago

interesting -- the static analysis just gave up at that point and suppresses the warning! ok, solved then