Why does my C Compiler let me get away with this?



  • I was researching an application issue today and noticed an obscenely large error number showing up in the log - here's the actual line from the log file:

    11/03/08 09:50:44     0 AF 8 ship_app    1000 DROP OFF ERROR SQL:0,valid:-9223372032559672606                          0 0

    Upon searching the code to find where this error msg is being logged from and found this:

    sprintf(jnlmsg,"DROP OFF ERROR SQL:%ld,valid:%ld", sqlca.sqlcode);

    C'mon compiler!  You should be able to save me from my own stupidity!!!



  • It should:

    daid@server:~> cat test.c
    #include >stdio.h<
    
    int main(int argc, char** argv)
    {
            long i;
            char buffer[1024];
            sprintf(buffer, "DROP OFF ERROR SQL:%ld,valid:%ld", i);
            return 0;
    }
    daid@server:~> gcc -o test.o -c test.c -Wall
    test.c: In function `main':
    test.c:7: warning: too few arguments for format
    


  •  Such is the magic* of varargs in C.



  • @bstorer said:

     Such is the magic* of varargs in C.

    *By magic I mean problem

     

    Oh, if that was the only one...  Varargs are also completely type-unsafe, and there's no way to determine the number of arguments without additional information.



  • Why? Since jnlmsg only requires one %s argument, you've fulfilled the compiler-determinable requirements. sprintf the error codes into the constant string before sprintfing them into jnlmsg and the compiler will CYA again.



  • This is why you should run lint against your code.  A lexical analysis of the code would have found that.  You do have a lint available, don't you?



  • @mrprogguy said:

    You do have a lint available, don't you?
    If not, check the trap in your dryer, or failing that, your navel.



  • @mrprogguy said:

    You do have a lint available, don't you?

    Yes, and there-in lies part of the rub.  The app itself is written in Oracle's Pro*C which gets transmogrified into a real C file and then compiled.  Running lint on the transmogrified file catches all the garbage that Oracle puts in there, but not this thing. 

    In fact, I ran lint on the example above and got this:

    lint: "tdwtf.c", line 7: warning 825: "i" may be used before set
    lint: "tdwtf.c", line 3: warning 828: argument "argc" unused in function "main"
    lint: "tdwtf.c", line 3: warning 828: argument "argv" unused in function "main"


    ==============
    name declared but never used or defined
        snprintf    stdio.h(593)
        __bufendtab         stdio.h(647)
        vsnprintf           stdio.h(594)
        __iob       stdio.h(172)
    function returns value which is always ignored
        sprintf

    Now, admittedly, I could be doing it wrong, but looks like it only caught the unused variables, not the really bad sprintf error.

    With that said, I haven't completely ruled out following up on bstorer's suggestion...

     



  • Not only that, but IMHO the compiler should also be suggesting that you not use sprintf at all, assuming snprintf is available.  But at least you have a shot at not having a buffer overflow with only %ld args, versus having 1 or more %s args.



  • @DoctorFriday said:

    C'mon compiler!  You should be able to save me from my own stupidity!!!

    If it did, it would be VB.

    C assumes you know WTF you are doing, it may not like it, but you are the programmer.

    VB assumes you need help, you can still write poor code, but VB will assist you in doing so.



  • @Daid said:

    It should:

    daid@server:~> cat test.c
    #include >stdio.h<
    
    int main(int argc, char** argv)
    {
            long i;
            char buffer[1024];
            sprintf(buffer, "DROP OFF ERROR SQL:%ld,valid:%ld", i);
            return 0;
    }
    daid@server:~> gcc -o test.o -c test.c -Wall
    test.c: In function `main':
    test.c:7: warning: too few arguments for format
    

    GCC is unique in that it can do type-checking on printf() format strings. None of the commercial compiler writers have figured out how to do it.



  • @DoctorFriday said:

    @mrprogguy said:

    You do have a lint available, don't you?

    Yes, and there-in lies part of the rub.  The app itself is written in Oracle's Pro*C which gets transmogrified into a real C file and then compiled.  Running lint on the transmogrified file catches all the garbage that Oracle puts in there, but not this thing. 

    In fact, I ran lint on the example above and got this:

    lint: "tdwtf.c", line 7: warning 825: "i" may be used before set
    lint: "tdwtf.c", line 3: warning 828: argument "argc" unused in function "main"
    lint: "tdwtf.c", line 3: warning 828: argument "argv" unused in function "main"


    ==============
    name declared but never used or defined
        snprintf    stdio.h(593)
        __bufendtab         stdio.h(647)
        vsnprintf           stdio.h(594)
        __iob       stdio.h(172)
    function returns value which is always ignored
        sprintf

    Now, admittedly, I could be doing it wrong, but looks like it only caught the unused variables, not the really bad sprintf error.

    With that said, I haven't completely ruled out following up on bstorer's suggestion...

     

    Well, that's a fine kettle of something.  The long and short of the problem is that compilers tend not to do a lexical study of varargs calls because it would make them so much slower, which is why lint is there.  BUT.  Having to ferret out which was your code and which was Oracle's in a lint report, though--it's not Dante's vision of Hell, but it's right up there.




  • @DoctorFriday said:

     

    sprintf(jnlmsg,"DROP OFF ERROR SQL:%ld,valid:%ld", sqlca.sqlcode);

     boost::format



  • @Carnildo said:

    @Daid said:
    It should:

    daid@server:~> cat test.c
    #include >stdio.h<

    int main(int argc, char** argv)
    {
    long i;
    char buffer[1024];
    sprintf(buffer, "DROP OFF ERROR SQL:%ld,valid:%ld", i);
    return 0;
    }
    daid@server:~> gcc -o test.o -c test.c -Wall
    test.c: In function `main':
    test.c:7: warning: too few arguments for format

    GCC is unique in that it can do type-checking on printf() format strings. None of the commercial compiler writers have figured out how to do it.

     

    How sure are you?

    @VisualStudio 8 said:

    >------ Build started: Project: Test, Configuration: Debug Win32 ------
    1>Compiling...
    1>test.cpp
    1>...\test.cpp(7) : warning C4313: 'sprintf' : '%d' in format string conflicts with argument 2 of type 'char *'
    1>...\test.cpp(7) : warning C4996: 'sprintf': This function or variable may be unsafe. Consider using sprintf_s instead. To disable deprecation, use _CRT_SECURE_NO_WARNINGS. See online help for details.
    1>...\test.cpp(7) : warning C4700: uninitialized local variable 'i' used



  • @ActionMan said:

     boost::format


    Or std::stringstream, for the matter at hand



  • @ulzha said:

    @ActionMan said:

     boost::format

    Or std::stringstream, for the matter at hand
    C != C++

    I had hoped that people here would get that distinction.  This is what I get for being optimistic.



  • @bstorer said:

    @ulzha said:

    @ActionMan said:

     boost::format

    Or std::stringstream, for the matter at hand
    C != C++

    I had hoped that people here would get that distinction.  This is what I get for being optimistic.

    You can translate "boost::format" to "GTFO of the stone age" if you like. C++ pwns C in this regard because you can get the compiler to report logic bugs as compile errors.


  • @ActionMan said:

    You can translate "boost::format" to "GTFO of the stone age" if you like. C++ pwns C in this regard because you can get the compiler to report logic bugs as compile errors.

     

    gcc catches this and many more if you ask for warnings. If your compiler doesn't, that's not the language's fault. 



  • @ActionMan said:

    @bstorer said:

    @ulzha said:

    @ActionMan said:

     boost::format

    Or std::stringstream, for the matter at hand
    C != C++

    I had hoped that people here would get that distinction.  This is what I get for being optimistic.

    You can translate "boost::format" to "GTFO of the stone age" if you like. C++ pwns C in this regard because you can get the compiler to report logic bugs as compile errors.
    Switching from C to C++ because you don't get warned for misusing sprintf is akin to flying coach because you don't like the brand of caviar they serve on your private jet.


  • @bstorer said:

    Switching from C to C++ because you don't get warned for misusing sprintf is akin to flying coach because you don't like the brand of caviar they serve on your private jet.

    I presume you meant "flying coach on Air Venezuela".  Otherwise, you are far too kind to C++. 



  • @DoctorFriday said:

     

    sprintf(jnlmsg,"DROP OFF ERROR SQL:%ld,valid:%ld", sqlca.sqlcode);

    I once submitted code like that in my final exam. It was a compiler, and the extra %d was on the generated comments. I found it funny that the teacher didn't realize that the generated .asm had weird comments like:

    LOOP 0982     ; End of loop -32835932

    Though the worst disaster I remember from using those functions was with the scanf() function. Do

    [code]scanf("%d", &myinteger);[/code]

    Then when you get asked for myinteger, type a letter instead. Wheee!!! All subsequent scanf()s pass through like crazy!!



  • @danixdefcon5 said:

    Filed under: [url=http://forums.thedailywtf.com/tags/The+editor+insists+on+changing+2600_amp_3B00+for+_2600_amp_3B00_amp/default.aspx]The editor insists on changing & for &amp[/url]

    Then use a real editor.



  • @morbiuswilters said:

    @bstorer said:

    Switching from C to C++ because you don't get warned for misusing sprintf is akin to flying coach because you don't like the brand of caviar they serve on your private jet.

    I presume you meant "flying coach on Air Venezuela".  Otherwise, you are far too kind to C++. 

    I considered putting "riding a Greyhound bus," but that was far too cruel to Greyhound.

Log in to reply