Isaac Dunham <ibid.ag@???> writes:
> On Wed, Aug 19, 2015 at 08:30:44PM +0000, Roger Leigh wrote:
>> On 19/08/2015 17:39, Rainer Weikusat wrote:
>>

>> >#define IFACE_TMPL \
>> >    "auto lo\n" \
>> >    "iface lo inet loopback\n\n" \
>> >    "iface wlan0 inet dhcp\n" \
>> >    "    wpa-ssid %s\n" \
>> >    "    wpa-psk \"%s\"\n"

>> >
>> >#define IFACES_PATH "/tmp"
>> >

>> >static void saveFile(char* essid, char* pw) //argv[1], argv[2]
>> >{
>> >    char *path;
>> >    FILE *fp;
>> >    unsigned p_len, e_len;

>> >

>> >    p_len = strlen(IFACES_PATH);
>> >    e_len = strlen(essid);
>> >    path = alloca(p_len + e_len + 2);

>> >    

>> >    strcpy(path, IFACES_PATH);
>> >    path[p_len] = '/';
>> >    strcpy(path + p_len + 1, essid);

>> >    

>> >    fp = fopen(path, "ab+");
>> >    fprintf(fp, IFACE_TMPL, essid, pw);
>> >    fclose(fp);
>> >}

>> >

>> >int main(int argc, char **argv)
>> >{
>> >    saveFile(argv[1], argv[2]);
>> >    return 0;
>> >}

>>
>> I'm not picking on this post in particular out of the rest of today's
>> thread, but I did think this was a good example. While I don't want to act
>> like a rabid C++ zealot, stuff like this really makes me shudder due to the
>> fragility and unnecessary complexity for something which is really trivial.
>>
>> While the relative safety and security of C string handling can be debated,
>> I do think the question needs asking: Why not use a language with proper
>> safe string handling and avoid the issue entirely? It's only "safe" until
>> it's refactored to break the existing assumptions and make it accidentally
>> unsafe. The constants such as 2, 1 plus the strlen() calls are prime
>> candidates for future bugs. It's not like this /needs/ to be done in C.
>
> It's not like it needs to be done this way in C, either. ;-)
>
> C provides snprintf, which looks like "printf to a memory buffer" but
> returns the number of bytes that it would output if there were enough space.
> You could thus do:
>
> // same includes and defines
>

> int main (int argc, char **argv)
> {
>     FILE *fp;
>     char *path;
>     ssize_t bytes;

>

>     if (argc < 3) {
>         printf( "usage: %s ESSID PASSWORD\n"
>             "writes an interfaces stanza to " 
>             IFACES_PATH "/ESSID\n", argv[0]);
>         exit(64);
>     }
>     bytes = snprintf(NULL, 0, IFACES_PATH "/%s", argv[1]);
>     path = malloc(bytes);
>     if (!path) {
>         perror(argv[0]);
>         exit(71);
>     }
>     // enough memory is guaranteed
>     snprintf(path, bytes, IFACES_PATH "/%s", argv[1]);

That's going to work with this particular problem which you incorrectly
(the original path wasn't a macro) reduced to appending a string of
unknown length to a constant string. Taking this into account, a
solution without snprintf would become something like

#define PATH "/tmp/"

char *p;

p = alloca(sizeof(PATH) + strlen(argv[1]));
sprintf(p, "%s%s", PATH, argv[1]);

or putting this into other terms: The snprintf buys you exactly
nothing. And you could have used asprintf to begin with. This would even
address what was considered to be the issue, namely, that memory
management and memory use are separate functions and that the
correctness of the latter depends on the correctness of the former via
implicit semantic constraints a compiler cannot check, something the
snprintf-code exhibits as well as it is still composed of the three
steps

1. Calculate the required length based on the input data.
2. Allocate a buffer of a sufficient size.
3. Copy the input data into this buffer.

Just in a somewhat less obvious way.

Considering code I have had the mispleasure to deal with, I consider the
double snprintf a disaster waiting to happen. Sooner or later, some
copy'n'paste rough rider will change the second call to

snprintf(path, bytes, IFACES_PATH "/%/%.%", argv[1], argv[2], argv[3]):

or even

// snprintf doesn't work
sprintf(path, IFACES_PATH "/%/%.%", argv[1], argv[2], argv[3]);

(as 'quick bug fix' of the first change) and likely won't even look at
the other code.