A passionate openSUSE user
RSS icon Home icon
  • how to fix brp and rpmlint warnings – today: I: Statement might be overflowing a buffer in strncat.

    Posted on May 28th, 2011 Dominique Leuenberger 2 comments

    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.

  • how to fix brp and rpmlint warnings – today:
    I: Program causes undefined operation
    (likely same variable used twice and post/pre incremented in the same expression)

    Posted on March 28th, 2011 Dominique Leuenberger No comments

    It seems the planned series sort of finds an audience, which in turn of course is motivational to keep on writing it. Today, we’ll have a look at this Informational message in BRP checks:

    I: Program causes undefined operation
    (likely same variable used twice and post/pre incremented in the same expression).
    e.g. x = x++; Split it in two operations.

    This is currently informational only and is not failing the build, but you might want to address them together with upstream.

    I assume you do know what “a++” means in C (otherwise, you should start reading C-books), so we just try to reproduce this error in a simple c test case:
    #include <stdio.h>

    int main() {
    int i=5;
    i = i++ * ++i;
    printf("The current value of i is %d\n", i);
    return 0;
    }

    When building this using gcc -Wall test.c, we get this compiler warning (which in turn is what brp translates to the information we’re discussing here)

    > gcc -Wall test2.c
    test.c: In function ‘main’:
    test.c:5:5: warning: operation on ‘i’ may be undefined

    So, let’s first see for ourselves what we would expect this to be? Hmm.. already for us, this looks not logic (and I surely hope nobody would write this code).
    Let’s just see what starting this executable gives shall we?

    The current value of i is 37

    Now, is this surprising? We multiplied, assigned it to i and as a result we get a prime number? By closely analyzing the line you will likely understand what the compiler did. But was this expected? If this is actually what the programmer intended, the code should just be rewritten to be more logical, like:
    i = (i+1) ^2 + 1;
    This is understandable for all of us and does not yield the surprise of what is going on.

    And that is actually all this warning is about: it requests the programmer to write code that can be understood and does not depend on what the compiler interprets. It might even very well be that the different optimization levels or the usage of different compilers might end up in different results.

    If you want to read some more about this topic, I suggest to have a look at:
    Wikipedia
    http://publications.gbdirect.co.uk/c_book/chapter8/sequence_points.html