[pulse] pass C variadic arguments to va_arg through globals by SuryanshSS1011 · Pull Request #2078 · meta/infer

SuryanshSS1011 · 2026-07-13T07:03:53Z

Fixes #1937

Problem

Pulse drops the extra arguments of a call to a C variadic function (trim_actuals_if_var_arg) and __builtin_va_arg had no model, so a value read with va_arg was not connected to the caller's argument. Writing through a variadic out-parameter, as in char **p = va_arg(args, char **); *p = malloc(n);, produced a false MEMORY_LEAK_C in the callee and a false PULSE_UNINITIALIZED_VALUE on the caller's variable, and real bugs through variadic arguments were missed.

Fix

When a summary of a C variadic function is applied, the nth variadic actual is bound to a global __infer_va_args_global_n in the caller's post, and the previous binding is restored after the call. A new va_arg model reads that global at the va_list cursor when the cursor is a known constant (and returns a fresh value otherwise), so the callee's pre mentions the globals and the usual summary application substitutes them with the actuals. va_start and va_copy maintain the cursor.

Tests

var_arg.c covers single, multiple and looped out-parameters, va_copy, a variadic call nested between va_start and va_arg, the same address passed twice, and genuine bugs (missing free, uninitialized read through an out-parameter, NULL out-parameter, unchecked allocation failure). The existing FP_sum_then_unreachable_npe_ok is now fixed.
specialization.c adds call_test_global_alias_bad and call_test_global_alias_good.
Known limitation, a va_list passed as a parameter, is marked FP_va_list_out_param_ok.

SuryanshSS1011 · 2026-08-06T14:39:52Z

@dulmarod @davidpichardie a small nudge on this one. Just rebased onto current main and all checks are green.

The change models C variadic out-parameters so Pulse stops reporting the MEMORY_LEAK_C false positive on the va_arg(a, char**); *p = malloc(...) idiom, riding the existing specialization mechanism rather than adding a name-based matcher. It is 223 lines across 7 files and scoped to the leak only. The PULSE_UNINITIALIZED_VALUE half of #1937 needs caller-actual binding that specialization cannot carry, so those tests stay marked FP_ and I described what a real fix would take on the issue.

Would love your read on this!

jvillard

Hi @SuryanshSS1011, sorry for the delay. Using specialisation for this is a nice direction! I'm a bit nervous about the impact on new uninitialized reads FPs. Maybe this isn't very far from solving them though, see my inline comment on PulseModelsC: If you were to write the actual values to the global(s) before the call in PulseCallOperations then pulse should do the rest for you.

Globals are visited from their address in materialize_pre_for_globals but their heap paths were built with LazyHeapPath.from_pvar, which adds the Dereference used for formals, and initialize_heap_path did not find globals in the initial stack. The aliasing assumption of the specialized summary was then put on the wrong values, so the specialized summary was unsat for the caller and the path was dropped, as in the new call_test_global_alias_bad where the null dereference was missed.

The variadic arguments of a call to a C variadic function were dropped when applying its summary and va_arg was not modelled, so writes through variadic out-parameters were lost (false MEMORY_LEAK_C and PULSE_UNINITIALIZED_VALUE, see meta#1937) and bugs through variadic arguments were missed. Bind the nth variadic argument to the global __infer_va_args_global_n in the caller for the duration of the call and model va_arg to read the global at the va_list cursor when it is known, so that applying the summary substitutes the callee values read by va_arg with the actual arguments. Also model va_start and va_copy.

SuryanshSS1011 · 2026-10-03T23:12:20Z

Thanks @jvillard for the review and suggestions! The rework worked out much better than I hoped and made this a lot easier. The caller now passes each variadic argument through a global that va_arg reads, so both the leak and the uninitialized-value false positives are gone, and it also catches some real bugs that main misses, like a NULL or uninitialized variadic out-parameter.

The first commit fixes a pre-existing bug this depends on. Aliasing specialization didn't work for heap paths rooted at globals, so the path was silently dropped, which also happens on plain C code (see call_test_global_alias_bad). Happy to split it into its own PR if you prefer.

The C pulse suite changes only in var_arg.c and specialization.c, the other C and C++ suites pass, and all checks are green.

jvillard · 2026-11-04T18:53:03Z

This is very nice and elegant now, thank you @SuryanshSS1011!

…ents of `...` A hidden return parameter is appended to both formals and actuals. Variadic arguments precede that actual, so truncating to a prefix bound the first extra argument to the return parameter. An integer zero could then trigger a false null dereference of the compiler-generated destination. ```cpp std::string make_value(const char* text, ...) { return std::string(text); } std::string f() { return make_value("value", 0); } // zero was bound to the hidden return parameter ``` Retain the named actuals and the last actual for variadic callees with an added return parameter. Other callees are unchanged. Limitations: `...` arguments remain unanalysed. Reports caused solely by the old mis-binding, including a null pointer as the first extra argument, disappear; those were attributed to the return parameter, not a principled variadic-pointer check. Full regression suite. Zero, later-zero, multiple-zero, nonzero, unknown and no-vararg controls stay silent; direct and declared-formal nulls still report. Matched corpus removes one re2 false positive and adds none. No existing expectations removed. No-Adapted to upstream meta#2078 (also fixes bind_variadic_actuals for return-param calls): meta#2078 binds the arguments of `...` to globals by dropping a prefix of the actuals, which counts the return parameter's address as one of them and shifts every argument by a position. Both the trim and the binding now derive the split from one `split_variadic_actuals`, so they cannot disagree. New regression test `c/pulse/variadic_return_param.c` covers both halves: no false null dereference when the first `...` argument is the integer zero, and `va_arg` still reading the first `...` argument rather than the return parameter. Upstream-PR: not yet submitted

A compound literal `(T){x}` was translated to the address of the temporary holding it, so the parent of `(int){1} << 0` shifted a stack address, and the vector literal an SSE set intrinsic returns, `return (__m128){w, 0, 0, 0}`, returned one. Pulse then reported STACK_VARIABLE_ADDRESS_ESCAPE on the intrinsic and on every scalar literal used as a value. In C the literal is an lvalue, `&(int){1}` is legal, and a use as a value carries its own `LValueToRValue` cast that already loads; in C++ the literal is a prvalue and taking its address is ill-formed, so nothing wanted the address. The literal now loads the value it stored whenever it is a prvalue of a type SIL holds by value, which is what the C path already did. // reported before, on the intrinsic itself static __inline__ __m128 _mm_set1_ps(float w) { return __extension__ (__m128){w, w, w, w}; } Limitations: an lvalue literal is unchanged, so C keeps its address semantics; records and arrays are unchanged, SIL holds those by address. `make -C infer/tests/codetoanalyze/cpp/pulse test` and the C and C++ cases. New: three `_ok` returns in `returns.cpp` plus a control that the value still compares equal to itself, and in `compound_literal.c` a C `&(int){1}` escape that is still reported. Expected test output regenerated for the combination with earlier frontier commits and with upstream meta#2078: infer/tests/codetoanalyze/c/pulse/issues.exp. This file is the last place in the series that touches those expectations, so the regenerated state lands here; the entries themselves come from upstream's new var_arg.c and specialization.c cases, from `variadic_return_param.c` added earlier in the series, and from the removal of `FP_sum_then_unreachable_npe_ok`, which upstream meta#2078 deleted. No-Upstream-PR: not yet submitted