search

kometbomb / klystrack

A chiptune tracker
http://kometbomb.github.io/klystrack/
Other
485 stars 28 forks source link

strncpy output may be truncated #238

Closed alexmyczko closed 5 years ago

alexmyczko commented 6 years ago

I am forwarding a debian bug report, for documentation purposes:

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=906476

kometbomb commented 6 years ago

Thanks, I'll look into this.

kometbomb commented 6 years ago

Fixed in klystron and klystrack. I don't have GCC 8 binaries so I am not sure if there are more warnings.

alexmyczko commented 6 years ago

Now I get, full log: http://sid.ethz.ch/debian/klystrack/NEW/klystrack_1.7.3%2Bgit20180828-1_i386.build (builds fine with gcc 7.3.0, fails with 8.2.0)

part of it: Generating dependencies for action.c... Generating dependencies for main.c... Compiling clipboard.c... Compiling menudefs.c... Compiling config.c... Compiling key.c... src/key.c: In function 'load_keymap': src/key.c:244:51: error: '%s' directive output may be truncated writing up to 999 bytes into a region of size 994 [-Werror=format-truncation=] snprintf(fullpath, sizeof(fullpath) - 1, "%s/key/%s", query_resource_directory(), tmpname); ^~ ~~~ src/key.c:244:2: note: 'snprintf' output 6 or more bytes (assuming 1005) into a destination of size 999 snprintf(fullpath, sizeof(fullpath) - 1, "%s/key/%s", query_resource_directory(), tmpname); ^~~~~~~~~~~~~~~~~~~~~~ src/key.c:234:2: error: 'strncpy' specified bound 1000 equals destination size [-Werror=stringop-truncation] strncpy(tmpname, name, sizeof(tmpname)); ^~~~~~~~~~~ src/key.c:246:2: error: 'strncpy' output may be truncated copying 100 bytes from a string of length 999 [-Werror=stringop-truncation] strncpy(mused.keymapname, tmpname, sizeof(mused.themename)); ^~~~~~~~~~~~~~~ src/key.c:238:3: error: 'strncpy' output may be truncated copying 100 bytes from a string of length 999 [-Werror=stringop-truncation] strncpy(mused.keymapname, tmpname, sizeof(mused.themename)); ^~~~~~~~~~~~~~~ cc1: all warnings being treated as errors make[3]: *** [Makefile:153: objs.release/key.o] Error 1

kometbomb commented 6 years ago

Those errors from the fact -Wstringop-truncation et al were introduced in GCC 8 and the Makefile has -Wall in the flags. And they are quite neat: the first one correctly deduces that the string can indeed be that long (because the intermediate buffers are 1000 and 1000 bytes long plus the "/key/" string between them. I will push a commit shortly.

kometbomb commented 6 years ago

Here you go: 0c8afa9

alexmyczko commented 6 years ago

almost:

Compiling key.c... src/key.c: In function 'load_keymap': src/key.c:246:2: error: 'strncpy' output may be truncated copying 99 bytes from a string of length 999 [-Werror=stringop-truncation] strncpy(mused.keymapname, tmpname, sizeof(mused.keymapname) - 1); ^~~~~~~~~~~~~~~~ src/key.c:238:3: error: 'strncpy' output may be truncated copying 99 bytes from a string of length 999 [-Werror=stringop-truncation] strncpy(mused.keymapname, tmpname, sizeof(mused.keymapname) - 1); ^~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors

kometbomb commented 6 years ago

So, we have a slight problem: the -Wstringop-truncation flag is not available on GCC <8.0 you can't simply say -Wno-stringop-truncation unless you also somehow check which compiler is being used (it would fail with GCC <8.0). In these cases this should fix it:

make EXTFLAGS="-Wno-stringop-truncation"

Or, #pragma could probably be used in combination with GCC version check.

kometbomb commented 6 years ago

I figured out a way to test against GCC and I am fixing issues.

alexmyczko commented 6 years ago

While you're at it, mind removing the contents of linux/ directory? And I'll also PR a klystrack.desktop file, just not sure yet to where exactly (directory)

kometbomb commented 6 years ago

Right. I set up Circle CI to simply compile the source and it is running GCC 8.0. That will catch all warnings my GCC 5 won't. All stringop related bugs are fixed in this repo but klystron still has a few of those.

That is, the original issue should now be fixed.

alexmyczko commented 6 years ago

@kometbomb this is what I get now, when I try to build it:

In file included from src/snd/music.c:34: src/snd/music.c: In function 'mus_load_instrument_RW': src/snd/music.c:1859:43: error: '' in boolean context, suggest '&&' instead [-Werror=int-in-bool-context] _VER_READ(&inst->program, (int)progstepssizeof(inst->program[0])); src/macros.h:41:3: note: in definition of macro 'VER' block;\ ^~~~~ src/macros.h:44:119: note: in expansion of macro 'SDL_RWread'

define VER_READ(file_version, first_version, last_version, var, size) VER(file_version, first_version, last_version, SDL_RWread(ctx, var, !size ? sizeof(*var)

: size, 1)); ^~~~~~ src/macros.h:45:28: note: in expansion of macro 'VER_READ'

define _VER_READ(x, size) VER_READ(version, 0, MUS_VERSION, x, size)

                        ^~~~~~~~

src/snd/music.c:1859:3: note: in expansion of macro '_VER_READ' _VER_READ(&inst->program, (int)progsteps*sizeof(inst->program[0])); ^~~~~ cc1: all warnings being treated as errors make[4]: [Makefile:138: objs.release/snd_music.o] Error 1 make[4]: Leaving directory '/var/www/debian/klystrack/NEW/klystrack-1.7.3+git20180911/klystron' make[3]: [Makefile:81: build] Error 2 make[3]: Leaving directory '/var/www/debian/klystrack/NEW/klystrack-1.7.3+git20180911/klystron' make[2]: [Makefile:129: build] Error 2 make[2]: Leaving directory '/var/www/debian/klystrack/NEW/klystrack-1.7.3+git20180911/klystrack' dh_auto_build: cd klystrack && make -j1 "INSTALL=install --strip-program=true" CFG=release returned exit code 2 make[1]: [debian/rules:15: override_dh_auto_build] Error 2 make[1]: Leaving directory '/var/www/debian/klystrack/NEW/klystrack-1.7.3+git20180911' make: *** [debian/rules:12: build] Error 2 dpkg-buildpackage: error: debian/rules build subprocess returned exit status 2 debuild: fatal error at line 1152: dpkg-buildpackage -rfakeroot -us -uc -ui failed

HarryUPf commented 6 years ago

Thanks @kometbomb , now it successfully compiles on manjaro-KDE. After that I had to manually copy the 'key' and 'res' folders into /usr/lib/klystrack/

alexmyczko commented 6 years ago

@HarryUPf should it not be /usr/share/klystrack/ since it's architecture independant?

HarryUPf commented 6 years ago

@alexmyczko this is what i get after a fresh compile. screenshot_20180913_000648 also, the AUR package (out-of-date) only copies the examples to /usr/share/klystrack/ https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=klystrack-git

alexmyczko commented 6 years ago

Aha, AUR forgets to put res/Default then: on Debian I get this list of files: dpkg -L klystrack:

8< ----- snip-snap /usr/lib/klystrack/res /usr/lib/klystrack/res/AHX /usr/lib/klystrack/res/Blacklyst /usr/lib/klystrack/res/C64 /usr/lib/klystrack/res/Classic /usr/lib/klystrack/res/Default /usr/lib/klystrack/res/Gameboy /usr/lib/klystrack/res/Golden_Brown /usr/lib/klystrack/res/Rust_Camo /usr/lib/klystrack/res/Rust_Red /usr/lib/klystrack/res/Ultimate_Proctamed /usr/share /usr/share/applications /usr/share/applications/klystrack.desktop /usr/share/doc /usr/share/doc/klystrack /usr/share/doc/klystrack/README.Debian /usr/share/doc/klystrack/changelog.Debian.gz /usr/share/doc/klystrack/copyright /usr/share/icons /usr/share/icons/klystrack.png /usr/share/man /usr/share/man/man1 /usr/share/man/man1/klystrack.1.gz

alexmyczko commented 5 years ago

I dropped -Werror :)