Thread: Sprintf overflows my buffer -- why?

It is (probably) guaranteed to be at least 32767.

Definitely.

char call[sizeof(ident_a) + strlen("security lock-keychain .keychain >& /dev/null")];

Illegal. Size is not constant.
However, you could do:

char call[sizeof(ident_a) + sizeof("security lock-keychain .keychain >& /dev/null")];

The problem is that the buffer is not large enough to hold your entire string. It is only large enough to hold "security lock-keychain %s.keychain >& /dev/null" minus the "%s".

Originally Posted by Lasston

1) If mkruk's idea is illegal, why does it work perfectly earlier in the code? I use

Code:

char rstring[11+strlen("dark__fun")];
snprintf(rstring, sizeof(rstring), "dark_%d_fun", (rand()*rand()));

with no problems whatsoever.

It's true that it compiles, but that's because it's a compiler extension. In the standard, it's illegal.

1) If mkruk's idea is illegal, why does it work perfectly earlier in the code?

Well... it is legal in C99 as it uses the variable length array feature. I am not sure if you really want to use that though (and honestly I am not familiar with it myself).

I am trying to learn standard, unextended C right now, so I guess I'll be taking out the array.

C99 is the 1999 edition of the C Standard. It so happens that the variable length array feature is a C++ extension for the g++ compiler, but that is not relevant here.

So... is there just no way to do something like this in the C language? It seems like no matter what I do I get an overflow.

You can use dynamic memory allocation with malloc() and free() from <stdlib.h>.

Originally Posted by Lasston

So... is there just no way to do something like this in the C language? It seems like no matter what I do I get an overflow.

Aside from Laserlights suggestion, there is always the option of "oversize" - if you KNOW that your sprintf() will not produce more than a few hundred bytes, make your string 4096 or something "ridiculuously large".

Naturally, this doesn't work if you have completely and utterly arbitrary inputs - in that case, you will have to find a way to calculate how much space it takes and then use malloc/free.

--
Mats

>1) If mkruk's idea is illegal, why does it work perfectly earlier in the code?

Code:

>char call[sizeof(ident_a) + strlen("security lock-keychain .keychain >& /dev/null")];
>snprintf(call, sizeof(call), "security lock-keychain %s.keychain >& /dev/null", keychain);
>int results=system(call);

Make this:

Code:

char call[1 + strlen(keychain) + strlen("security lock-keychain .keychain >& /dev/null")];
snprintf(call, sizeof(call), "security lock-keychain %s.keychain >& /dev/null", keychain);
int results=system(call);

That's still C99.

strlen() is a function. Its return value can only be evaluated at runtime. Consequentally, the compiler doesn't know how big the array is going to be, and you're using variable length arrays.

To solve this problem, the first thing you need to do is figure out how many digits rand() can return. One way to do that is to look at RAND_MAX and count its digits.

Code:

int digits_in_number(int n) {
    int digits = 0;

    while(n) {
        digits ++;
        n /= 10;
    }

    return digits;
}

digits_in_number(RAND_MAX);

Of course, you might be even better off passing the actual random number to a function like that, to get the exact amount of memory that you need.

Note that I've created a function to figure out how much memory to allocate, so again, you'll either have to use VLAs (variable length arrays) or dynamic memory allocation. I suggest memory allocation. It's not too hard.

Code:

char *call = malloc(digits_in_number(ident_a)
    + strlen("security lock-keychain .keychain >& /dev/null") + 1);  /* +1 for the NULL */
int result;
sprintf(call, "security lock-keychain %s.keychain >& /dev/null", keychain);
result = system(call);
free(call);

A few notes about my version:

• I'm using sprintf() here, instead of snprintf(). The latter is C99, so if you're using it you might as well use variable length arrays anyway.
• I'm declaring result at the beginning of a block (right after an opening curly brace), instead of the middle of the block. The latter, as you had it, is also C99.
• I'm adding one to the length of the string, to accomodate the NULL terminator.

I'm not sure why you're adding the size of ident_a to the string, though, because you're not using ident_a anywhere. Perhaps it should be passed to the sprintf() someplace?

Anyway . . . hopefully that helped a little bit . . . .

>I'm not sure why you're adding the size of ident_a to the string, though
That's supposed to be keychain. That's the only change I made from his original code, plus adding one for the string terminator.