:: Re: [DNG] Systemd Shims
Top Page
Delete this message
Reply to this message
Author: Isaac Dunham
Date:  
To: Roger Leigh
CC: dng
Subject: Re: [DNG] Systemd Shims
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.