best practice regarding char pointers returned by functions

Question

What is best practice when dealing with functions which return malloc'd pointers to C-strings?

Here's an example:

FILE *f;
char *tmp;
for (i = 0; i <= 10; i++) {
    tmp = concat(fn, i);
    f = fopen(tmp, "w");
    free(tmp);
    // read f, do something useful ...
    fclose(f);
}

char* concat(char *a, int b) returns a pointer to a new C-string which contains the concatenation of a and b.

Not only do I have to specify a temporary pointer which is then passed to fopen, I also have to free(tmp) every time. I would rather prefer something like this:

FILE *f;
char *tmp;
for (i = 0; i <= 10; i++) {
    f = fopen(concat(fn, i), "w");
    // read f, do something useful ...
    fclose(f);
}

But this of course leads to memory leaks. So what is best practice here? Something like concat(char *a, int b, char *result) where result is expected to be a pre-allocated memory for the resulting C-string? This solution has its disadvantages like limited or not optimal size of result.

Answer 1

Both approaches are used in the industry. In your example, people could make an assumption about the maximum size for the resulting filename and use a local array this way:

for (int i = 0; i <= 10; i++) {
    char filename[1024];
    snprintf(filename, sizeof filename, "%s%d", fn. i);
    FILE *f = fopen(filename, "w");
    if (f != NULL) {
        // read f, do something useful ...
        fclose(f);
    } else {
        // report the error?
    }
}

Note that the truncation can be detected with if (snprintf(filename, sizeof filename, "%s%d", fn. i) >= (int)sizeof filename).

If no assumption should be made about the filename length or if the filename composition method is more complicated, returning an allocated string may be a more appropriate option, but testing for memory allocation errors should also be done:

for (int i = 0; i <= 10; i++) {
    char *filename = concat(fn, i);
    if (filename == NULL) { 
       /* handle the error */
       ...
       // break / continue / return -1 / exit(1) ...
    }
    FILE *f = fopen(filename, "w");
    if (f == NULL) {
        /* report this error, using `filename` for an informative message */
    } else {
        // read f, do something useful...
        // keep `filename` available for other reporting 
        fclose(f);
    }
    free(filename);
}

If you are not ready to perform all this bookkeeping, you should probably use a different language with more elaborate object life cycle management or with a garbage collector.

---

Finally, using C99 compound literals, you could define concat to fit your simplified use case:

char *concat(char *dest, const char *s, int b) {
    sprintf(dest, "%s%d", s, b);
    return dest;
}
#define CONCAT(a, b) concat((char[strlen(a) + 24]){""}, a, b)

CONCAT defines an unnamed local variable length char array of the appropriate size and constructs the concatenation of string a and int b into it. I changed the case to uppercase to underscore the fact that a is evaluated twice in the expansion, and thus should not be an expression that involve side-effects.

You could use this macro as expected in your second code fragment:

    FILE *f;
    char *tmp;
    for (i = 0; i <= 10; i++) {
        f = fopen(CONCAT(fn, i), "w");
        // read f, do something useful ...
        fclose(f);
    }

I probably would not recommend this type of usage, but this is only my opinion.

Answer 2

A more solid design:

FILE *open_file_number(const char *str, int number)
{
    size_t size = strlen(str) + 32;
    char *path = malloc(size);

    if (path == NULL)
    {
        return NULL;
    }
    snprintf(path, size, "%s%d", str, number);

    FILE *file = fopen(path, "w");

    free(path);
    return file;
}

for (i = 0; i <= 10; i++)
{
    FILE *file = open_file_number(some_path, i);

    if (file != NULL)
    {
        // Do your stuff
        fclose(file);
    }
}

Answer 3

Your first code snippet, where you save the returned pointer and free it when you're done using it, is the proper way to work with a function returning malloc'ed memory.

There are several POSIX functions, such as strdup and getline, that work in this manner, so this is a well known idiom.

Alternatives are:

• Return a pointer to a static buffer. This has the disadvantage that it is not thread safe and also can't be called twice in one expression.
• Accept a pointer to a properly sized buffer, in which case properly sizing the buffer is up to the caller.

Answer 4

What is best practice when dealing with functions which return malloc'd pointers to C-strings?

Best practice: don't use them. A library expecting the caller to free returned data is almost certainly badly designed, with very few exceptions. We know this from 40 years of C language history, where crappily written libraries have created millions upon millions of memory leak bugs.

The basic rule of sane, useful library API design is:

Whoever allocates something is responsible for cleaning up their own mess.

Since C doesn't have RAII or constructors/destructors, that unfortunately means that the sane library needs to provide you with a function for the clean-up and you need to remember to call it. If it doesn't provide such a function, you might want to consider writing wrapper functions that do this - correcting the bad library design for them.

If you are the one implementing the library, you should always try to leave memory allocation to the caller, whenever possible. This is traditionally done by the function taking a pointer to a buffer which it writes to. Then either leaves it to the caller to allocate enough memory (like strcpy/strcat), or alternatively provide a variable with maximum buffer size, after which the function returns how much of the buffer it actually used (like fgets).

---

In your example, a soundly designed concat would perhaps look like

const char* concat (char* restrict dst, const char* restrict src, int i);

Where src is the source string, i is the integer to add and dst is a large-enough buffer provided by the caller. Optionally, the function returns a pointer equivalent to dst, for convenience. The above also implements proper const correctness plus a micro-optimization with restrict that means that the pointers passed aren't allowed to overlap.

Usage:

char buf [LARGE_ENOUGH];
fp = fopen(concat(buf, foo, i), "w");

Answer 5

If you know the maximum size of your strings, you could also do something like this:

char* concat_(char* buf, size_t s, char *a, int b)
{
    /* ... your code ... */
    return buf;
}

#define concat(a, b)    concat_((char[100]){""}, 100, (a), (b))

The macro allocates an unnamed local variable and passes it on to the concat_ function. This function can then do whatever it did before, and just return the pointer to that same object. No malloc, no free, no worries (other than possibly blowing up your stack, if you make the buffer too big).

Edit: Please note that, as Gerhardh pointed out in the comments, this creates a local object (which has automatic storage duration), so the pointer returned by the macro is only valid within the same block where that macro was actually called. Therefore you can't for instance use that pointer as a return value of the functoin which called the macro.