• Debra (unregistered)

    (insert a witty regex with "frist" here)

  • (nodebb)

    Do we have a copy of the 2014 version backed up somewhere?

  • (nodebb)

    Honorable mention:

    • The "-i" flag is not portable; on some platforms (BSD) it requires an argument (backup suffix). So, this would never work on OS X (among others). But maybe the application is Linux-only.
    • The file path and file name is printed as it is, without any escaping or quoting. And passed to shell... which means that uses can have some Bobby Tables fun. Even without any malicious intent, spaces will break the application. But maybe the directory is fixed in configuration and the filename is generated...
  • (nodebb)

    The real WTF is to have to rely on an OS shell command to do text operations from inside a developed, compiled application. It's like building a car whose heater runs on a wood flame in a towed wheelbarrow.

  • Foo AKA Fooo (unregistered)

    "the pile of sed flags that precede it" -- it's worse. Only "-i" is a sed flag, the rest is a loop to read the whole file line by line. sed is a line-oriented language, and normally a sed program doesn't even see the newlines, so they actually have to concat each line to get at the newlines at all, which is, of course, very inefficient for larger files.

    The really sad thing is that this is at most the 4th worst WTF in these 2 lines, after not working at all (as the article says), allowing for shell injections (as Kamil Podlesak explains) and calling an external program for such a task from C (as Mr. TA states) ...

  • (nodebb)

    not to mention not checking the result of snprintf so if the filename is rather long, other pain will happen

  • Brian Boorman (google) in reply to Foo AKA Fooo

    To be fair, you don't know that there is a shell injection bug or not. I once had an application where sprintf was used to generate some user-displayable strings. Got flagged in code review, but.... this was an embedded application, whose compiler CRT didn't yet support snprintf, and there was no way for the user to enter any characters into the device (it was a display and keypad, keycodes went directly to the host, host sent binary pixel data to display).

    If the application controls all the string data and the user can't, there's no injection bug. Certainly it's a smell that should be investigated, but you can't say it's an actual bug.

  • (nodebb) in reply to Foo AKA Fooo

    If it's just for deleting the newlines, and you insist on using an external tool instead of writing the code, sed isn't the right tool anyway. A bit of "tr -d \n" would do it, and probably faster.

  • ooOOooGa (unregistered)

    I once had an amusing conversation with my manager and a coworker.

    M: So how are we doing on the latest project?

    Me: I have it written and it isn't causing compile errors.

    CW: It compiles. Ship it!

    We all laughed, because we all recognized instantly that it was a joke. Successfully compiling is not the bar that needs to be reached.

    I have to agree with Lowell. TRWTF is not the breaking change to the code. It is pushing the breaking change to production without even once checking to see if it successfully runs.

  • ooOOooGa (unregistered) in reply to Steve_The_Cynic

    The thing is, it doesn't just delete newlines. It only deletes some of the newlines. Specifically, only the newlines followed by a comma. Which may end up being all of them. Or at least all of them except possibly one at the very end of the file. It is not possible to say for sure.

    So maybe there is a slightly different tr command that would do the job the same way. Maybe deleting all of the newlines is correct. But that is two too many maybe in there for me to say for certain.

  • Foo AKA Fooo (unregistered) in reply to Steve_The_Cynic

    I'd thought of tr, but it wouldn't be exactly equivalent, since apparently they only want to remove newlines before "," (unless that's wrong too, which is quite possible).

    awk or perl could do it. Though they're generally more heavy than sed, at least they could operate line-by-line instead of reading the whole thing first.

  • Jonathan (unregistered)

    This seems like one of the things where even if you get the command right you're still doing wrong.

    If someone brought this to me my immediate reaction would be "Don't mix backing up files with munging text!". A backup should be a byte-for-byte copy. If you a tool that joins certain lines, that should be something separate.

  • (nodebb) in reply to ooOOooGa

    Bah, yeah. Ugh. Not enough coffee.

  • Duke of New York (unregistered)

    TRWTF is ignoring the result of the system call. I mean, it's just backups. Who cares if they were written correctly?

  • (nodebb)

    That's what she sed.

  • Loren Pechtel (unregistered) in reply to Jonathan

    *This seems like one of the things where even if you get the command right you're still doing wrong.

    If someone brought this to me my immediate reaction would be "Don't mix backing up files with munging text!". A backup should be a byte-for-byte copy. If you a tool that joins certain lines, that should be something separate.*

    I'm wondering if this was a fix for lines getting word-wrapped somehow. Perhaps on the boss' computer using something like Notepad with word wrap turned on... I've seen reasonably literate people fooled by that when looking at comma-separated files with long lines. It's certainly inappropriate in a backup.

    I don't think there is a meaningful injection issue here. This is a local app and %s almost certainly came from a configuration file. Anyone capable of putting something malicious in there would be capable of simply doing the malicious thing in the first place. Injection only matters when the code has privileges the user doesn't.

    I also don't think it's inherently a bad thing to use an external program here. We routinely use libraries, how is using a system utility all that different? Don't ignore the error, though! (Admittedly, it wouldn't be the easiest thing to do as the error is going to come back asynchronously.)

  • Foo AKA Fooo (unregistered) in reply to Loren Pechtel

    You're right for malicious injection, but there's also inadvertent injection such as a user innocuously putting a space in a file name. Many shell scripts are vulnerable to this, unfortunately, but you wouldn't expect a C program to be.

    Calling an external program is much more heavy than calling a library function, though it probably won't matter here since it only runs rarely. Involving a shell (as "system" does) means actually running two external programs, and relying on e.g. environment variables that influence the shell's behaviour etc. The error, however, comes back synchronously since "system" waits for the shell to finish.

  • xtal256 (unregistered) in reply to Jonathan

    I assumed that backup.txt was not the backed up file itself but rather a list of files to back up. Perhaps comma separated.

  • KalleC (unregistered) in reply to ooOOooGa

    We had a answer for questions about code quality: "It's double checked. It goes trough precompiler AND compiler without any errors."

  • Programmer Bot 1-32612 (unregistered)

    I'm guessing the bug was discovered by a customer who desperately needed to restore his zapped database. Perfect.

  • jay (unregistered)

    What is this "backup" idea of which you speak?

    I make backups from my home computer to a thumb driver regularly. And when I say "regularly", I mean that when I made a backup a couple of weeks ago, I noticed that the previous one was from 14 months before.

  • jay (unregistered) in reply to ooOOooGa

    "It compiles. Ship it!"

    Like I just said to my boss recently: Why do we bother testing? If something doesn't work, our customers will tell us.

  • Daniel (unregistered) in reply to jay

    The company I'm working once decided to push quality a bit with the following motto: "Do it right the first time". There were flyers distributed, e-mails sent, ... All with that motto as primary goal: Main thought was: Use a bit more to build it the first time and safe money by not doing lots of fixes later on (Not a bad thing by itself). Still we had a great many laughs about along the line of: "It will be cheaper since if we do it right the first time, there is no reason to invest in testing at all"

  • Haven't Written C++ For a While (unregistered)

    Haven't written C++ for a while, but isn't the snprintf invocation broken too? sizeof will get the size of the pointer, not the memory allocated in the buffer, so only the first 8 or so bytes would be copied to command.

  • Daniel (unregistered) in reply to Haven't Written C++ For a While

    Not necessarily: if you write something like

    char buffer[128]; int size = sizeof(buffer); char* bufferPtr = buffer; int sizePtr = sizeof(bufferPtr);

    size will be 128 (assuming a char size of 1). However sizePtr will the size of a pointer (8, 4, 2 or whatever the size of a pointer is on your specific platform) and not correspond with the size of the supplied data. This can be a problem as you don't see the difference at the sprintfn but only at the place the buffer is allocated (and since it's perfectly valid code you will only see that during runtime in form of memory corruption bugs).

  • Erk (unregistered)

    So, did they try:

    snprintf(command, sizeof(command),"sed -i ':a;N;$!ba;s/\n,/,/g' %s/%s.txt",path,file);

    Or am I missing something? (And yes, this should only fix the linefeed).

  • (nodebb) in reply to Daniel

    This can be a problem as you don't see the difference at the sprintfn but only at the place the buffer is allocated (and since it's perfectly valid code you will only see that during runtime in form of memory corruption bugs).

    It's unlikely that the buffer allocated for a sed command where the known static text is already more than eight characters is going to be less than eight characters in size itself, so the bug, will not cause memory corruption. If you only snprintf eight characters into a 128 byte array, there's no buffer overrun.

    The real WTF is that this is an example of cargo cult programming. The programmer was aware of the dangers of buffer overruns with string functions but they have been given the magic incantation of inserting an n in the appropriate place and they believe that this is the whole solution to the problem. There is no check made to ensure that the resulting string was not truncated. That might not be a serious issue in this case, but reflect on (for example) the difference between doing an operation (e.g. unlink on /etc/profile and on /etc/profile.d/some_file

Leave a comment on “He Sed What?”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article