4
\$\begingroup\$

I developed a basic template for beginning a C program. It attempts to implement the following:

  1. Memory allocation and error handling
  2. Message logging
  3. Safer string copying
  4. Safer string concatenation
  5. Test framework
  6. Error handling

How does it look?

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

/*Macros for constants*/
#define INIT_BUF_SIZE 10000
#define MAX_INT_STR_LEN 20

/*Function-like macros*/
#define MALLOC(newbuf, bufsize, logbuf) if ((rc = str_malloc(newbuf, bufsize, \
        logbuf)) != SUCCESS) return rc
#define CAT(dest, source, logbuf) if ((rc = str_cat(dest, source, \
        logbuf)) != SUCCESS) return rc
#define TESTCASE(func, logbuf, testcount) if ((rc = func(logbuf)) != SUCCESS) \
                                                    return rc; testcount++
/*Return codes*/
typedef enum {
  SUCCESS,
  ERR_STR_COPY,
  ERR_STR_CAT,
  ERR_STR_MALLOC,
  ERR_RC_NOT_SET } td_rc;

/*Dynamically allocated string with allocated size */
struct strbuff{
  size_t size;
  char *str;
};

/*Function prototypes*/

/*Convenience wrapper around malloc with optional log parameter*/
td_rc str_malloc(struct strbuff *newbuf, size_t bufsize, 
    struct strbuff *logbuf);
/*Safer replacement for strcpy with optional log parameter*/
td_rc str_copy(struct strbuff *dest, char *source, struct strbuff *logbuf);
/*Safer replacement for strcat with optional log parameter*/
td_rc str_cat(struct strbuff *dest, char *source, struct strbuff *logbuf);
/*Test runner*/
td_rc test_all(struct strbuff *logbuf);
/*Test cases*/
td_rc test_str_malloc(struct strbuff *logbuf);
td_rc test_str_copy(struct strbuff *logbuf);
td_rc test_str_cat(struct strbuff *logbuf);

/*Output HTTP header and log messages*/
int main(int argc, char** argv){
  td_rc rc = ERR_RC_NOT_SET;
  struct strbuff logbuf;
  MALLOC(&logbuf, INIT_BUF_SIZE, NULL);
  logbuf.size = INIT_BUF_SIZE;
  if ((rc = str_copy(&logbuf, "content-type: text/html\n\n", NULL)) 
      == SUCCESS) {
    if (argc > 1)
      if (strcmp(argv[1], "test") == 0)
        /*If the first parameter is "test", run all test cases*/
        rc = test_all(&logbuf);
    printf("%s", logbuf.str);
  }
  free(logbuf.str);
  return rc;
}

td_rc str_malloc(struct strbuff *newbuf, size_t bufsize, 
    struct strbuff *logbuf){
  td_rc rc = ERR_RC_NOT_SET;
  newbuf->str = malloc(sizeof *newbuf->str * bufsize);
  if (!newbuf->str) {
    /*If malloc fails, write error to log if supplied
      otherwise write to standard output*/
    char errmes[100];
    sprintf(errmes, "malloc failed : size requested %lu \n", 
        (unsigned long)bufsize); 
    if (!logbuf)
      printf("%s", errmes);
    else CAT(logbuf, errmes, NULL);
    return ERR_STR_MALLOC;
  }
  /*Terminate the new empty string*/
  *newbuf->str = '\0';
  newbuf->size = bufsize;
  return SUCCESS;
} 

td_rc str_copy(struct strbuff *dest, char *source, struct strbuff *logbuf){
  size_t sourcelen = strlen(source);
  td_rc rc = ERR_RC_NOT_SET;
  /**dest should have been initialized with str_malloc*/
  if (sourcelen > dest->size) {
    /*If if the source string is longer than the destion buffer, 
     * write error to log if supplied otherwise write to standard output*/
    char errmes[100];
    sprintf(errmes, "string copy failed : source length %lu > \
        dest allocated space %lu\n", (unsigned long)sourcelen, \
        (unsigned long)dest->size); 
    if (!logbuf)
      printf("%s", errmes);
    else CAT(logbuf, errmes, NULL);
    return ERR_STR_COPY;
  }
  memcpy(dest->str, source, sourcelen+1);
  return SUCCESS;
} 

td_rc str_cat(struct strbuff *dest, char *source, struct strbuff *logbuf){
  td_rc rc = ERR_RC_NOT_SET;
  /**dest should have been initialized with str_malloc*/
  size_t destlen = strlen(dest->str);
  size_t destfree = dest->size - destlen;
  size_t sourcelen = strlen(source);
  if (sourcelen > destfree) {
    /*If the source string is longer than the free space in the 
     * destination buffer, write error to log if supplied otherwise write 
     * to standard output*/
    char errmes[100];
    sprintf(errmes, "string concatenation failed : source length %lu > \
        dest free space %lu\n", (unsigned long)sourcelen, \
        (unsigned long)destfree); 
    rc = ERR_STR_CAT;
    if (!logbuf)
      printf("%s", errmes);
    else 
      rc = str_cat(logbuf, errmes, NULL);
  } else { 
    memcpy(dest->str+destlen, source, sourcelen+1);
    rc = SUCCESS;
  }
  return rc;
} 

td_rc test_all(struct strbuff *logbuf){
  td_rc rc = ERR_RC_NOT_SET;
  int testcount = 0;
  char rc_str[MAX_INT_STR_LEN];
  /*If a test case fails, stop further processing and output the log, 
   * otherwise output a test summary*/
  TESTCASE(test_str_malloc, logbuf, testcount);
  TESTCASE(test_str_copy, logbuf, testcount);
  TESTCASE(test_str_cat, logbuf, testcount);
  sprintf(rc_str, "%d", testcount);
  CAT(logbuf, "\n", NULL);
  CAT(logbuf, rc_str, NULL);
  CAT(logbuf, " tests completed successfully\n", NULL);
  return SUCCESS;
}

td_rc test_str_malloc(struct strbuff* logbuf){
  td_rc rc = ERR_RC_NOT_SET;
  struct strbuff teststr;
  MALLOC(&teststr, 5, logbuf);
  if (teststr.size == 5)
    rc = SUCCESS;
  else rc = ERR_STR_MALLOC;
  free(teststr.str);
  return rc;
}

td_rc test_str_copy(struct strbuff* logbuf){
  td_rc rc = ERR_RC_NOT_SET;
  struct strbuff teststr;
  MALLOC(&teststr, 7, logbuf);
  if ((rc = str_copy(&teststr, "foo", logbuf)) == SUCCESS) {
    if (strcmp(teststr.str, "foo")) 
      rc = ERR_STR_COPY;
    else {
      if ((rc = str_copy(&teststr, "bar", logbuf)) == SUCCESS) {
        if (strcmp(teststr.str, "bar")) 
          rc = ERR_STR_COPY;
      }
    }
  }
  free(teststr.str);
  return rc;
}

td_rc test_str_cat(struct strbuff* logbuf){
  td_rc rc = ERR_RC_NOT_SET;
  struct strbuff teststr;
  MALLOC(&teststr, 7, logbuf);
  if ((rc = str_cat(&teststr, "foo", logbuf)) == SUCCESS) {
    if (strcmp(teststr.str, "foo")) 
      rc = ERR_STR_CAT;
    else {
      if ((rc = str_cat(&teststr, "bar", logbuf)) == SUCCESS) {
        if (strcmp(teststr.str, "foobar")) 
          rc = ERR_STR_CAT;
      }
    }
  }
  free(teststr.str);
  return rc;
}
\$\endgroup\$
  • \$\begingroup\$ Are you limiting code to pre-C99? \$\endgroup\$ – chux - Reinstate Monica Jan 30 '18 at 21:47
  • \$\begingroup\$ @chux, I want the template to be as portable as possible and I understood the ANSI C runtime library was the lingua franca provided by most systems, OSes, devices, etc. Having said that, if other runtime library functions such as snprintf are effectively universally available, I could use them too. \$\endgroup\$ – user2309803 Jan 31 '18 at 5:24
  • \$\begingroup\$ Consider there is no version that is a universal subset of C - C evolves. Any sub-language selection will carry pros and cons. I recommend a C99 target with maybe some limitations. See MS Visual Studio C99 support. \$\endgroup\$ – chux - Reinstate Monica Jan 31 '18 at 5:30
8
\$\begingroup\$

I would say that you've got a case of "solution in search of a problem" here — and your solutions are mostly faulty!


#define MALLOC(newbuf, bufsize, logbuf) if ((rc = str_malloc(newbuf, bufsize, \
        logbuf)) != SUCCESS) return rc

Look up "hygienic macros." The problem with a macro definition like this one here is that it'll do the wrong thing if the programmer writes

if (useSmallBuffer)
  MALLOC(mybuf, 100, logbuf);
else
  MALLOC(mybuf, 200, logbuf);

(Expand it out. Do you see why the control flow goes wrong?)

Whenever you write a "statement-like" macro in C or C++, you must wrap it in do { ... } while (0) for hygiene.

Also, consider what happens with

int ra = 1, rb = 2, rc = 3;
MALLOC(mybuf, 3, logbuf);
mybuf[0] = ra;
mybuf[1] = rb;
mybuf[2] = rc;

Macros generally should not take "hidden" or "implicit" parameters.


/*Macros for constants*/
#define INIT_BUF_SIZE 10000
#define MAX_INT_STR_LEN 20

These macros' names are unnecessarily cryptic, and their purpose is obscure. Why do you need them at all?

The only place you use INIT_BUF_SIZE is in main

MALLOC(&logbuf, INIT_BUF_SIZE, NULL);
logbuf.size = INIT_BUF_SIZE;

— which means that at least its scope should be restricted to main.

const size_t initial_logbuf_size = 10000;
MALLOC(&logbuf, initial_logbuf_size, NULL);
logbuf.size = initial_logbuf_size;

The next thing we notice about this snippet is that we're setting logbuf.size = K immediately after a call to MALLOC(&logbuf, k, ...). This smells awfully like we've trying to preserve some invariant here — such as, "foo.size always contains the malloc'ed length of the buffer foo." We should move the code that preserves that invariant into the MALLOC macro.

#define MALLOC(newbuf, bufsize, logbuf) \
do { \
    int rc = str_malloc(newbuf, bufsize, logbuf); \
    if (rc != SUCCESS) return rc; \
    (newbuf)->size = (bufsize); \
} while (0)

Notice that I placed parentheses around the macro arguments (newbuf) and (bufsize) when I used them inside the macro itself.


Working on that macro, something caught my eye. We now have some logic ((newbuf)->size = (bufsize);) placed after the early return statement. And we're using raw pointers (instead of something like C++'s unique_ptr<T>) to manage our memory. This smells like a recipe for a memory leak! But of course the same smell was there before this change; it was just masked by the unnecessary complexity of the code. Consider:

void foo() {
    int rc = 0;
    struct strbuff buffer1, buffer2;
    MALLOC(&buffer1, 1000, NULL);  // possible early return
    MALLOC(&buffer2, 1000, NULL);  // possible early return
    free(buffer1.str);
    free(buffer2.str);
}

If the underlying malloc of buffer2 fails and returns NULL, then the buffer pointed to by buffer1 will be leaked!

To deal with the freeing of resources in case of early return, C programmers usually use a pattern like goto cleanup; — see for example this SO question or Apple's famous "goto fail" bug.


Any time I'm looking at C code, I look specifically at how they handle their buffers: Are they correctly passing around buffer lengths to avoid buffer overflows? This doesn't necessarily mean using silly helper functions like strcpy_s; it just means not doing anything terribly dumb.

So when I see sprintf, in code that is specifically intended to help protect against buffer overflows, it sets off alert bells:

char errmes[100];
sprintf(errmes, "string concatenation failed : source length %lu > \
    dest free space %lu\n", (unsigned long)sourcelen, \
    (unsigned long)destfree); 

Now, at first glance, the longest possible string we could be printing into the buffer errmes here is "string concatenation failed : source length 4000000000 > dest free space 4000000000\n", which is only 84 characters long. So that's cool, right? We've got 15 bytes of "fudge" left over! No problem! We therefore resist the angel on our shoulder pleading with us to use the correct buffer-overflow-proof construction,

char errmes[100];
snprintf(errmes, sizeof errbuf, "string concatenation failed : source length %lu > \
    dest free space %lu\n", (unsigned long)sourcelen, \
    (unsigned long)destfree); 

Go away, angel!

But "fudge" has a way of being wrong, or at least fragile. Suppose a year from now you went back and changed this code to

char errmes[100];
sprintf(errmes, "WARNING: string concatenation failed : source length %lu > \
    dest free space %lu\n", (unsigned long)sourcelen, \
    (unsigned long)destfree); 

Here we've added 9 bytes to the string: "WARNING: " (don't forget that extra space we added!). We add it all up again and determine that the maximum length of the string is now 84+9 = 93 bytes. No problem.

Except that where there's one bug, there's usually two. Bugs are caused by carelessness, and carelessness isn't a one-time occurrence. Look again at the line above. How many characters are really in that string?

Did you spot the trailing backslash that marks a line continuation? The format string in your original code actually looks like this:

"string concatenation failed : source length %lu >         dest free space %lu\n"

It's 8 bytes longer than we thought it was! So with "WARNING: " added, it does overflow the buffer, and we get a security exploit.

The general morals of this story are bolded above, but here's one more specific moral: Never ever use backslash-continuation, except in the special case of defining a multi-line macro body.


Finally, let's look at str_cat one more time. I'll //... out all the parts that don't interest me right now...

td_rc str_cat(struct strbuff *dest, char *source, struct strbuff *logbuf){
  //...
  if (sourcelen > destfree) {
    //...
    if (!logbuf)
      printf("%s", errmes);
    else 
      rc = str_cat(logbuf, errmes, NULL);
  } else { 
    //...
  }
  //...
}

Yes, that's right — you've managed to write a recursive string concatenation routine! str_cat calls itself!

This means that if there's no room in the buffer to concatenate the input strings, we'll produce an error message and attempt to concatenate it onto the log buffer... and if there's no room in the log buffer, we'll produce an error message and — well, thankfully, the recursive call passes NULL instead of a log buffer, so we won't recurse infinitely. That's a small blessing.

Your logbuf parameter is essentially an attempt to allow the user to customize your library's error-reporting mechanism. If you're really interested in going down that route, I recommend that you learn about function pointers, which would allow you to write a library function like this:

typedef void (*error_reporter)(const char *, ...);

td_rc strbuff_plusequals(struct strbuff *dst, struct strbuff src, error_reporter report_error)
{
    const size_t dstlen = strlen(dst->str);
    const size_t srclen = strlen(src.str);
    if (srclen >= dst->size - dstlen) {
        report_error("string concatenation failed: source length %zu > dest free space %zu", srclen, dst->size - dstlen);
        return ERR_STR_CAT;
    } else {
        memcpy(dst->str + dstlen, src.str, srclen + 1);
        return SUCCESS;
    }
}

struct strbuff strbuff_view(const char *s) {
    return (struct strbuff){ strlen(s), (char *)s };
}

void report(const char *fmt, ...);

int main() {
    struct strbuff s;
    int err = strbuff_create(&s, 100);
    if (err) exit(err);
    err = strbuff_plusequals(&s, strbuff_view("hello"), report);
    if (err) exit(err);
    err = strbuff_plusequals(&s, strbuff_view(" world"), report);
    if (err) exit(err);
    assert(strcmp(s.str, "hello world") == 0);
    strbuff_destroy(s);
}

The writing of strbuff_create and strbuff_destroy is left as an exercise for the reader. :)

Another exercise for the reader is to add a void *cookie parameter to report_error.


Anyway, I'd say you should learn about snprintf and strcpy and so on, before trying to invent your own string-handling library. Start with the tools that are provided to you and get good with them, first.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ A very patient and complete answer. \$\endgroup\$ – chux - Reinstate Monica Jan 29 '18 at 15:58
  • 2
    \$\begingroup\$ @chux: I feel like I must say: my answer is extremely incomplete compared to all the things I could have said! (And technically I bet I could have been more patient too. ;)) \$\endgroup\$ – Quuxplusone Jan 29 '18 at 18:22
  • \$\begingroup\$ Agree'd. There is much more - yet complete as much as can be handled at once, IMO. I hope OP considers many of your fine suggestions and posts again with an improved version. Then the next level of feedback can occur. \$\endgroup\$ – chux - Reinstate Monica Jan 29 '18 at 18:32
  • \$\begingroup\$ @Quuxplusone, thanks for your code analysis. I agree with you on the following points - 1. Statement-like macros should be wrapped in do { ... } while (0) 2. The macro MAX_INT_STR_LEN (maximum length of string containing a 64 bit int) is not useful 3. The statement logbuf.size = INIT_BUF_SIZE; should not be in the main function as is it already assigned at the end of the str_malloc function 4. The backslashes in the sprintf string were a mistake 5. MALLOC would cause a memory leaks in the case you mentioned because the return rc statement would cause free(logbuf.str); to be skipped \$\endgroup\$ – user2309803 Jan 29 '18 at 20:32
  • 1
    \$\begingroup\$ "There is much more" might include things like "putting the tests in the same .o file with the code is unusual," "it's too easy to memory-leak," "logging to stderr is more usual than logging to stdout," "ifs without braces are the devil" (I thought I mentioned that but I guess it was only in the context of hygienic macros), and even "if you're using malloc anyway, why bother keeping track of the current capacity? just call realloc before each concatenation and let libc worry about capacity issues." \$\endgroup\$ – Quuxplusone Jan 30 '18 at 22:00
4
\$\begingroup\$

Some ideas better as an answer than a comment

A weakness with rc = str_cat(logbuf, errmes, NULL); is at least conceptual - code is calling a failing function. Code is in an error handler of the function str_cat() and there is something wrong for who-knows-what core reason. Calling str_cat() again is risky. Better to get the error message out first.

If snprintf() is not available, using an generous right-sized buffer rather than a magic number 100 is prudent, one based on the size of the format and it worst-case arguments. Example: with long as 64-bit, 111 was needed in your code (see following). IMO, a generous buffer is 2x whatever you think it might be.

string copy failed : source length 18446744073709551615 >           dest allocated space 18446744073709551615 

Rather than

    char errmes[100];
    sprintf(errmes, "string copy failed : source length %lu > \
        dest allocated space %lu\n", (unsigned long)sourcelen, \
        (unsigned long)dest->size); 

Consider

    #define ERR_STR_CPY "string copy failed : source length %lu > \
        dest allocated space %lu\n"
    // /3 is about log10(2)
    #define ULONG_DECIMAL_LEN (sizeof(unsigned long)*CHAR_BITS/3 + 2)
    #define ERR_STR_CPY_N (sizeof ERR_STR_CPY + 2*ULONG_DECIMAL_LEN)
    char errmes[ERR_STR_CPY_N*2];
    sprintf(errmes, ERR_STR_CPY, (unsigned long)sourcelen, (unsigned long)dest->size); 

It is unclear, perhaps a mistake, that OP put so many spaces in the format below

    // long format?
    sprintf(errmes, "string copy failed : source length %lu > \
        dest allocated space %lu\n", (unsigned long)sourcelen, \
        (unsigned long)dest->size); 

    // wanted format?
    sprintf(errmes, "string copy failed : source length %lu > " 
        "dest allocated space %lu\n", (unsigned long)sourcelen, 
        (unsigned long)dest->size); 

That is why programmer computed magic numbers like 100 vs. program computed values are risky.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Btw, I learned the other day that GCC has a secret hidden feature that will find all these format-string bugs for you: -Wformat-overflow=2 -Wformat-truncation=2. Unfortunately these options are not part of -W -Wall -Wextra -Wformat=2 -Wformat-security -pedantic; you have to find out about them by word of mouth. :/ Anyway, -Wformat-overflow=2 does indeed complain about a couple of places in OP's code. \$\endgroup\$ – Quuxplusone Jan 30 '18 at 21:50
  • \$\begingroup\$ @Quuxplusone "-Wformat-overflow=2 -Wformat-truncation=2" --> very interesting. FWIW, the biggest issue I have about snprintf() is the lack of error handling employed when the buffer was truncated. Sure, the array did not overrun, but its contents are certainly problematic. snprintf(command, 12, "%s", "erase drive not"); "-Wformat-truncation=2"` can help. Thanks \$\endgroup\$ – chux - Reinstate Monica Jan 30 '18 at 22:07
  • \$\begingroup\$ I don't consider truncation a huge problem usually, and when it is, can't you just do int written = snprintf(buf, sizeof buf, "fmt"...); if (written >= sizeof buf) report_error(); else trust(buf);? Anyway, I bet we agree that "biggest issues" are relative. ;) \$\endgroup\$ – Quuxplusone Jan 30 '18 at 22:37
  • \$\begingroup\$ Thanks for your suggestions. Lots of good ideas from yourself and @Quuxplusone but I need some time to digest them \$\endgroup\$ – user2309803 Jan 30 '18 at 22:49
  • 1
    \$\begingroup\$ @Quuxplusone snprintf() returns a negative on encoding error also. Yes, int written = snprintf(..); if (written >= sizeof buf) is usually good enough to wrap negatives as hoped and catch both encoding error and truncation. \$\endgroup\$ – chux - Reinstate Monica Jan 30 '18 at 23:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.