Close file_handle in fpm_main

[nielsdos]

The file handle wasn't destroyed/closed. I don't know if this really matters all too much but it seems to me that it should be destroyed, just like is the case for the cgi sapi.

[bukka]

I might be missing something but from the code this seems to me like a serious leak that this is fixing. If I read it correctly, the filename and file fd is leaked on every request. We might need to do some testing to confirm it as I can't believe this could be happening for so long without noticing.

[nielsdos]

I didn't use a tool or anything to find this. I just stumbled upon this and saw there was no freeing. Do you have suggestions on how to test it reliably?

[bukka]

Running FPM with 1 pool with static pm and one child and the hit it with mulitple requests. Just GDB (requires setting set follow-fork-mode child) might show if it's getting freed or maybe trying to use Valgrind or other tools might show it as well. I had some issues with Valgrind in past when trying to debug child but it was for callgrind. Memcheck might work though.

[nielsdos]

I don't see an increase in entries in /proc/<pid of FPM>/. I guess this is due to the file handle using a stream instead of a FILE*, but I'm not sure to how it all works internally and I didn't look that far into it tbh.

But I do see a memory leak report even without using Valgrind, but only when using opcache starting from the second request to the same file. The script name is being leaked (at least) every time I reload the webpage. Possibly other resources (that aren't being reported?) could be leaked as well, idk for sure. When I apply this PR I no longer see the leak.

That leak looks like:

[Sun Mar 12 00:54:15 2023]  Script:  '-'
/home/niels/php-src/Zend/zend_string.h(150) :  Freeing 0x00007fbaca65f4e0 (64 bytes), script=-

[bukka]

I just noted in the main comment some reasoning for this. Just to answer why the /prod/<child_pid>/fd is not increasing - it is because with opcache, there is no fd because accelerator overrides zend_stream_open_function and does not create any fd in file handle. And without opcache it is being freed in shutdown as I mentioned.

[nielsdos]

Updated such that only the success path destroys the file handle.

[bukka]

I have done some debugging and it is actually slightly more complicated. The thing is that without opcache, the file is added to CG(open_files) which is added in open_file_for_scanning:

#0  open_file_for_scanning (file_handle=0x7fffffffe070) at Zend/zend_language_scanner.l:532
#1  0x0000555555bda669 in compile_file (file_handle=0x7fffffffe070, type=8) at Zend/zend_language_scanner.l:646
#2  0x00005555558d34f1 in phar_compile_file (file_handle=0x7fffffffe070, type=8) at /home/jakub/prog/php/81/ext/phar/phar.c:3360
#3  0x0000555555c55bbd in zend_execute_scripts (type=8, retval=0x0, file_count=3) at /home/jakub/prog/php/81/Zend/zend.c:1840
#4  0x0000555555b9877c in php_execute_script (primary_file=0x7fffffffe070) at /home/jakub/prog/php/81/main/main.c:2542
#5  0x0000555555dec7d0 in main (argc=4, argv=0x7fffffffe4f8) at /home/jakub/prog/php/81/sapi/fpm/fpm/fpm_main.c:1919

Then it is freed inside php_request_shutdown :

#0  zend_file_handle_dtor (fh=0x7ffff525e0b0) at /home/jakub/prog/php/81/Zend/zend_stream.c:215
#1  0x0000555555c3db95 in zend_llist_destroy (l=0x555556c3c158) at /home/jakub/prog/php/81/Zend/zend_llist.c:109
#2  0x0000555555d1dbcf in zend_stream_shutdown () at /home/jakub/prog/php/81/Zend/zend_stream.c:287
#3  0x0000555555c38aca in shutdown_executor () at /home/jakub/prog/php/81/Zend/zend_execute_API.c:404
#4  0x0000555555c538a6 in zend_deactivate () at /home/jakub/prog/php/81/Zend/zend.c:1277
#5  0x0000555555b96fee in php_request_shutdown (dummy=0x0) at /home/jakub/prog/php/81/main/main.c:1848
#6  0x0000555555dec9fe in main (argc=4, argv=0x7fffffffe4f8) at /home/jakub/prog/php/81/sapi/fpm/fpm/fpm_main.c:194

That explains why you don't see the issue with opcache disabled.

With opcache it obviously does not scan the file on subsequent executions so there is a leak. So it makes sense to do the destroy but maybe in slightly more optimized way (adding new suggestiong).

[nielsdos]

Thanks for the extensive debugging and explanation, very interesting issue actually.
I'll merge your suggestion into my commit soon-ish, and if it's okay I'll add a Co-authored-by tag for you. I think I'll also add a comment explaining why we do the if check.

[nielsdos]

Force pushed the suggestion, and added a comment, and added your co-authored-by. Thanks alot!