Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OS_LibShutdown is not called #32

Open
Louson opened this issue Oct 21, 2019 · 10 comments
Open

OS_LibShutdown is not called #32

Louson opened this issue Oct 21, 2019 · 10 comments

Comments

@Louson
Copy link

Louson commented Oct 21, 2019

In unix at least :
fcgiapp has a call to OS_LibInit() that allocates a structure (asyncIoTable) which is never freed.
I guess OS_LibShutdown should be called in FCGX_Finish().

@Louson
Copy link
Author

Louson commented Oct 21, 2019

See pull request #33

@mcarbonneaux
Copy link
Member

i'not sure is usefull to do that, because is call in FCGIexit only when exit.

in FCGX_Accept is calling FCGX_Init only if libInitialized is initialized... only at the first request...

if you call at every FCGX_Finish you force at each FCGX_Accept to reinit at each Accept...

and os init activate the signal handler and i'm not sure they are correctly coded to reenabled at each accept...

@LeSpocky
Copy link

I can confirm the initial analysis. I'm getting this with Valgrind on my app running on embedded Linux with spawn-fcgi:

==818== Memcheck, a memory error detector
==818== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==818== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
==818== Command: /var/www/htdocs/cgi-bin/iswebgui.cgi
==818== 
==818== 
==818== HEAP SUMMARY:
==818==     in use at exit: 448 bytes in 1 blocks
==818==   total heap usage: 16,925 allocs, 16,924 frees, 16,994,819 bytes allocated
==818== 
==818== 448 bytes in 1 blocks are still reachable in loss record 1 of 1
==818==    at 0x4841B04: calloc (vg_replace_malloc.c:752)
==818==    by 0x48A4BCF: OS_LibInit (os_unix.c:171)
==818==    by 0x48A4085: FCGX_Init (fcgiapp.c:2087)
==818==    by 0x48A40F1: FCGX_IsCGI (fcgiapp.c:1946)
==818==    by 0x48A4365: FCGI_Accept (fcgi_stdio.c:120)
==818==    by 0x10D415: main (iswebgui.c:100)
==818== 
==818== LEAK SUMMARY:
==818==    definitely lost: 0 bytes in 0 blocks
==818==    indirectly lost: 0 bytes in 0 blocks
==818==      possibly lost: 0 bytes in 0 blocks
==818==    still reachable: 448 bytes in 1 blocks
==818==         suppressed: 0 bytes in 0 blocks
==818== 
==818== For counts of detected and suppressed errors, rerun with: -v
==818== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

The code in main() is like an ordinary CGI, extended with FCGI stuff like this:

#include <fcgi_stdio.h>
…
void main( void )
{
    while ( FCGI_Accept() >= 0 )
    {
        /* all that classic CGI stuff */
    }
    FCGI_Finish();
    return 0;
}

@mcarbonneaux
Copy link
Member

mcarbonneaux commented May 30, 2020

but FCGX_Accept is calling FCGX_Init only if libInitialized is initialized...
not at each Accept...

in fcgiapp.c :

if (! libInitialized) {
        rc = FCGX_Init();
        if (rc) {
            return rc;
        }
    }

only one time along the live of the process...
and i think not need to be release at finish because you can reopen FCGI before exit...
eventualy when exit... you can explicitly call os_shutdown.... but the majority of what done in shutdown are automaticly release by the end of the process (when call FCGIexit)...
ok is not very clean... but it's work...

@xdevelnet
Copy link

IMO executing OS_LibShutdown() right before return EXIT_SUCCESS; from process is the only correct solution for now. Unfortunately this is required in some cases like CI Valgrind integraiton, which yells for every single memory leak.

It would be great if OS_LibShutdown() would be exported to fcgiapp.c/h to something like FCGX_Deinit() 🙂

@mcarbonneaux
Copy link
Member

FCGIexit is for that i think... they call OS_LibShutdown()....

@Louson
Copy link
Author

Louson commented Nov 7, 2021

FCGIexit does call OS_LibShutdown, but it also terminates the process when calling exit().

It would be great to have a similar function in declared in fcgiapp.h that returns a code without exiting the process.

In my case, I'm using fcgi in a side library along my main process, I am not creating a process for it.

@ThomasDevoogdt
Copy link

What is the current status of this topic? I agree to have OS_LibShutdown exported somewhere.
The initial commit 217069d makes sense since FCGX_Init and FCGX_Finish are opposites, and thus easy to understand. FCGIexit is not exported, and perhaps also not desirable because applications might want to have control of the exit by themselves. Any reason to not re-open that PR and accept this commit?

@Louson
Copy link
Author

Louson commented Oct 10, 2022

On my side, I decided to fix it by freeing the memory manually :

  FCGX_Free(&request, 0);
  OS_LibShutdown();

This implies to include fcgios.h

I am not sure this issue can have an easy fix. I understand that my commit could cause some regression. I suggest the issue remains open with this hack.

Here is an example https://gitlab.com/louson.fr/grav/-/blob/master/pages/03.papers/connect-your-firmware-with-fastcgi/fcgi-server.c

@mcarbonneaux
Copy link
Member

mcarbonneaux commented Oct 10, 2022

FCGX_Free(&request, 0);

normaly FCGX_Finish free the request data at fcgiapp.c:2045 but leave the connection open if request->keepConnection are true (depend on the FCGI_KEEP_CONN flag send from fcgi server).

This implies to include fcgios.h

effectively the OS_LibShutdown are exposed by this include at fcgios.h:104 and possibly can be called explicitly in that way.

FCGIexit does call OS_LibShutdown, but it also terminates the process when calling exit().

FCGIexit must be used in place of exit() is there role.

if fact idealy is to call FCGIexit event if the exit status are 0 (you must call FCGIexit(0) in this case), in this case OS_LibShutdown are systematicly call before exit.

Normaly you need to call OS_LibShutdown only before exit of the process. is the way is developped...

but effectively if you need to use in side library, you must call explicitly OS_LibShutdown in using fcgios.h include.

but you can call it at library shutdown to avoid to reinit each time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants