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]);
fp = fopen(path, "ab+");
free(path);
if (!fp) {
perror(argv[0]);
exit(73);
}
fprintf(fp, IFACE_TMPL, argv[1], argv[2]);
fflush(fp);
if (errno || ferror(fp)) {
perror("error writing file");
fclose(fp);
exit(73);
}
exit(0);
}
Now, yes, that's lengthy.
(I really do write things that way, though with some helpers..)
But a lot of that could easily be put into functions:
-the snprintf() trick could become its own function; it's borrowed from
toybox's "xmprintf()".
-the perror(); exit(); code blocks could become a single function; toybox
uses xmalloc()/xopen()/xwrite()/... and perror_exit().
Do that, and you can make it about as long as your saveFile().
But when it's all said and done, I can write something that I can compile
into a static binary with a generic cross-toolchain and drop on a WRT54G.
> void saveFile(const std::string& essid, const std::string& pw)
> {
> std::string path(IFACES_PATH);
> path += '/';
> path += essid;
>
> std::ofstream fp(path);
> if (fp)
> {
> boost::format fmt(IFACE_TMPL);
> fmt % essid % pw;
> fp << fmt.str() << std::flush;
> }
> }
>
> No leaks. No buffer overflows. Safe formatting. No file handle leaks.
> And it's readable--the intent is obvious since there's no extraneous buffer
> memory management. And it will compile down to something equivalent or even
> more efficient.
>
> If you use std::string you can still work directly with C functions using
> "const char *"--just use the .c_str() method and you get a suitable pointer.
>
> In my own code I use boost.filesystem, so rather than using "std::string
> path" you could then do
>
> path p = IFACES_PATH / essid;
>
> and have the path concatentation handled directly, and then use p.string()
> to get a string back. Even safer and more maintainable--work with path
> components directly rather than mangling strings.
>
> void saveFile(const std::string& essid, const std::string& pw)
> {
> path p = IFACES_PATH / essid;
> std::ofstream fp(p.string();
> if (fp)
> {
> boost::format fmt(IFACE_TMPL);
> fmt % essid % pw;
> fp << fmt.str() << std::flush;
> }
> }
>
> This is obviously easier and faster to write and maintain, so your energies
> are spent productively on the problem at hand, rather than faffing around
> with manual buffer management.
_______________________________________________
Dng mailing list
[email protected]
https://mailinglists.dyne.org/cgi-bin/mailman/listinfo/dng
