C String Processing -- magnet link example

Question

I'm trying to get my head around C-string processing. I've decided to try my hand at making a dirt-simple magnet link generator. The format that a magnet link needs is

magnet:?xt=<hash>&tr=tracker1url&tr=tracker2url&....&tr=trackerNurl

This program takes a hash as an argument on the command line, and takes lines on standard input which are either empty lines or the URLs of trackers.

Am I doing this in a sane way? What can I do better? Please note that I'm fairly new to C, so if you see anything weird or dangerous, let me know about that as well.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#define SIZE 4096 

/*
 * Exit with the given code, and print an error
 * message if ec != 0
 */
void die(int ec, char *message) {
    if (ec) {
        fprintf(stderr, "ERROR: %s\n", message);
    }

    exit(ec);
}

/*
 * takes one input argument -- the hash code
 * of the thing you're looking to generate a
 * magnet link for, on the command line, and
 * then a list of trackers on the stdin stream.
 */
int main(int argc, char **argv) {
    // If there is no
    if (argc < 2)
        die(1, "No hash code given!");

    // buf is the internal buffer. bufEnd is supposed
    // to be a pointer to the very end of the string
    // to prevent linear time string concatenation.
    char buf[SIZE];
    char *bufEnd;

    // line is a buffer for each line of standard in
    char line[SIZE];

    // 20 is the number of chars in "magnet:?xt=..."
    // magic number is bad but i don't know a better way
    if (20 + strlen(argv[1]) >= SIZE)
        die(1, "buffer overflow detected");

    // Here we initialize buffer
    bufEnd = buf + snprintf(buf, SIZE, "magnet:?xt=urn:btih:%s", argv[1]);

    // keep reading input lines, adding "&tr=..." params
    // to the magnet link for non-empty lines
    while (fgets(line, SIZE, stdin)) {
        int len = strlen(line); // find out how big this line is

        // check for empty line. skip it if it's empty
        if (!(strcmp(line, "\n"))) continue;

        // check to see if there's room left in the array
        // and if so, then add the next arg.
        if (SIZE - (bufEnd - buf + len + 4) > 0) {
            bufEnd += sprintf(bufEnd, "&tr=%s", line) - 1;
        }
        else die(1, "buffer overflow detected");
    }

    // change the trailing newline into a nul byte
    if (bufEnd - buf < SIZE) {
       *bufEnd = '\0';
    }
    else die(2, "crazy buffer overflow detected");

    // output it with backwards brackets (to show whitespace...
    // debugging purposes)
    fprintf(stdout, ">%s<\n", buf);
    fflush(stdout);
    return EXIT_SUCCESS;
}

Answer 1

Your code is nicely written and you have checked for errors well. It is perhaps a little over-commented, many of the comments explaining the obvious (and hence being just 'noise').

String handling in C is messy and there are always many ways of achieving the same thing. There's nothing really wrong with using snprintf although it can be less efficient than using strcat and friends. Since you are using snprintf I'll concentrate on how you use it and ignore any alternatives. Someone else might like to comment on them.

You first append the command line parameter:

if (20 + strlen(argv[1]) >= SIZE)
    die(1, "buffer overflow detected");

bufEnd = buf + snprintf(buf, SIZE, "magnet:?xt=urn:btih:%s", argv[1]);

In this you have traversed argv[1] twice, once in the strlen call and then again in snprintf. You could avoid one of these:

const char *limit = buf + sizeof buf;
char *in = buf;

in += snprintf(in, limit-in, "magnet:?xt=urn:btih:%s", argv[1]);
if (in >= limit) {
    die(1, "buffer overflow detected");
}

This is more efficient and also avoids embedding the magic number 20 (embedded constants are usually best avoided). It uses the fact the snprintf returns the number of bytes needed for the string it wanted to print but doesn't overflow the buffer. So if it returns a number greater than or equal to the space available , you know there was an overflow. I also changed bufEnd to in, used limit to mark the end of the buffer and added braces around the call to die

Within your while loop you also traverse each string twice. The same technique as above can be used to avoid this. Also your check for blank lines does not trap lines that contains white-space as well as a \n. For that you might use:

const char *s = line + strspn(line, " \t\r\n");
if (*s == '\0')
    continue;