Potential deadlock when putenv fails #17403

Activity

1. matyhtf

   added

   Bug

   Status: Needs Triage

   on Jan 8, 2025
2. cmb69

   added

   Extension: standard

   Status: Verified

   and removed

   Status: Needs Triage

   on Jan 8, 2025
3. cmb69 commented on Jan 8, 2025

   

   cmb69

   on Jan 8, 2025

   Member

   More actions

   Needs to be fixed for PHP-8.3+.
4. matyhtf commented on Jan 8, 2025

   

   matyhtf

   on Jan 8, 2025

   Last edited by matyhtf

   ContributorAuthor

   More actions

   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
5. Girgias

   added the

   Bug

   issue type on Feb 5, 2025
6. ndossche

   changed the title [-]The lock is not released when putenv fails.[/-] [+]Potential deadlock when putenv fails[/+] on Apr 20, 2025
7. ndossche

   added a commit that references this issue on Apr 20, 2025

   Fix phpGH-17403: Potential deadlock when putenv fails

   This issue will close once commit 37afae0 is merged into the 'master' branch.

   37afae0
8. ndossche commented on Apr 20, 2025

   

   ndossche

   on Apr 20, 2025

   Member

   More actions

   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.
9. ndossche

   linked a pull request that will close this issueFix GH-17403: Potential deadlock when putenv fails #18368on Apr 20, 2025
10. SakiTakamachi commented on Apr 24, 2025

    

    SakiTakamachi

    on Apr 24, 2025

    Last edited by SakiTakamachi

    Member

    More actions

    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);
11. ndossche

    added 2 commits that reference this issue on Apr 24, 2025

    Fix GH-17403: Potential deadlock when putenv fails

    ...

    This issue will close once commit 32e0912 is merged into the 'master' branch.

    32e0912

    Merge branch 'PHP-8.3' into PHP-8.4

    ...

    This issue will close once commit c6ed104 is merged into the 'master' branch.

    c6ed104
12. ndossche

    closed this as completedin 32e0912on Apr 24, 2025
13. ndossche

    added a commit that references this issue on Apr 24, 2025

    Merge branch 'PHP-8.4'

    ...

    This issue will close once commit c0da1bc is merged into the 'master' branch.

    c0da1bc