Isaac Dunham <ibid...@gmail.com> 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. _______________________________________________ Dng mailing list Dng@lists.dyne.org https://mailinglists.dyne.org/cgi-bin/mailman/listinfo/dng