-
Notifications
You must be signed in to change notification settings - Fork 220
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
Use after free in handleConnection (cs104_connection.c:821) #61
Comments
I think this is not a problem of the library. It is beause you call the destructor (CS104_Connection_destroy) while being inside of a callback function of the object you want to delete. You shouldn't call CS104_Connection_destroy inside the callback function. |
This is a library problem, because there is no thread synchronizing between connection thread and user threads, and so right after flag for calling destructor was set in the callback function it might be read in user thread and destructor will be called while connection thread is still in handleConnection(), and illegal memory access will occur. |
What flag are you referring to? AFAICS the CS104_Connection_destroy function is waiting for the connection thread before it releases any memory. |
I'm referring for a (for example) bool flag passed as bool* pointer through void *parameter argument to the connectionHandler(). I could not reproduce this problem right now, but sometimes attached simple test program (simplified version of simple_client.c) calls connectionHandler twice for CS104_CONNECTION_CLOSED event:
And, this looks like the real issue, sorry for misleading. |
Just reproduced the issue with exaclty same test program, here is the Valgrind report:
|
Thanks for the example code. I see a problem with calling "CS104_Connection_close" inside the connecton handler (when stop dt is received). It shouldn't be called there because it is the function that waits for the connection handling thread to terminate (and is also called by CS104_Connection_destroy for this purpose). I guess it should be explicitely mentioned what functions can be called inside the callbacks and what functions cannot be called. For sure I could also add some synchronization code to allow the function to be called inside the callback. Yet I don't like the idea to blow up the synchronization code. |
I'd applied following patch to the sources of library:
And got following output:
So, it's possible to notify user about this problem, or even print message about invalid function calls from callbacks if connection thread will set an internal flag ("in_callback", for example) before entering callback (and clear this flag after callback functions returned) and check this flag inside "dangerous" functions, so user will be able to see and fix those issues. I'd seen some other places where return values of system functions was not properly checked, and i'm think those places potentially can cause such problems too. Of course, the patch above will cause memory leaks (which is a bit better than memory corruptions or segfaults, i think), so it can't completely replace correct fix for this problem (but can detect the problems). |
Hello.
Valgrind says:
This error may occur when connection was closed and user program set flag about this in connectionHandler() and then call CS104_Connection_destroy() on this connection fast enough.
Also, if user calls CS104_Connection_destroy() in connectionHandler() on CONNECTION_CLOSED event - following error occurs before cs104_connection.c:821 :
So, there is 2 memory errors at once (and one of them is 1-byte memory corruption).
The text was updated successfully, but these errors were encountered: