Overzealous strnlen checking

[johnsonjh]

@yonhan3

I have an application that would need quite a few modifications to build with OpenOSC, which I think shouldn't be necessary (or at least optional), since strnlen checking is, in my opinion, overzealous, and the modifications would make the program less understandable if not less secure.

Take this code for example:

#include <stddef.h>
#include <string.h>

#define MAX_STRING_LEN 32

int main(void) {
    static const char* string = "MyString";
    /* ... */
    size_t len = strnlen(string, MAX_STRING_LEN);

    return 0;
}

$ gcc -O test.c -include openosc.h -lopenosc

In file included from /usr/include/openosc_map.h:21,
                 from /usr/include/openosc.h:179,
                 from <command-line>:
In function ‘strnlen’,
    inlined from ‘main’ at test.c:9:18:
/usr/include/openosc_redirect_map.h:570:36: error: call to ‘openosc_strnlen_chk_warn’ declared with attribute error: strnlen caller with bigger length than size of source buffer
  570 |                   : (STRNLEN_CASE2 openosc_strnlen_chk_warn(_sz, src, len)))
      |                     ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I can't see what the point is in raising an error here, at least not in all cases... this means that we'd need to have some code where truncation actually happens (or the size is exactly the same like) to not raise an error:

     static const char* string = "MyStringxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx";       

In my use case, I have code like this (that is used in example code). It conveys useful information both to the programmer that the MAX_STRING_LEN (as set in headers) is the maximum usable string length, and also to the program later, which can truncate strings to len later on.

Replacing this kind of code with something like ...

    static const char* string = "MyString"; /* Maximum usable length of string is MAX_STRING_LEN */

... doesn't seem to be a great benefit (and now many IDE's won't show the user value of MAX_STRING_LEN because it is in a comment, etc.)

(Using strlen in all the cases where OpenOSC would complain is fine too, but isn't helpful when using strlen is banned by policy, along with the other non-n functions.)

[yonhan3]

Thanks for opening this issue.

I think OpenOSC can add a new define flag to turn on/off the remapping of strnlen():

-DOPENOSC_STRNLEN_DISABLE

By default, we will remap strnlen, but if you explicitly add the above flag in your CFLAGS, then we will disable strnlen() remapping, thus not throwing errors for strnlen().

what do you think?

[johnsonjh]

That would work, but it would be extra great if within the ifdef in openosc.h, you could also set OPENOSC_STRLEN_MAP_DISABLED (or similar) when OPENOSC_STRNLEN_DISABLE has been recognized and defined.

That would be ideal because I plan to put a workaround in place to allow our code to be compiled with OpenOSC enabled, and without requiring the person (or build system) doing the build to be aware of this and change their build system configuration:

#if defined(__OPENOSC_H__)
# define strnlen_trunc(s, l) strlen(s)
#else
# define strnlen_trunc(s, l) strnlen(s, l)
#endif 

I then use my strnlen_trunc function in place of strnlen in the places where OpenOSC complains but I know this usage is safe and correct.

With your proposed solution, but also setting OPENOSC_STRLEN_MAP_DISABLED, I'd be able to amend my workaround to:

#if defined(__OPENOSC_H__) && !defined(OPENOSC_STRLEN_MAP_DISABLED)
#  define strnlen_trunc(s, l) strlen(s)
# else
#  define strnlen_trunc(s, l) strnlen(s, l)
#endif

That way, if OpenOSC remapping of strnlen is disabled, strnlen_trunc remains strnlen used as usual, and for backwards compatibility, such as if someone builds with -DOPENOSC_STRNLEN_DISABLE against an older version of OpenOSC that doesn't support disabling the remapping, the workaround will still be activated.

[yonhan3]

Sure, good idea. I will set OPENOSC_STRNLEN_MAP_DISABLED (or similar) if OPENOSC_STRNLEN_DISABLE is defined.

I will add this in our next OpenOSC release.

Thanks!