One day looking through code



  • Here are some gems I have found looking through code. I am used to find a few strange things, however finding so many in one day was unexpected.


    Copied & pasted into 2 separate places (seems good code after all :-):
    ...
    sprintf(tmp, "%s ", "and");
    ...

     
    Based on another function which had 2 case statements. I guess the key is future extensibility:
    ...
    BOOL IsOfTypeXYZ(long nType)
    {
       switch (nType) {
          case TYPE_XYZ:
               return TRUE;
          default:
               break;
       }
       return FALSE;
    }
    .....

     

    I confronted the developer about this one. The answer was indeed "future extensibility". I am showing the short instance (which is the worst). There also was an instance with more switch cases, and the stated reason for using the loop was "have to make sure the functions are called in a correct order and some parts of it have to be excluded with #ifndef's".
    ....
    #define VAR_TYPE_1  0
    ....
    for (int i = 0; i <= VAR_TYPE_1; i++) {
       swtich(i){
         case VAR_TYPE_1: 
              f1();
       }
    }

     

     

     



  • Some examples from our own code base, found near sprintf (var, "%s", string) code: 

    How to clear a string in C: 

    sprintf (var, "%s", "");

    How to check if a string is empty:

    if (strlen (var) <= 0) 

    How to copy a string without that "%s" crap:

    sprintf (dest, "My string");

    And, not in the same league, but a bit funny:

    sprintf (msg, "Unable to open file ""%s"" for read", fname);

    Guess they never checked that the error message had any quotes in it... 

     



  • @andy314 said:

    #define VAR_TYPE_1  0
    ....
    for (int i = 0; i <= VAR_TYPE_1; i++) {
       swtich(i){
         case VAR_TYPE_1: 
              f1();
       }
    }
     

    Though not quite so bad, at my old job we had one guy that seemed to hate if()'s. I guess at some point, somebody told him a "case" was faster, so instead of if/else, if he was checking a single variable, he used a switch/case/default. It was terrible!  



  • @Skybert said:

    And, not in the same league, but a bit funny:

    sprintf (msg, "Unable to open file ""%s"" for read", fname);

    Guess they never checked that the error message had any quotes in it... 

    I don't think it's a big deal if the error message had quotes, it would just make for an uglier logfile (or wherever that's going). It's the potential buffer overflow that bugs me (if the user can set the name of the file, which seems possible):

    ./thatprogram -f perl -e print 'A'x10000 

     



  • Being as the signature of sprintf is "int sprintf(char *s, const char *format, ...)" it is good practice to be in the habit of writing for instance:

       sprintf(msg, "%s", "100% correct");

    instead of

       sprintf(msg, "100% correct");

     



  • Some people like to live dangerously.  Like the people who wrote the animated cursor loading code in Windows.  Seriously, it is embarassing for coders everywhere when things like that happen.  I would happily go through all Windows code, FOR FREE, just to fix all these mistakes / oversights.  My only fear is that I might kill myself out of depression after a few weeks.  Anyway, reading x number of bytes into a buffer of size y bytes without checking x against y is just silly.

    Note: By silly, I of course mean totally fucking retarded.



  • @Skybert said:

    How to check if a string is empty:

    if (strlen (var) <= 0) 

     

    How else would you test if a string is empty.  I was taught to use this method because you can't use sizeof if it's a pointer and checking for NULL won't always work either.



  • @ssprencel said:

    @Skybert said:

    How to check if a string is empty:

    if (strlen (var) <= 0) 

    How else would you test if a string is empty.  I was taught to use this method because you can't use sizeof if it's a pointer and checking for NULL won't always work either.


    if(var == NULL || *var = '\0')

    Using strlen() is much slower, since it iterates through the string until it finds a terminating null -- and it might crash the program if the null is missing.


  • @Carnildo said:


    if(var == NULL || *var = '\0')

    Using strlen() is much slower, since it iterates through the string until it finds a terminating null -- and it might crash the program if the null is missing.


    shouldn't that be

    if(var == NULL || *var == '\0')




  • @rbowes said:

    Though not quite so bad, at my old job we had one guy that seemed to hate if()'s. I guess at some point, somebody told him a "case" was faster, so instead of if/else, if he was checking a single variable, he used a switch/case/default. It was terrible!  

     

    Ah man, did you really have to remind me on that??? I've had to rewrite code that was full of:

    Select Case bVal

       Case True:   ' Do something

       Case False:   ' Do something else

       Case Else:   ' Some "just to be sure" code

    End Select

     

    EDIT: Yes, bVal was of type boolean.



  • I see the opposite a lot.

    if else if else if else if else if else if else if

    And with the same method call in the ifs, of course.



  • @newfweiler said:

    Being as the signature of sprintf is "int sprintf(char *s, const char *format, ...)" it is good practice to be in the habit of writing for instance:

       sprintf(msg, "%s", "100% correct");

    instead of

       sprintf(msg, "100% correct");

    Err... umm.... or perhaps just use strcpy(), or better yet, strncpy()?  (You're right of course, your suggestion is better practice than the original, but might as well avoid the varargs and format-string-processing overhead.)



  • @AssimilatedByBorg said:

    @newfweiler said:

    Being as the signature of sprintf is "int sprintf(char *s, const char *format, ...)" it is good practice to be in the habit of writing for instance:

       sprintf(msg, "%s", "100% correct");

    instead of

       sprintf(msg, "100% correct");

    Err... umm.... or perhaps just use strcpy(), or better yet, strncpy()?  (You're right of course, your suggestion is better practice than the original, but might as well avoid the varargs and format-string-processing overhead.)

    You're right:  strncpy() is better.  There's no point in formatting if there's nothing to format.  But with regard to printf(), there is no non-formatting equivalent.  The first argument is the format string, not the string to be printed, so 'printf("100% correct");' is not correct.  It will compile and link but it may raise an error at runtime.

     


  • Discourse touched me in a no-no place

    @newfweiler said:

    But with regard to printf(), there is no non-formatting equivalent. 
    Erm - yes there is. @newfweiler said:
    The first argument is the format string, not the string to be printed, so 'printf("100% correct");' is not correct.
    It's incorrect, but apparently not for the reasons you think @newfweiler said:
      It will compile and link but it may raise an error at runtime.
    In that case, you'd want the following:

     printf("100%% correct")



  • @PJH said:

    @newfweiler said:

    But with regard to printf(), there is no non-formatting equivalent. 
    Erm - yes there is.

    You should at least mention the name - puts(); 



  • From what I understad about varargs and printf, it won't look at the arguments until it finds a % character in the format string. Still, it has to scan it for %, so it's slower.



  • @newfweiler said:

    @AssimilatedByBorg said:
    @newfweiler said:

    Being as the signature of sprintf is "int sprintf(char *s, const char *format, ...)" it is good practice to be in the habit of writing for instance:

       sprintf(msg, "%s", "100% correct");

    instead of

       sprintf(msg, "100% correct");

    Err... umm.... or perhaps just use strcpy(), or better yet, strncpy()?  (You're right of course, your suggestion is better practice than the original, but might as well avoid the varargs and format-string-processing overhead.)

    You're right:  strncpy() is better.  There's no point in formatting if there's nothing to format.

     

    No! strncpy() is a very bad function. It is one of the historical design blunders in the C standard library - this function does not do what you think it does. It is just as bad as strcpy(), but in a different way: if the source string is too long, then the destination will not be null-terminated. Many security advisories have occurred as a result of programmers thinking that strncpy was a safe alternative to strcpy. There are almost no cases in which strncpy is any use at all.

    The actual safe alternative to strcpy, in the C standard library? snprintf(dest, n, "%s",  src). Ironic, I know. Otherwise you do it by hand with memcpy() and compute the lengths yourself, if speed really matters here.

    (Some systems have strlcpy, which does what you think strncpy should do, but it's not available in very many places) 



  • @asuffield said:

    No! strncpy() is a very bad function. It is one of the historical design blunders in the C standard library - this function does not do what you think it does. It is just as bad as strcpy(), but in a different way: if the source string is too long, then the destination will not be null-terminated. Many security advisories have occurred as a result of programmers thinking that strncpy was a safe alternative to strcpy. There are almost no cases in which strncpy is any use at all.

    The actual safe alternative to strcpy, in the C standard library? snprintf(dest, n, "%s",  src). Ironic, I know. Otherwise you do it by hand with memcpy() and compute the lengths yourself, if speed really matters here.

    (Some systems have strlcpy, which does what you think strncpy should do, but it's not available in very many places) 

    Or you could just do the safe and quick thing...

    [code]strncpy(dest, src, BUFFER_LEN - 1);[/code]

    [code]dest[BUFFER_LEN - 1] = 0;[/code]

     



  • It's not really that difficult.
    char * strlcpy ( char * destination, const char * source, size_t num )
    {
        strncpy(destination, source, num);
        destination[num-1] = '\0';
    }
     
    And strncpy makes perfect sense if you're using static char arrays, but nobody cares about that extra byte these days.


  • @Faxmachinen said:

    It's not really that difficult.

    char * strlcpy ( char * destination, const char * source, size_t num )
    {
        strncpy(destination, source, num);
        destination[num-1] = '\0';
    }
     
    And strncpy makes perfect sense if you're using static char arrays, but nobody cares about that extra byte these days.
    You might as well do num-1 in the strncpy call as well - you are about to overwrite the numth character anyway!

Log in to reply