CODEWITHSUNDEEP
cvariadic-functions
fearless_fool
fearless_fool

Reputation: 35159

Implementing a sub-function with va_list and va_arg

I have a varargs-style function that I want to split out to a va_list-style sub-function. The original function:

void container_append(container_t *c, element_t *element, ...) {
  element_t *e;
  va_list ap;

  va_start(ap, element);
  while((e = va_arg(ap, element_t *)) != NULL) {
    container_append_aux(c, e);
  }
  va_end(ap);
}

Note that the caller must terminate the list of elements with a NULL, but that should not cause any problems. Refactored:

void container_append(container_t *c, element_t *element, ...) {
  va_list ap;
  va_start(ap, element);
  container_vappend(c, ap);
  va_end(ap);
}

void container_vappend(container_t *c, va_list ap) {
  element_t *e;
  while ((e = va_arg(ap, element_t *)) != NULL) {
    container_append_aux(c, e);
  }
}

However, when I call it like this:

container_append(c, NULL);

... inside of container_vappend(), the call to va_arg() is returning something that isn't NULL.

This is a transcription of a more complicated function, but barring any typos, did I miss something in the setup or usage of va_list and va_arg()?

Upvotes: 1

Views: 509

Answers (2)

chux
chux

Reputation: 153338

In addition to issued identified well by @zwol ...

container_append(c, one or more arguments, NULL); is potential undefined behavior (UB).

container_append() expects a element_t *element and NULL may be simply 0.

NULL which expands to an implementation-defined null pointer constant ...

An integer constant expression with the value 0, or such an expression cast to type void *, is called a null pointer constant.

NULL is not specified even to be the same size as a pointer.

va_arg(ap, element_t *) is then UB.

To make a safer call, use container_append(c, args, (element_t *) NULL);

Upvotes: 5

zwol
zwol

Reputation: 140495

When container_append is called like this

container_append(c, NULL);

the named parameter element will be 0, and there won't be any anonymous parameters. Under those conditions, container_append must not call va_arg at all, or the program has undefined behavior. It happened to work by accident before you refactored the code, but the original code is just as buggy as the refactored version.

You can either check element before the loop...

void
container_append(container_t *c, element_t *element, ...)
{
    if (!element) return;

    container_append_aux(c, element);

    va_list ap;
    va_start(ap, element);
    while ((element = va_arg(ap, element_t *)))
        container_append_aux(c, element);
    va_end(ap);
}

... or you can make all of the element arguments be anonymous:

void
container_append(container_t *c, ...)
{
    va_list ap;
    va_start(ap, c);

    element_t *e;
    while ((e = va_arg(ap, element_t *)))
        container_append_aux(c, e);

    va_end(ap);
}

The latter structure is more compatible with the vappend refactor you want to do.

EDIT: Regarding this query in a comment:

I thought va_start(ap, element) (in the caller) would set up va_arg to return element first. Maybe it doesn't work that way?

Indeed, it does not work that way. va_start sets up va_arg to return the first of the anonymous arguments. If there weren't any anonymous arguments, then you walk off the end the very first time you call va_arg, and trigger UB.

Upvotes: 4

Related Questions