Home > Coding > Spot the flaws

Spot the flaws


void someFunction(char* inputStr)
{
char buffer[8];

int n = snprintf(buffer, sizeof(buffer) - 1, inputStr);
buffer[n] = 0;

/* ... */

And while you’re at it – see if you can spot the motivations behind what’s being done.

 

Categories: Coding Tags: , ,
  1. Victarus
    June 15, 2012 at 3:44 pm | #1

    Using the string as the format string: One percentage sign or backslash spells potential bad juju.

    I *think* snprintf is the common-but-not-actually-standard safe printf (I’m pretty sure there’s one printf variant that’s used all over the place but whose return value is different depending on the implementation), but I might be mistaken on that. Either way, the first source I found says “returns the number of characters that would have been written had size been sufficiently large, not counting the terminating nul character”, so it’s a problem either way.

    The null terminator would be at the end of the string anyway. It wouldn’t really be much of a safe printf otherwise, would it?

  2. Victarus
    June 15, 2012 at 3:46 pm | #2

    NOTE: In an environment that isn’t responding to a blog post, I reserve the right to poke at the problem for a while to make sure I’m not making a stupid assumption, so don’t hold it against me if I got something wrong. :p

  3. June 15, 2012 at 4:29 pm | #3

    You got the bulk of the problems there – if you know there’s no masks in the source string, use strncpy or friends instead, per the one percentage thing :)

    Modern snprintfs return the length of the /source/ string, but in short, snprintf isn’t guaranteed to return a value between 0 and the size of your buffer. Infact, in the majority of cases it’s going to guarantee to give you an offset bigger than your index (e.g. if inputStr is 10 bytes long, it would return 10 under GCC).

    snprintf is the buffer-overflow protecting version of snprintf. The second argument factors in the need for a null byte. So, sizeof(buffer) would be correct, this version is forcing a shorter buffer size.

    Lastly, the buffer[n] is just bad practice. The author is trying to protect against a perception that snprintf might not write the final null when the buffer fills. In that case, its the last byte of buffer you want to write to, not some return value from another function that may change at some point in the future. C.f. what if snprintf returns -1 to indicate some kind of error?

    char buffer[8] ;
    snprintf(buffer, sizeof(buffer), "%s", inputStr) ;
    // Incase our snprintf doesn't back-fill the
    // end of buffer with a zero when the buffer fills.
    // This is where the original author got his
    // sizeof(buffer) - 1 from.
    buffer[sizeof(buffer)-1] = 0;
    
  1. No trackbacks yet.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

Follow

Get every new post delivered to your Inbox.

Join 183 other followers

%d bloggers like this: