From bb6d834140ec7d36529e9338708ff39b40161d61 Mon Sep 17 00:00:00 2001 From: Peter M Date: Mon, 20 Apr 2026 21:01:15 +0200 Subject: [PATCH] Fix localtime/1 memory leak, use-after-free, and TZ restore bugs The original localtime/1 implementation had several correctness issues: - Memory leak on newlib/picolibc (ESP32): setenv leaks the old "NAME=value" string on every overwrite (espressif/esp-idf#3046). Fix: use putenv with a fixed-size static buffer that is installed once and modified in place, avoiding repeated allocations. Guarded behind __NEWLIB__ / __PICOLIBC__ so standard platforms use normal setenv/unsetenv. - Use-after-free: the getenv("TZ") pointer was used after setenv overwrites it, reading freed or stale memory. Fix: strdup the old TZ value before modifying the environment. - Missing tzset after restore: the original code called tzset only when setting the new TZ, not after restoring the old one, leaving libc in an inconsistent state for subsequent calls. - Missing localtime_r NULL check: a failing localtime_r would pass NULL to build_datetime_from_tm. Fix: raise badarg on NULL. The static buffer workaround raises badarg for oversized TZ strings rather than silently leaking. The restore path falls back to setenv if the buffer is too small (refreshing the cached env pointer to maintain the static buffer invariant), since the environment must always be restored. When TZ was originally unset, "UTC0" is used as the restore value on newlib/picolibc since putenv cannot remove entries; this is a pragmatic approximation (not a true restore) that is acceptable because unset TZ defaults to UTC on these platforms. The concurrency model assumes AtomVM is the sole user of TZ -- no external threads should read or write TZ or call time functions that depend on it during the temporary override. See: https://github.com/espressif/esp-idf/issues/3046 Signed-off-by: Peter M --- CHANGELOG.md | 1 + src/libAtomVM/nifs.c | 112 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 111 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c674a2fa0..4a1da951d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Stop using deprecated `term_from_int32` on RP2 platform - Stop using deprecated `term_from_int32` on ESP32 platform - Fixed improper cast of ESP32 `event_data` for `WIFI_EVENT_AP_STA(DIS)CONNECTED` events +- Fixed `erlang:localtime/1` memory leak, use-after-free, and TZ restore bugs on newlib/picolibc ## [0.7.0-alpha.1] - 2026-04-06 diff --git a/src/libAtomVM/nifs.c b/src/libAtomVM/nifs.c index ff4a760cdd..096c5ee83b 100644 --- a/src/libAtomVM/nifs.c +++ b/src/libAtomVM/nifs.c @@ -1949,6 +1949,64 @@ term nif_erlang_universaltime_0(Context *ctx, int argc, term argv[]) return build_datetime_from_tm(ctx, gmtime_r(&ts.tv_sec, &broken_down_time)); } +// Workaround for newlib/picolibc setenv memory leak: use putenv with a +// fixed-size static buffer. The buffer is installed once via putenv and then +// modified in place so repeated TZ changes never allocate. +// See: https://github.com/espressif/esp-idf/issues/3046 +// Both newlib and picolibc leak the old "NAME=value" string on overwrite. +#if defined(__NEWLIB__) || defined(__PICOLIBC__) +#define AVM_TZ_SETENV_LEAKS 1 +#else +#define AVM_TZ_SETENV_LEAKS 0 +#endif + +#if AVM_TZ_SETENV_LEAKS + +// Max TZ value length is 60 bytes; longest POSIX TZ strings (e.g. +// "CET-1CEST,M3.5.0/2,M10.5.0/3") are well under this limit. +#define TZ_BUFFER_SIZE 64 +#define TZ_MAX_VALUE_LEN (TZ_BUFFER_SIZE - 4) // 3 for "TZ=" + 1 for '\0' + +static char tz_buffer[TZ_BUFFER_SIZE] = "TZ="; +static bool tz_buffer_installed = false; +// Cached pointer into the environment; assumes AtomVM is the sole user of TZ +// (no external threads reading or writing TZ or calling time functions +// that depend on it during the temporary override). +static char *tz_env_value = NULL; + +// Write a TZ value into the static buffer. Returns false if the value is +// too long to fit (the buffer is left unchanged in that case). +// Caller must hold env_spinlock. +static bool set_static_tz_value(const char *tz) +{ + size_t tz_len = strlen(tz); + if (tz_len > TZ_MAX_VALUE_LEN) { + return false; + } + if (!tz_buffer_installed) { + // Install a full-width placeholder first. Some libc implementations + // copy the environment string instead of keeping our static buffer, and + // installing just "TZ=" would leave too little storage for later + // in-place updates. + memset(tz_buffer + 3, ' ', TZ_MAX_VALUE_LEN); + tz_buffer[3 + TZ_MAX_VALUE_LEN] = '\0'; + if (putenv(tz_buffer) != 0) { + return false; + } + tz_buffer_installed = true; + tz_env_value = getenv("TZ"); + if (tz_env_value == NULL) { + tz_env_value = tz_buffer + 3; + } + } + memcpy(tz_env_value, tz, tz_len); + memset(tz_env_value + tz_len, 0, TZ_MAX_VALUE_LEN - tz_len); + tz_env_value[TZ_MAX_VALUE_LEN] = '\0'; + return true; +} + +#endif // AVM_TZ_SETENV_LEAKS + term nif_erlang_localtime(Context *ctx, int argc, term argv[]) { char *tz; @@ -1972,17 +2030,62 @@ term nif_erlang_localtime(Context *ctx, int argc, term argv[]) smp_spinlock_lock(&ctx->global->env_spinlock); #endif if (tz) { - char *oldtz = getenv("TZ"); + char *oldtz = NULL; + char *oldtz_env = getenv("TZ"); + if (oldtz_env) { + oldtz = strdup(oldtz_env); + if (UNLIKELY(oldtz == NULL)) { +#ifndef AVM_NO_SMP + smp_spinlock_unlock(&ctx->global->env_spinlock); +#endif + free(tz); + RAISE_ERROR(OUT_OF_MEMORY_ATOM); + } + } + +#if AVM_TZ_SETENV_LEAKS + if (!set_static_tz_value(tz)) { +#ifndef AVM_NO_SMP + smp_spinlock_unlock(&ctx->global->env_spinlock); +#endif + free(oldtz); + free(tz); + RAISE_ERROR(BADARG_ATOM); + } +#else setenv("TZ", tz, 1); +#endif tzset(); localtime = localtime_r(&ts.tv_sec, &storage); + if (oldtz) { +#if AVM_TZ_SETENV_LEAKS + if (!set_static_tz_value(oldtz)) { + setenv("TZ", oldtz, 1); + tz_env_value = getenv("TZ"); + } +#else setenv("TZ", oldtz, 1); +#endif + free(oldtz); } else { +#if AVM_TZ_SETENV_LEAKS + // Cannot truly unset TZ with the static buffer approach; + // putenv does not support removal. + // NOTE: This is a pragmatic approximation, not a true restore. + // When TZ was originally unset, setting it to "UTC0" permanently + // changes observable process state (getenv("TZ") will return + // "UTC0" instead of NULL). This is acceptable on newlib/picolibc + // embedded targets where unset TZ already defaults to UTC. + if (!set_static_tz_value("UTC0")) { + unsetenv("TZ"); + } +#else unsetenv("TZ"); +#endif } + tzset(); } else { - // Call tzset to handle DST changes tzset(); localtime = localtime_r(&ts.tv_sec, &storage); } @@ -1991,6 +2094,11 @@ term nif_erlang_localtime(Context *ctx, int argc, term argv[]) #endif free(tz); + + if (UNLIKELY(localtime == NULL)) { + RAISE_ERROR(BADARG_ATOM); + } + return build_datetime_from_tm(ctx, localtime); }