[PATCH 2/2] stdlib: Re-implement free (environ) compatibility kludge for setenv

[Carlos O'Donell]

On 1/24/25 3:12 PM, Andreas K. Huettel wrote:
> Am Freitag, 24. Januar 2025, 11:22:41 Mitteleuropäische Normalzeit schrieb Florian Weimer:
>> For the originally failing application (userhelper from usermode),
>> it is not actually necessary to call realloc on the environ
>> pointer.  Yes, there will be a memory leak because the application
>> assigns a heap-allocated pointer to environ that it never frees,
>> but this leak was always there: the old realloc-based setenv had
>> a hidden internal variable, last_environ, that was used in a similar
>> way to __environ_array_list.  The application is not impacted by
>> the leak anyway because the relevant operations do not happen in
>> a loop.
>>
>> The change here just uses a separte heap allocation and points
>> environ to that.  This means that if an application calls
>> free (environ) and restores the environ pointer to the value
>> at process start, and does not modify the environment further,
>> nothing bad happens.
> 
> OK so I *think* I understand what you are doing here. In the intermediate
> version the array was not at the starting point of a memory allocation
> (but at the end of struct environ_array), meaning a free (outside libc)
> on the address of the array led to havoc. Now the array is a separate
> allocation with a pointer to it at the start of struct environ_array.
> (See also notes below.)
> 
> This makes sense to me, but I'm not really good enough for a R-b, please
> get that from someone else.
> 
> That said, we're reaching the point now where additional fix-ups are
> very late.
> 
> * Has the adressed problem been observed anywhere else *except* 
>   userhelper?

The alternative solution is an entire revert of the broader changes, which I don't
think is required.

With this fix in place we now can handle:

(a) environ set to heap allocated value that the application expects to call free on.

(b) environ set to a non-heap allocated value, which we shouldn't assume is heap
    allocated.

This ensures maximum backwards compatibility.
 
> * Any other ideas to estimate possible impact?

I think this covers all the cases I could imagine.

The implementation is making no assumptions about the incoming environ pointer.

The implementation is retaining backwards compatibility by making sure the outgoing
environ pointer can be freed with free().

> * In case this has no wider impact, would you be OK with backporting the
>   fix after the release?

My preference is that with an identified defect we should work to include the fix in
the release.

This issue effectively becomes a known blocker and we delay to resolve it with as little
risk as possible.

The alternative is a complete backout of Florian's other changes, but we have very
positive downstream indications that they resolve the graphics stack issues.

> We can make the final decision on monday, but that's cutting it fine.

Agreed, and that's always going to be the case with release blocking bugs.

-- 
Cheers,
Carlos.