martin-lueders / ML_modules

free modules for VCV Rack
BSD 3-Clause "New" or "Revised" License
90 stars 18 forks source link

v1: Cloner buffer overflow #70

Closed cschol closed 2 years ago

cschol commented 2 years ago

This is an issue on v1 of the plugin.

The compiler shows the following warning:

src/Cloner.cpp:131:40: warning: ‘__builtin___sprintf_chk’ writing a terminating nul past the end of the destination [-Wformat-overflow=]
  131 |     if(value) {sprintf(displayStr, "%2u", (unsigned) *value);}
      |                                        ^
In file included from /usr/include/stdio.h:867,
                 from /usr/include/c++/9/cstdio:42,
                 from ../../include/common.hpp:6,
                 from ../../include/rack.hpp:6,
                 from src/ML_components.hpp:1,
                 from src/ML_modules.hpp:1,
                 from src/Cloner.cpp:1:
/usr/include/x86_64-linux-gnu/bits/stdio2.h:36:34: note: ‘__builtin___sprintf_chk’ output between 3 and 11 bytes into a destination of size 2
   36 |   return __builtin___sprintf_chk (__s, __USE_FORTIFY_LEVEL - 1,
      |          ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   37 |       __bos (__s), __fmt, __va_arg_pack ());
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

When I run Rack in development mode (with make run) and add Cloner it will crash with a buffer overflow. This does not happen when I use official Rack version 1.1.6 and the Library module, but I think that might only be working by accident (or just not halting the application due to buffer overflow in non-debug mode).

The fix is easy: just increase the display character array by 1 to size 3. Looks like v2 already has a refactored version of this code.

Note, that v1 updates will continue to be accepted.

martin-lueders commented 2 years ago

Thanks for pointing this out, The Windows based compilation did not issue any warning, but I could reproduce the problem under Linux. The issue should be fixed in the latest commit to v1. Indeed the code has changed in v2. I will later send the library update requests for both versions.

cschol commented 2 years ago

Confirmed fixed in v1. Thank you!