how to fix brp and rpmlint warnings – today: I: Statement might be overflowing a buffer in strncat.

Malcolm Lewis sent me a link to one of his failing packages, with the idea to bring the next post for this series (which has had a too long break already). So Malcolm: Thank you for the heads up and here we go with another hopefully helpful solution to one of brp error (brp actually stands for Build Root Policy)

So, just like before, we have a look at the error, including the specific code parts raising the errors:

I: Statement might be overflowing a buffer in strncat. Common mistake:
BAD: strncat(buffer,charptr,sizeof(buffer)) is wrong, it takes the left over size as 3rd argument
GOOD: strncat(buffer,charptr,sizeof(buffer)-strlen(buffer)-1)

Ok, so the error is very helpful with even telling us what is a bad use of strncat and what a good use of it would look like. For completeness, let’s see what the compiler itself reports on this error:

In function ‘strncat’,
inlined from ‘gtkui_icmp_redir’ at ec_gtk_mitm.c:173:14:
/usr/include/bits/string3.h:152:3: warning: call to __builtin___strncat_chk might overflow destination buffer [enabled by default]

Let’s see what man 3 strncat tells us about the usage of strncat.

Synopsis

#include
char *strcat(char *dest, const char *src);
char *strncat(char *dest, const char *src, size_t n);

Description
The strcat() function appends the src string to the dest string overwriting the ‘\0’ character at the end of dest, and then adds a terminating ‘\0’ character. The strings may not overlap, and the dest string must have enough space for the result.

The strncat() function is similar, except that it will use at most n characters from src. Since the result is always terminated with ‘\0’, at most n+1 characters are written.

The package suggested by Malcolm contains this code (snipped together):

#define PARAMS_LEN 50
static char params[PARAMS_LEN];

strncat(params, gtk_entry_get_text(GTK_ENTRY(entry1)), PARAMS_LEN);
strncat(params, "/", PARAMS_LEN);
strncat(params, gtk_entry_get_text(GTK_ENTRY(entry2)), PARAMS_LEN);

This is not an uncommon error at all. We create a buffer, 50 chars long, we concatenate text into the buffer and specify a max length of PARAMS_LEN. What is not taken into account here is that the string in params is getting longer, and we do not account for this when adding text to it. And additionally the leading \0 is ignored as well.

Translating the BRP Result’s suggestion stating how we should write this code, we will result in this:
#define PARAMS_LEN 50
static char params[PARAMS_LEN];

strncat(params, gtk_entry_get_text(GTK_ENTRY(entry1)), sizeof(params)-strlen(params)-1);
strncat(params, "/", sizeof(params)-strlen(params)-1);
strncat(params, gtk_entry_get_text(GTK_ENTRY(entry2)), sizeof(params)-strlen(params)-1);

sizeof(params) is what we had there before, and equals to PARAMS_LEN. But we now also take into account that we already do have some characters stored in params and also account for a trailing NULL byte.

And with this simple fix, the error vanished.