Fix use-after-free and memory leaks when realloc returns NULL

[aaranmcguire]

PR 2: Fix use-after-free and memory leaks when realloc returns NULL

Summary

Five locations call realloc and assign the result directly back to the
original pointer without checking whether realloc returned NULL. When
realloc fails it returns NULL but does not free the old allocation.
Each site below therefore:

1. Leaks the original allocation (the pointer is overwritten with NULL)
2. Immediately uses the now-NULL pointer, causing a crash

These are all in hot paths triggered by normal, well-formed files.

---

1. spss/readstat_por_read.c — maybe_read_string (string buffer growth)

Vulnerable code

ctx->string_buffer = realloc(ctx->string_buffer, ctx->string_buffer_len);
memset(ctx->string_buffer, 0, ctx->string_buffer_len);  /* ← crashes if NULL */

Why it triggers

string_buffer_len is set from a length field in the POR file
(bounded by MAX_STRING_LENGTH = 20000). The buffer grows whenever a longer
string is encountered. On any memory-constrained system, the realloc can
fail; the old ctx->string_buffer pointer is leaked and memset crashes.

Reproduction

Parse a POR file with a moderately long string variable under a 1 MB
address-space limit (ulimit -v 1024 on Linux).  The realloc for the
string buffer will fail and crash in memset.

Fix

-ctx->string_buffer = realloc(ctx->string_buffer, ctx->string_buffer_len);
-memset(ctx->string_buffer, 0, ctx->string_buffer_len);
+unsigned char *new_string_buf = realloc(ctx->string_buffer, ctx->string_buffer_len);
+if (new_string_buf == NULL) { retval = READSTAT_ERROR_MALLOC; goto cleanup; }
+ctx->string_buffer = new_string_buf;
+memset(ctx->string_buffer, 0, ctx->string_buffer_len);

---

2. spss/readstat_por_read.c — read_variable_label_record

Vulnerable code

varinfo->label = realloc(varinfo->label, 4*strlen(string) + 1);
retval = readstat_convert(varinfo->label, 4*strlen(string) + 1, ...);
/* ← readstat_convert writes to varinfo->label without null check */

Why it triggers

strlen(string) can be up to MAX_STRING_LENGTH (20000), making the
allocation ~80 KB. On failure, varinfo->label becomes NULL and
readstat_convert writes through the NULL pointer.

Fix

-varinfo->label = realloc(varinfo->label, 4*strlen(string) + 1);
-retval = readstat_convert(varinfo->label, 4*strlen(string) + 1, string, ...);
+size_t label_alloc_len = 4*strlen(string) + 1;
+char *new_label = realloc(varinfo->label, label_alloc_len);
+if (new_label == NULL) { retval = READSTAT_ERROR_MALLOC; goto cleanup; }
+varinfo->label = new_label;
+retval = readstat_convert(varinfo->label, label_alloc_len, string, ...);

---

3. sas/readstat_xport_read.c — xport_read_labels_v8 and xport_read_labels_v9

Vulnerable code (v8, two pointers; v9, four pointers)

/* v8 */
name  = realloc(name,  name_len + 1);
label = realloc(label, label_len + 1);
/* immediately used: */
if (read_bytes(ctx, name, name_len) != name_len || ...)

/* v9 additionally: */
format   = realloc(format,   format_len + 1);
informat = realloc(informat, informat_len + 1);

Why it triggers

name_len and label_len are 16-bit values read directly from the XPORT
file, so up to 65535 bytes each. format_len and informat_len are also
file-controlled. If any realloc fails, the corresponding pointer becomes
NULL and read_bytes immediately tries to write through it.

Reproduction

An XPORT v8 file declaring label_count=1 with label_len=0xFFFF (65535)
will request a 64 KB realloc. Under a constrained process this fails,
leaking name and crashing on read_bytes(ctx, label=NULL, 65535).

Fix

Apply the temp-pointer pattern to all four realloc calls:

-name  = realloc(name,  name_len + 1);
-label = realloc(label, label_len + 1);
+char *tmp_name = realloc(name, name_len + 1);
+if (tmp_name == NULL) { retval = READSTAT_ERROR_MALLOC; goto cleanup; }
+name = tmp_name;
+
+char *tmp_label = realloc(label, label_len + 1);
+if (tmp_label == NULL) { retval = READSTAT_ERROR_MALLOC; goto cleanup; }
+label = tmp_label;

(Same pattern for format and informat in xport_read_labels_v9.)

---

4. stata/readstat_dta_read.c — dta_handle_value_labels bypasses 16 MiB safety cap

Vulnerable code

utf8_buffer = realloc(utf8_buffer, utf8_buffer_len);
/* null check IS present, but raw realloc bypasses readstat_malloc cap */

Why it matters

readstat_malloc and readstat_realloc enforce a 16 MiB hard cap
(MAX_MALLOC_SIZE = 0x1000000). utf8_buffer_len = 4 * txtlen + 1 where
txtlen can approach 16 MB (limited by a prior readstat_realloc on
table_buffer). So utf8_buffer_len can reach ~64 MB. Because raw
realloc is used instead of readstat_realloc, a crafted DTA file with a
large value-label table can allocate four times the intended cap.

Fix

-utf8_buffer = realloc(utf8_buffer, utf8_buffer_len);
+utf8_buffer = readstat_realloc(utf8_buffer, utf8_buffer_len);

---

5. readstat_writer.c — six capacity-doubling realloc calls

Vulnerable code (pattern, repeated six times)

/* in readstat_add_value_label, readstat_add_label_set,
   readstat_add_variable, readstat_append_string_ref,
   readstat_add_note, readstat_variable_set_label_set */
writer->variables = realloc(writer->variables,
        writer->variables_capacity * sizeof(readstat_variable_t *));
/* ← if realloc returns NULL:
     - old pointer leaked
     - next write to writer->variables[count++] crashes */

Why it triggers

These arrays double in capacity when full (starting at 16 entries). Any
write-heavy application — e.g. writing a dataset with thousands of variables
or thousands of value labels — will trigger multiple resizes. Under memory
pressure, any realloc can return NULL.

Fix (same pattern applied to all six sites)

-writer->variables = realloc(writer->variables,
-        writer->variables_capacity * sizeof(readstat_variable_t *));
+void *tmp = realloc(writer->variables,
+        writer->variables_capacity * sizeof(readstat_variable_t *));
+if (tmp == NULL) return NULL;
+writer->variables = tmp;