Potential deadlock when putenv fails

[matyhtf]

Description

https://github.com/php/php-src/blob/master/ext/standard/basic_functions.c#L846

The else branch of the putenv function does not call tsrm_env_unlock() to release the lock, which could potentially lead to a deadlock.

PHP Version

PHP 8.4

Operating System

Ubuntu 22.04

[cmb69]

Needs to be fixed for PHP-8.3+.

[matyhtf]

Hi cmb69, thanks for your reply.

Another issue is that the implementation of putenv with ZTS may have a bug. Please take a look.

The environ is a global array, if multiple threads concurrently execute putenv with the same key, the pe.previous_value pointer may point to a memory address that has already been freed.

The putenv C function does not copy the string that is passed in; instead, it directly stores the pointer in environ.

Thread 1:

RINIT
putenv(env1)
pe.previous_value = NULL
// ... more code 

Thread 2:

env1 and env2 have the same key

RINIT
putenv(env2)
pe.previous_value = env1

Thread 1:

RSHUTDOWN
// php_putenv_destructor()
free(pe->putenv_string) // free env1

Thread 2:

RSHUTDOWN
// php_putenv_destructor()
// pe->previous_value is env1
putenv(pe->previous_value) // This memory address has already been freed

[nielsdos]

I understood the deadlock problem and made a PR for it.
I didn't understand the race condition. I don't understand how freeing putenv_string can cause a UAF on previous_value. I also note that grabbing and resetting previous_value happens under the lock.

[SakiTakamachi]

Since the putenv_entry structure is managed by BG(), in the case of ZTS, each thread should have its own value. Therefore, I don't think it will become a UAF.
Did that actually happen? If so, I may have missed something.

php-src/ext/standard/basic_functions.c

Line 403 in 54e662c

zend_hash_init(&BG(putenv_ht), 1, NULL, php_putenv_destructor, 0);