String Concatenation using a variadic function in C

Question

I am trying to write a basic quiz program in C. It will basically store cards and answers to them. But in the meanwhile I am trying to use the new techniques I learned like variadic functions and dynamic memory allocation.

I want the program to be able to scale as I change the constants, there should be no "magic numbers" as defined by K&R. The problem is that I cannot use variables in the format parameter of fscanf. I need to define the string length by hand. In order to get over this limitation I have attempted to write a string concatanation function, that will generate the fscanf format parameter.

e.g.

char * scanf_string = et_concat(5, "%", CARD_SIZE, "s | %", ANSWER_SIZE, "s");

The constants are defined in consts.h

#define CARD_SIZE 200
#define ANSWER_SIZE 1000
#define CONCAT_SIZE 20

The et_concat function is in etstring.h. This is where the segmentation fault happens.

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

char * et_concat (int count, char * str, ...)
{
    va_list ap;
    int j;
    char *concatted_string = (char *) malloc (count*CONCAT_SIZE+1);

    va_start(ap, str);
    for (j = 0; j < count; j++) {
        strcat(concatted_string, va_arg(ap, char *));
    }
    va_end(ap);

    return concatted_string;
}

The code I am trying to call et_concat from is in reader.c

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "consts.h"
#include "etstring.h"

int iterate_inputs()
{
  char card[CARD_SIZE];
  char answer[ANSWER_SIZE];
  FILE *fp = fopen("data.txt","r");
  if (fp == NULL)
  {
    return EXIT_FAILURE;
  }
  char * scanf_string = et_concat(5, "%", CARD_SIZE, "s | %", ANSWER_SIZE, "s");
  printf(scanf_string);
  fscanf(fp, scanf_string, card, answer);
  printf("%s | %s\n", card, answer);
  fclose(fp);
  return EXIT_SUCCESS;
}

Thanks very much in advance.

Answer 1

Your concatenation routine has various errors and shortcomings:

• You already read the first string in the non-variadic argument str, but you print only variadic arguments, of which there are only four left. You essentially skip the first string and the fifth string is garbage.

• After allocating the string, you don't initialise it, so that it might contain garbage. Setting the first char to '\0' is enough to initialize a zero-terminated empty string.

• You allocate a fixed buffer and don't really check for overflow. This is a bit counterintuitive: Either you allocate a buffer (which you must free later) and allow for arbitrary string lengths or you pass in a buffer of restricted length and don't care about allocation in your routine.

There's also the problem that your arguments aren't all strings. A simple solution to that is to use a strinfification macro. You must expand the argument first, so that your string has the real value, not the macro's name, which doesn't mean anything to scanf. The problem with this solution is that the size given in the format string doesn't include the terminating '\0', so that you should allocate your buffers like so:

char card[CARD_SIZE + 1];

Here's a corrected version of your implementation, which still doesn't check for buffer overflows, though:

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

#define CARD_SIZE 200
#define ANSWER_SIZE 1000
#define CONCAT_SIZE 20

char *et_concat(int count, ...)
{
    va_list ap;
    char *concatted_string = malloc(count * CONCAT_SIZE + 1);
    int j;

    *concatted_string = '\0';

    va_start(ap, count);
    for (j = 0; j < count; j++) {
        strcat(concatted_string, va_arg(ap, char *));
    }
    va_end(ap);

    return concatted_string;
}

#define STR1(X) #X
#define STR(X) STR1(X)

int main()
{
    char *scanf_string = et_concat(5, 
        "%", STR(CARD_SIZE), "s | %", STR(ANSWER_SIZE), "s");

    printf("'%s'\n", scanf_string);
    free(scanf_string);

    return 0;
}

Edit I had written STR1(CARD_SIZE) in the code, when it should be just STR. Fixed now.

Answer 2

There is a much simpler way to construct your format strings. Since the "variables" are compile-time constants, you can stringify them using a macro, and then let the compiler concatenate them for you. The compiler will automatically concatenate adjacent string constants into a single string, eg. "this" "that" -> "thisthat". confession: I totally checked-out the linked question to get these (simple!?) macros to work right.

$cat strfy.c
#define CARD_SIZE 200
#define ANSWER_SIZE 20
#define XSTR(x) STR(x)
#define STR(x) #x

printf("%" XSTR(CARD_SIZE) "s | %" XSTR(ANSWER_SIZE) "s", card, answer);

$cpp -P strfy.c
printf("%" "200" "s | %" "20" "s", card, answer);

As M Oehm points out, you'll still need this type of macro with your variadic function in order to fold the integers to strings. Otherwise, the function would need to be more complicated to handle the varying types of arguments.

Still another way would be to use sprintf to build the format string. But this way has the extra complication that you'd need to pre-calculate the size of the string. That problem can be solved by using snprintf with NULL as the target string, and the return value will be the needed size.

#define CARD_SIZE 200
#define ANSWER_SIZE 1000
#include <stdlib.h>
#include <stdio.h>

int main()
{
    char *fmt;
    size_t fmt_sz;
    char card[CARD_SIZE + 1];
    char answer[ANSWER_SIZE + 1];

    fmt = malloc(fmt_sz = snprintf(NULL, 0, "%%%ds | %%%ds", CARD_SIZE, ANSWER_SIZE) + 1);
    snprintf(fmt, fmt_sz, "%%%ds | %%%ds", CARD_SIZE, ANSWER_SIZE);
    //printf("%s", fmt);
    scanf(fmt, card, answer);

    return 0;
}