5
\$\begingroup\$

I'm working on a project of mine which required multiple calls to strncat(), and it came to me that it'd have been much easier if C also had something like "file://" + getenv("HOME") + filename for string generation. So I built a function that works in a similar fashion, it is a variadic function, the arguments are NULL terminated.

Usage: char *str = mutant_string ("file://", getenv ("HOME"), filename, NULL);

It's obviously up to the user to free the memory str allocated.

The code:-

char *mutant_string (const char *s, ...) {

    va_list argv;
    va_start (argv, s);

    char *return_string = strdup (s);

    size_t realsize, nmemb;

    realsize = strlen (return_string); // Or initial size

    char *c, *ptr;

    while ((c = va_arg(argv, char*)))
    {
        nmemb = strlen (c);
        realsize += nmemb;

        ptr = realloc (return_string, sizeof(char) * (realsize + 1));

        if (!ptr) {
            perror ("realloc");
            free (return_string);
            va_end (argv);
            return NULL;
        }

        return_string = ptr;
        strncat (return_string, c, nmemb);
    }

    return_string[realsize] = 0;

    va_end (argv);

    return return_string;
}

I welcome any kind of observations or opinions. Let me know if there's an external library or function that does exactly this and something that I can read.

Thank you.

\$\endgroup\$
4
\$\begingroup\$

It looks like this code needs a couple of headers to be included before it will compile:

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

It also only works on platforms that have strdup(). It would be easy to make it more portable.

If s is a null pointer, then we have undefined behaviour, but we haven't indicated that we don't allow a null for the first argument.

Why are we not testing whether strdup() returns a null pointer?

Well done for avoiding the common hazard with realloc(). Too many C programmers fail to deal properly with allocation failure. The scope of ptr could be reduced so that we declare and initialise together, and we don't need to multiply by sizeof (char) (which must be 1, by definition):

char *ptr = realloc(return_string, realsize + 1);

The use of strncat() in a loop is wasteful, since it starts from the beginning every time. Since we know where the string ends, it's better to start from that position.

The algorithm could be much more efficient. Instead of many realloc() calls, we could make two passes over the arguments: one to measure the total size, and one to copy the characters across.

Compare this extract from a Valgrind run of the code:

==25527== HEAP SUMMARY:
==25527==     in use at exit: 0 bytes in 0 blocks
==25527==   total heap usage: 5 allocs, 5 frees, 1,063 bytes allocated

against the two-pass version that doesn't need to realloc:

==24899== HEAP SUMMARY:
==24899==     in use at exit: 0 bytes in 0 blocks
==24899==   total heap usage: 2 allocs, 2 frees, 1,037 bytes allocated

That's fewer than half the allocations for a very simple "Hello World" test program:

char *mutant_string (const char *s, ...);

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

int main(void)
{
    char *s = mutant_string("Hello", " ", "World", "!", NULL);
    if (!s) {
        return 1;
    }
    puts(s);
    free(s);
}

Here's what the two-pass version looks like:

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

char *mutant_string(const char *s, ...)
{
    if (!s) {
        return NULL;
        /* alternatively - return a new, empty string */
    }

    va_list argv;

    /* First pass - measure the total length */
    size_t total_len = strlen(s);
    va_start(argv, s);
    const char *c;
    while ((c = va_arg(argv, const char*))) {
        total_len += strlen(c);
    }
    va_end(argv);

    char *buf = malloc(total_len + 1);
    if (!buf) {
        return buf;
    }

    /* Second pass - copy the string data */
    char *p = buf;
    {
        /* first argument */
        size_t len = strlen(s);
        memcpy(p, s, len);
        p += len;
    }

    /* other arguments */
    va_start(argv, s);
    while ((c = va_arg(argv, const char*))) {
        size_t len = strlen(c);
        memcpy(p, c, len);
        p += len;
    }
    va_end(argv);
    *p = '\0';

    return buf;
}
\$\endgroup\$
1
  • \$\begingroup\$ Wow .. this was so much helpful! Thank you. \$\endgroup\$ – debdutdeb Mar 29 at 4:39
3
\$\begingroup\$

Usage: char *str = mutant_string ("file://", getenv ("HOME"), filename, NULL);

mutant_string("Hello", " ", "World", "!", NULL); contains a weakness.

NULL, a null pointer constant is not certainly a char *, a null pointer. It could be an int 0 for example and cause grief with va_arg(argv, char*) with undefined behavior (UB).

When an integer, passing a NULL to a ... function does not convert the NULL to a pointer.

When NULL is an integer, it is often typed to be the same size of a char*, such as 0L perhaps - reducing bad UB. Yet that is not required nor are int and char * certainly passed the same way. Robust uses a pointer to terminate the list.

Consider alternatives

mutant_string("file://", getenv ("HOME"), filename, (char*)NULL); 
mutant_string("file://", getenv ("HOME"), filename, (char*)0);

the arguments are NULL terminated.

Fails a corner case of mutant_string(NULL) as code attempts strdup(NULL) and likely strlen(NULL). Easy to fix with a test.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Oh yes, I'd forgotten the gotcha with NULL and varargs! Good catch. \$\endgroup\$ – Toby Speight Mar 30 at 17:31
2
\$\begingroup\$

The main issue with this is that variadic functions have non-existent type safety. It is better to avoid them - it's one of those old things in C that's simply broken.

I would suggest to instead write a function like:

char* strcat_alloc (size_t n, const char* list[n])

Okay, so that takes an array as parameter which may be cumbersome. To make it easier to use, we can combine it with a variadic wrapper macro and compound literals:

#define cat(...) strcat_alloc( sizeof (const char*[]){ __VA_ARGS__ } / sizeof(const char*), \
                               (const char*[]){ __VA_ARGS__ } )

Now isn't that as bad as a variadic function? Not at all, because in this case we force every parameter passed to be a pointer part of an initializer list. C has somewhat strict rules then, particularly if you use a conforming compiler. Then it's neither allowed to pass pointers of wrong type, not integers etc.

Complete example together with a naive implementation of the function follows. It can be optimized a lot, but the main point I wished to make is the function interface.

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

#define cat(...) strcat_alloc(sizeof (const char*[]){ __VA_ARGS__ } / sizeof(const char*), \
                              (const char*[]){ __VA_ARGS__ } )

char* strcat_alloc (size_t n, const char* list[n])
{
  size_t size = 0;
  for(size_t i=0; i<n; i++)
  {
    size += strlen(list[i]);
  }
  size++; // null term
  
  char* result = malloc(size);
  if(result == NULL)
  {
    return result;
  }
  
  *result = '\0';
  for(size_t i=0; i<n; i++)
  {
    strcat(result, list[i]);
  }
  return result;
}

int main (void)
{
  char STR1[] = "hello";
  char STR2[] = "world";
  char* str = cat(STR1, " ", "meow", " ", STR2, " ", "meow");
  puts(str);
  free(str);
}
\$\endgroup\$
4
  • \$\begingroup\$ Thanks for this answer. I actually didn't know you could cast __VA_ARGS__ like that (or is it even called casting?). Can you please provide me with any reading material on this? Really appreciate the help. \$\endgroup\$ – debdutdeb Mar 29 at 13:55
  • 2
    \$\begingroup\$ @debdutdeb It's not a cast, it's a compound literal, like I wrote. A temporary anonymous variable if you will. Basically (type){ initializers } and it gets the same scope as the one it was declared in. In this case __VA_ARGS__ forms the initializer list of a temporary array, and initializer lists of pointers have rather strict typing rules. \$\endgroup\$ – Lundin Mar 29 at 13:59
  • \$\begingroup\$ for(size_t i=0; i<n; i++) { strcat(result, list[i]); } uses the a slow algorithm, but code is, as you say, "can be optimized a lot" \$\endgroup\$ – chux - Reinstate Monica Mar 30 at 13:26
  • \$\begingroup\$ @chux-ReinstateMonica Yes, this is just a naive placeholder code. For real quality code I would probably just speculatively allocate 256 bytes or so, then fill those bytes up at the same time as I iterate through the string arrays. And if it goes beyond 256 bytes, realloc size*2 and so on. \$\endgroup\$ – Lundin Mar 30 at 13:49

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.