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

Broken curl_global_init/curl_global_cleanup ref-counting #4636

Closed
pgroke-dt opened this issue Nov 25, 2019 · 2 comments
Closed

Broken curl_global_init/curl_global_cleanup ref-counting #4636

pgroke-dt opened this issue Nov 25, 2019 · 2 comments
Labels

Comments

@pgroke-dt
Copy link

@pgroke-dt pgroke-dt commented Nov 25, 2019

I did this

I checked the curl_global_init/curl_global_cleanup source code to find out if I was supposed to call curl_global_cleanup after getting a non-successful return code from curl_global_init.

I expected the following

I expected to find to find an implementation that expects me to not call curl_global_cleanup after a failed init attempt, because that's the "usual way to do it" (all or nothing, you can't "release" what you didn't "acquire"). OR an implementation that expects me to call curl_global_cleanup, but at least keeps track of the initialization state, so that only the parts that were really initialized in the failed init attempt are cleaned up.

What I found

The initialized counter is immediately incremented, before any initialization is attempted. After that there are multiple error-exit points, none of which reset the initialized counter. After that the library is in a state where

  • It cannot be used, since it wasn't initialized completely. This is OK, after all the init function returned an error.
  • Calling curl_global_init again will immediately "succeed", but not change anything. This is bad, because if the library is used in a module/plugin/..., another part of the application may call curl_global_init, get the "success" return code, think all is fine and go ahead to try to use it.
  • Calling curl_global_cleanup will restore the initialized counter back to zero, but it may also call cleanup functions of other libraries who's corresponding init function was not even called (or at least not successfully). This again is bad in a module/plugin/..., because if the module that tried to initialize curl calls curl_global_cleanup to try to "undo" the damage to the initialized counter, curl may call the cleanup function of one of its dependencies, without having initialized it, thereby potentially undoing the initialization of this dependency that some other module may have done before.

curl/libcurl version

Current master, and blame says it's been this way forever (more than 10 years).

operating system

All

@bagder

This comment has been minimized.

Copy link
Member

@bagder bagder commented Nov 25, 2019

Confirmed. But then again, when init fails there's really no given mitigation for an application so I presume very few user ever actually try to call it again and expect it to work...

@bagder bagder added the libcurl API label Nov 25, 2019
@pgroke-dt

This comment has been minimized.

Copy link
Author

@pgroke-dt pgroke-dt commented Nov 26, 2019

Well, the "GLOBAL CONSTANTS" section of https://curl.haxx.se/libcurl/c/libcurl.html goes into detail explaining how to initialize libcurl in an application that uses modules/plugins/... Exactly the use case described there will be problematic with the current implementation, because the second module trying to initialize libcurl will see a spurious success code.

So I guess the true reason this isn't a problem is that initialization very rarely fails (if at all), except for when it's a configuration problem (e.g. a required dependency missing on the system). And in that case (configuration problem), most computer users I know simply accept any kind of misbehavior from an application, even including crashes.

However for our use case that's not an acceptable solution.

bagder added a commit that referenced this issue Nov 28, 2019
... so that failures in the global init function don't count as a
working init and it can then be called again.

Reported-by: Paul Groke
Fixes #4636
bagder added a commit that referenced this issue Nov 28, 2019
... so that failures in the global init function don't count as a
working init and it can then be called again.

Reported-by: Paul Groke
Fixes #4636
@bagder bagder closed this in bc5d22c Dec 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.