`tal_strdup(ctx, NULL)` returns uninitialized string

[whitslack]

There is an edge case in tal_strdup that results in the return of an unterminated string to the caller.

tal_strdup(ctx, NULL)

ccan/ccan/tal/str/str.h

Line 19 in cd56b18

#define tal_strdup(ctx, p) tal_strdup_(ctx, p, TAL_LABEL(char, "[]"))

tal_strdup_(ctx, NULL, "char[]")

ccan/ccan/tal/str/str.c

Lines 15 to 19 in cd56b18

	char *tal_strdup_(const tal_t *ctx, const char *p, const char *label)
	{
	/* We have to let through NULL for take(). */
	return tal_dup_arr_label(ctx, char, p, p ? strlen(p) + 1: 1, 0, label);
	}

tal_dup_arr_label(ctx, char, NULL, 1, 0, "char[]")

ccan/ccan/tal/tal.h

Lines 407 to 410 in cd56b18

	#define tal_dup_arr_label(ctx, type, p, n, extra, label) \
	((type *)tal_dup_((ctx), tal_typechk_(p, type *), \
	sizeof(type), (n), (extra), false, \
	label))

(char *) tal_dup_(ctx, NULL, 1, 1, 0, false, "char[]")

ccan/ccan/tal/tal.c

Lines 814 to 854 in cd56b18

	void *tal_dup_(const tal_t *ctx, const void *p, size_t size,
	size_t n, size_t extra, bool nullok, const char *label)
	{
	void *ret;
	size_t nbytes = size;
	if (nullok && p == NULL) {
	/* take(NULL) works. */
	(void)taken(p);
	return NULL;
	}
	if (!adjust_size(&nbytes, n)) {
	if (taken(p))
	tal_free(p);
	return NULL;
	}
	/* Beware addition overflow! */
	if (n + extra < n) {
	call_error("dup size overflow");
	if (taken(p))
	tal_free(p);
	return NULL;
	}
	if (taken(p)) {
	if (unlikely(!p))
	return NULL;
	if (unlikely(!tal_resize_((void **)&p, size, n + extra, false)))
	return tal_free(p);
	if (unlikely(!tal_steal(ctx, p)))
	return tal_free(p);
	return (void *)p;
	}
	ret = tal_alloc_arr_(ctx, size, n + extra, false, label);
	if (ret && p)
	memcpy(ret, p, nbytes);
	return ret;
	}

p = NULL;
size = 1;
n = 1;
extra = 0;
nullok = false;
label = "char[]";
nbytes = 1;

if (false && NULL == NULL) ; // branch not taken

if (!adjust_size(&nbytes, 1)) ; // branch not taken

if (1 + 0 < 1) ; // branch not taken

if (taken(NULL)) ; // branch not taken

ret = tal_alloc_arr_(ctx, 1, 1 + 0, false, "char[]");

ccan/ccan/tal/tal.c

Lines 500 to 507 in cd56b18

	void *tal_alloc_arr_(const tal_t *ctx, size_t size, size_t count, bool clear,
	const char *label)
	{
	if (!adjust_size(&size, count))
	return NULL;
	return tal_alloc_(ctx, size, clear, label);
	}

size = 1
count = 1
clear = false
label = "char[]"

if (!adjust_size(&size, 1)) ; // branch not taken

ret = /*return*/ tal_alloc_(ctx, 1, false, "char[]");

if (ret && NULL) ; // branch not taken
//	memcpy(ret, p, nbytes);
return ret;

So at the end of this call chain, we return a pointer to a buffer of size 1 whose sole byte has never been initialized, and we're claiming that this pointer points to a NUL-terminated string. That's bad! ☹️