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.

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.

And if efficiency isn't the prime consideration (and given the context,
it isn't), then an interpreted language is likely an even better choice.


Regards,
Roger