Application for converting markdown to PDF

Question

First time writing C in a long time and would like some code review to try and improve my code. I have some specific questions, but first an introduction to the code.

The application is a simple command line application for converting markdown to pdf. The source code can be found in the repository on Github. The main logic is in main.c, and is hopefully pretty straight forward.

My first question is, have I made any common C errors, like buffer overflows or unreleased memory? I don't think so, I don't use any malloc calls in my code, but would like someone more experienced to take a look.

I'm compiling the application and dependencies statically, how common is this? My main reason for this is that one of the dependencies (wkhtmltopdf) is a bit tricky to compile, so I want to be able to provide a binary release that doesn't depend on a shared library.

I have added the dependencies to the repository, not what I'm used to from Ruby or Go, but it seems like the most straight forward way. One other solution I tried was to download the dependencies using git in the Makefile, like this. So they did not have to be added, but that did not feel like a good C solution. Feels a bit like a "roll you own" solution. How would you do this?

Is there anything in the Makefile I could do to make it easier to build for various package managers? E.g. apt-get, homebrew, ports, pkg?

Are there anything else I should do to be a good *nix citizen? E.g. the man page.

This block in main.c feels a bit clunky, and I would like to extract it to a couple of methods instead, to get a better flow when reading the main application logic. But I could not find a good way to do so. The logical parts are read_markdown, create_renderer, generate_filename and write_html. But the code seems to be doing to much for a simple extract method. Do you have any suggestions?

/* Read SOURCE(s) */

hoedown_buffer *ib = hoedown_buffer_new(HOEDOWN_IUNIT);
if (n_files == 1) {
    int error_code = read_file_to_buffer(ib, in);
    if (error_code > 0) {
        fprintf(stderr, "I/O errors found while reading input.\n");
        hoedown_buffer_free(ib);
        fclose(in);
        exit(error_code);
    }
} else {
    int i;
    for (i = 0; i < n_files-1; i++) {
        FILE *ir = fopen(file[i], "r");
        int error_code = read_file_to_buffer(ib, ir);
        if (error_code > 0) {
            fprintf(stderr, "I/O errors found while reading input.\n");
            hoedown_buffer_free(ib);
            fclose(ir);
            exit(error_code);
        }
        if (page_break_between_sources) {
            if (i < n_files-1) {
                hoedown_buffer_puts(ib, "\n\n<div class=\"page-break\"></div>\n\n");
            }
        }
        fclose(ir);
    }
}

hoedown_renderer *renderer = hoedown_html_renderer_new(0, 0);
hoedown_document *document = hoedown_document_new(renderer, HOEDOWN_EXTENSIONS, HOEDOWN_MAX_NESTING);

hoedown_buffer *ob = hoedown_buffer_new(HOEDOWN_OUNIT);
hoedown_document_render(document, ob, ib->data, ib->size);
hoedown_buffer_free(ib);
fclose(in);

/* Generate a tmpfile for html output */
char nameBuff[23];
char filePath[30];
int filedesc = -1;
memset(nameBuff, 0, sizeof(nameBuff));
memset(filePath, 0, sizeof(filePath));

strncpy(nameBuff, "/tmp/mdpdf-XXXXXX.html", 22);

filedesc = mkstemps(nameBuff, 5);
FILE *out = fdopen(filedesc, "w");

/* Use tmp file to build a file:// path for wkhtml */
sprintf(filePath, "file://%s", nameBuff);

/* Write tmp html output to file */
write_html_to_file(out, ob, verbose, stylesheet);
fclose(out);

/* Cleanup hoedown */
hoedown_buffer_free(ob);
hoedown_document_free(document);
hoedown_html_renderer_free(renderer);

Answer 1

Packaging

Compiling statically is tricky. I understand your reasoning, but if you want e.g. to get your program into a distribution, having a proper build with a separate package for each library is preferable (even necessary?). If you provide a compiled distribution yourself, your users won't object I guess.

For dependencies, I usually expect no bundled libraries in small programs, larger ones typically include them in addition to being able to use the system-wide installed one as well. Including it directly in the repository puts a burden on you to keep up-to-date with the upstream version, so I'd personally go for the "you have to download and install dependencies yourself" route. If it's tricky for one library, could you patch it and provide a fork yourself, or get the maintainers to fix it?

Love the man page.

For building, I think you should read up on that at the distribution sites. I can only speak for Gentoo, where you'd generally be okay if your Makefile is sufficiently clean to be fixed by the installation (so e.g. you choice of -03 would most certainly be overridden. Frankly the Makefile looks okay though. Maybe look at pkgconfig for the Qt dependencies.

Code

Can you generate html_data.h from source files during the build? That would be cleaner and easier to handle with separate files (editing and checking).

mdpdf_usage should just be a single printf, really, just don't worry about the cost, it'll look cleaner.

You're using false in some places, I'd suggest using it consistently for other return codes as well (when possible). I.e. read_file_to_buffer should do that (what does 5 as return value even mean).

The fputs copy loop in write_html_to_file isn't efficient. The (void)s before the fwrite calls are out of place I think, you don't check the return value for fprintf either. I'd also split that function into separate pieces and call it (or a sub-function) twice if verbose is set, less duplication of code that way.

You could check if you can get rid of non-essential fclose calls and frees on exit, since everything will be closed/reclaimed anyway and it will take more time to do it this way.

The memset calls on nameBuff and filePath are not needed I think.

Can't you just run the loop with the files even with just one file? That gets rid if some nesting. For the spliting of main, well, just do the logical split you already wrote and pass everything necessary between them. If you have to many arguments, create a my_context structure to keep that state (not globally!) and then you have nice logical separation into functions.

The read sizes are too small. Page size is likely 4k, so use that for reading into buffers (HOEDOWN_IUNIT) unless you have a reason I can't see. Similarly, HOEDOWN_OUNIT is just 64 bytes? That seems again very small.

Everything else seems largely a stylistic choice (declaring variables at the top, tabs, indentation), so IMO it looks rather good in general.

Answer 2

Answers to your specific questions:

• My first question is, have I made any common C errors, like buffer overflows or unreleased memory?

  A leak is always connected to a resource. A resource is by definition something that you acquire manually, and that you must release manually. Memory is a prime example, but there are other resources, too (file handles, mutex locks, network connections, etc.).

  A leak occurs when you acquire a resource, but subsequently lose the handle to the resource so that nobody can release it. A lesser version of a leak is a "still-reachable" kind of situation where you don't release the resource, but you still have the handle and could release it. That's mostly down to laziness, but a leak by contrast is always a programming error.

  I don't see any leaks in your code, but I'm only using my half-human side to analyze that. I would use a more thorough tool such as Valgrind to provide more substantive confirmation.

• I'm compiling the application and dependencies statically, how common is this? My main reason for this is that one of the dependencies (wkhtmltopdf) is a bit tricky to compile, so I want to be able to provide a binary release that doesn't depend on a shared library.

  I have added the dependencies to the repository, not what I'm used to from Ruby or Go, but it seems like the most straight forward way. One other solution I tried was to download the dependencies using git in the Makefile, like this. So they did not have to be added, but that did not feel like a good C solution. Feels a bit like a "roll you own" solution. How would you do this?

  I had a project that was somewhat similar to yours in this aspect. As long as all those dependencies are available, then there is no problem

  The problem for my project to compile was that I couldn't guarantee that. I also didn't want the user scrounging around trying to figure out how to get my application to simply compile. Here's what I did, I used CMake to handle all of that. It took a bit of work, but here's the jist of what it does: it takes all of the dependencies that your application relies on and searches the host computer for them. If it finds them, it stores that location for it's use later when compiling. If it can't find them, then it manually downloads them and performs it's own build of the dependency which it will then use later for compiling. The real "fun" part comes in where you dependency has sub-dependencies, which I also had to deal with...

  It's a lot more work, but also a whole lot more portable and less stressful for the end user (which should be the ultimate goal really for all developers, making the usage of your application as hassle-free as possible for your consumers).

• Is there anything in the Makefile I could do to make it easier to build for various package managers? E.g. apt-get, homebrew, ports, pkg?

  Another reason to use CMake or another similar build system, so that it generates a Makefile for you based on the system it's running on.

• Are there anything else I should do to be a good *nix citizen? E.g. the man page.

  man pages are nice. Besides, how else are you supposed to tell your users to RTFM?

  

---

A few comments on the code posted in the question (hopefully not conflicting with @ferada):

• I notice some suspiciously similar code in a few places:

  if (error_code > 0) {
      fprintf(stderr, "I/O errors found while reading input.\n");
      hoedown_buffer_free(ib);
      fclose(in);
      exit(error_code);
  }
  

  The stuff within the conditional could be extracted to a function so that reuse would be possible, and would help your code follow the DRY principles. This should also shorten your code a bit

• You should be declaring your counter variable (int i in this case, within your for loops), so that it is in the smallest scope possible.^(C99)

• You don't have to memset your char arrays to 0, just initialize them that way.

  char nameBuff[23] = "";
  

  Or the more "universal" way to zero out an array (since it works with every type):

  char nameBuff[23] = {};
  

• Nice job generating a temporary file safely! I have one big problem with it though, and it has to do with this line

  strncpy(nameBuff, "/tmp/mdpdf-XXXXXX.html", 22);
  

  See that /tmp/ part? That isn't guaranteed to be the temporary file storage location for every Unix system, making your code a lot less portable. For example, the location my system prefers to point to is /var/folders/xp/j1l6q2z12n54k9y4r7574l0w0000gn/T/. Here's how I solved the problem, abstracted away in a function:

  const char* getTmpDir(void)
  {
      char *tmpdir = NULL;
      if ((tmpdir = getenv("TEMP"))) return tmpdir;
      else if ((tmpdir = getenv("TMP"))) return tmpdir;
      else if ((tmpdir = getenv("TMPDIR"))) return tmpdir;
      else return "/tmp/";
  }
  

  To which I created a file like such:

  // Creates temporary file safely
  char flacFile[FILENAME_MAX] = "";
  const char *fileRoot = getTmpDir();
  snprintf(flacFile, FILENAME_MAX, "%sXXXXXXXXXXXXX.flac", fileRoot);
  mkstemps(flacFile, 5); // the 5 is for the length of the suffix ".flac"
  

  Since I can never actually know the length of fileRoot for my static array creation, I just used the maximum length that a filename could be, very helpfully stored in the predefined macro FILENAME_MAX.