I spy demons coming out of your nose...



  • Behold (C, not C++, for those of you who are wondering):
    [code]
    /* These really are globals, yes. */
    extern unsigned int num_screens, default_screen;
    void do_some_X11_init_thingy(Display *dpy, /irrelevant args/)
    {
    char *ds, sp, nds[50];
    int scn;
    /
    Variable definitions and irrelevant code snipped.
    What follows is a bit summarized, but effective. /
    num_screens = ScreenCount(dpy) - 1;
    default_screen = DefaultScreen(dpy);
    for (scn = 0; scn <= num_screens; scn++)
    {
    /
    Yes, the original code is indented like this. /
    if (scn != default_screen)
    {
    /
    Much more irrelevant logic snipped. /
    ds = DisplayString(dpy);
    sp = strrchr(ds, '.');
    strncpy(nds, ds, sp-ds+1);
    nds[sp-ds+1] = 0;
    sprintf(nds, "%s%d", nds, scn);
    /
    Passes nds to some sort of opener-function, and does
    other things we don't care about /
    }
    }
    /
    Calls some other function */
    }
    [/code]


  • FoxDev

    I can see that there is something fishy here but my knowledge of C is... spotty at best.

    can someone help me pinpoint the point at which it is apparently legal for the compiler to cause demons to erupt from my nose?



  • That's a security-unconscious version of strcpy() right there



  • Nasty buffer overflow if :

    ds = "SomeDerpStringLongerThan50CharactersBecauseWhyWouldThisEverHappeneverAmIright.Foo";
    

  • FoxDev

    and this is why i'll stick to higher level languages that at least make it very difficult to buffer overflow.

    thank you very much!


  • Trolleybus Mechanic

    Unless you know that DisplayString is guaranteed to return a string shorter than that.


    Filed under: not enough data



  • Well, assuming DisplayString doesn't return null and there's a '.' in it and the substring is less than 50 chars... (a lot of buttumptions!)

    That sprintf is stupid.

    sprintf(nds + (sp - ds + 1), "%d", scn);
    

    And pray it fits.



  • @tufty said:

    That's a security-unconscious version of strcpy() right there

    @dcon said:

    Well, assuming DisplayString doesn't return null and there's a '.' in it and the substring is less than 50 chars... (a lot of buttumptions!)

    That sprintf is stupid.

    It's worse than you think considering how much work they did to not overrun the buffer in the past two lines...then they go and proceed to risk stomping all over it just to tack a number on the end?

    Filed under: creative sprintf implementations are a hazard to program functionality

    Also, the <hr> tag breaks any inline code below it. Yay, Discourse!



  • You'd still be better-off coding defensively.


  • Trolleybus Mechanic

    Certainly. I'm simply pointing out that when you're in control of the entire code-base (or at least the part that matters), you don't have to worry about things you've already taken care of elsewhere.

    Besides, it wouldn't be C if you couldn't just pull random garbage from the heap at will.



  • A quick check of the X11 C Language Reference and apparently they are trying to effectively do the C equivalent of the following C# code:

    ds = ds.Replace(".","");
    

    Filed Under: I don't always reinvent the wheel, but when I do, I make it square.



  • @GOG said:

    Unless you know that DisplayString is guaranteed to return a string shorter than that.

    @blakeyrat said:

    You'd still be better-off coding defensively.

    @GOG said:

    Certainly. I'm simply pointing out that when you're in control of the entire code-base (or at least the part that matters), you don't have to worry about things you've already taken care of elsewhere.

    Besides, it wouldn't be C if you couldn't just pull random garbage from the heap at will.

    Besides, they already appear to be coding defensively, just against the wrong thing. It's pointless to spend a bunch of time guarding against a buffer overflow when on the very next line, sprintf() is within its rights to party all over your buffer, the null terminator, and the rest of memory.



  • @MathNerdCNU said:

    A quick check of the X11 C Language Reference and apparently they are trying to effectively do the C equivalent of the following C# code:

    ds = ds.Replace(".","");
    

    Filed Under: I don't always reinvent the wheel, but when I do, I make it square.

    Actually, it's more like

    ds=ds.SubStr(0, ds.FindLastOf(".")).Append(scr.ToString());
    

  • :belt_onion:

    @tarunik said:

    sprintf() is within its rights to party all over your buffer, the null terminator, and the rest of memory.

    i don't know why, but this line makes me laugh so hard.


  • Discourse touched me in a no-no place

    @dcon said:

    That sprintf is stupid.

    It's beyond stupid, since it's writing into a string character array at same time as reading from it. And the code isn't defensive at all, since it doesn't actually check whether anything will fit in the buffer it's about to strncpy() into earlier in the function (let alone using the idea of allocating a buffer large enough; the cost of a malloc()/free() really won't matter if you're dealing with iterating over the displays). The whole thing is implemented by a lazy idiot.

    Fortunately, most systems only have one “screen” so this code is pointless. (Yes, they might have many actual screens, but they're usually arranged into a single virtual screen. The old X style of making them be separate logical surfaces is long obsolete.)



  • QFT.

    The fact that this "works" at all seems to be mostly dumb luck (i.e., if the format string started with anything other than %s, it'd probably break in all sorts of funny ways).


  • Discourse touched me in a no-no place

    @cvi said:

    The fact that this "works" at all seems to be mostly dumb luck (i.e., if the format string started with anything other than %s, it'd probably break in all sorts of funny ways).

    That's broken, but things are more badly broken (but less obviously so) before that.



  • Didn't spend too much time looking at the code - the sprintf() stood out for me. But you're right - there's much terribleness to be had there.

    In fact, shouldn't this code blow up on most systems by default? IIRC the string returned by DisplayString() would be :0, with no dots, which should be at the very least bad news when nds is manually zero-terminated.

    (FIxed lenght strings are of course bad, but seem to be pretty much standard in most code that messes with X11 that I've seen. 😦)



  • You are only in control WTHIN a given method (or in this case free function) - EVERYTHING outside of that needs to be verified. Today you may be the caller, but nobody knows who will call this routine next year.



  • Surely anything that that involves X is a WTF. OpenBSD historically (I don't know about today) you had to turn off explicitly particular OS configuration variables.


  • Trolleybus Mechanic

    Agreed. With two qualifications:

    1. "Being in control" has nothing to do with code scope and everything to do with who's writing the code. It's not very difficult to point out a situation where you might not even be in control within a given method (someone else might modify it tomorrow).

    2. When methods call methods, we're always dealing with a contract. It might be explicit - specified in the API - or implicit (as in the scenario I'd described). If your contract is with yourself, you can get away with a lot more than if you are dealing with a third-party library. Nevertheless, if the contract specifies an output within given bounds, it is reasonable to expect that it will be met - all the more so, if you're also responsible for the method being called fulfilling the contract.

    All I'm saying is that there's a very definite line between defensive programming and cargo-cult and I don't have enough information to judge this particular example.



  • A agree with the difference between defensive programming and cargo-cult. My observations are based on 40 years professional experience. From my perspective, if one modifies a function/method they are now the owner and have full responsibility.

    There is also the "contract is with yourself"....I have found that even in solo projects - full application of defensive techniques have major value, as there can be considerable time (and therefore change of who I am) involved in the lifetime of code that was originally intend as one-time-throw-way.



  • Oh hey TheCPUWizard is back. Whee.



  • Never left.


  • Trolleybus Mechanic

    Agreed.



  • @dkf said:

    And the code isn't defensive at all, since it doesn't actually check whether anything will fit in the buffer it's about to strncpy() into earlier in the function

    @cvi said:

    In fact, shouldn't this code blow up on most systems by default? IIRC the string returned by DisplayString() would be :0, with no dots, which should be at the very least bad news when nds is manually zero-terminated.

    Yes, thanks for pointing that out to me. What happens is that strrchr returns NULL because there are no dots in the display string, which causes the length passed to strncpy to be so far off base it isn't even funny (it in effect negates a pointer value). Result: the display string can go on and on, and it'll copy all of it, negating the use of strncpy altogether! Morons.

    Filed under: Is that shellcode in my DISPLAY?



  • @darkmatter said:

    sprintf() is within its rights to party all over your buffer, the null terminator, and the rest of memory.

    i don't know why, but this line makes me laugh so hard.

    The first time I heard this was in Raymond Chen's blog. Apparently, it's a popular phrase among Microsoft programmers.



  • @tarunik said:

    Yes, thanks for pointing that out to me. What happens is that strrchr returns NULL because there are no dots in the display string, which causes the length passed to strncpy to be so far off base it isn't even funny (it in effect negates a pointer value). Result: the display string can go on and on, and it'll copy all of it, negating the use of strncpy altogether! Morons.

    Actually, it seems to be a bit worse than that:

    man strncpy:
    
    If the length of src is less than n, strncpy() writes  additional null bytes to dest to ensure that a total of n bytes are written.
    

    I originally thought that strncpy would copy only until and including the \0 from the source string, assuming that the length parameter was large enough. Then I remembered that strncpy is particularly retarded and useless...



  • (quote from man page snipped since it was breaking the Discourse regex soup parser)
    @cvi said:

    I originally thought that strncpy would copy only until and including the \0 from the source string, assuming that the length parameter was large enough. Then I remembered that strncpy is particularly retarded and useless...

    QFT, also:

    1. This guarantees a crash if the DISPLAY string lacks a dot. How could they never have caught this?
    2. The OpenBSD folk invented strlcpy for a reason...

Log in to reply