-
Notifications
You must be signed in to change notification settings - Fork 729
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
After a fork do not release resources shared with parent #493
Conversation
This resolves the issue of C_Login returns CKR_GENERAL_ERROR after a fork. https://sourceforge.net/p/opensc/mailman/message/34073754/
a less invasive approach would be to let reader-pcsc.c call getpid itself to check whether it should release the context or not. You would not have to change the ABI for that, only gpriv in reader-pcsc.c |
following my suggestion above, you would
That would be it. No other reader driver needs to be touched and the ABI stays the same. |
I thought about keeping the ABI stable, but does it matter at all? Is that really an ABI, or just an internal library? |
|
||
/* remove all cards from readers */ | ||
for (i=0; i < (int)sc_ctx_get_reader_count(context); i++) | ||
card_removed(sc_ctx_get_reader(context, i)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this also leaks memory in the forked process...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
card_removed
also frees some internal resources that should be freed in the forked process
Yes, ABI stability is not so important. However, the lib's version should be changed in the process. Instead of pushing a boolean parameter from top to bottom, I think it's cleaner (see also http://programmers.stackexchange.com/questions/147977/is-it-wrong-to-use-a-boolean-parameter-to-determine-behavior) to:
Note that you need to rework C_Finalize (and the operations it performs), too, to be "fork-safe" |
I don't quite agree with that advice. This is an internal API which can be changed very easily when needed (for example convert the boolean flag to a set of flags), thus it can be extended whereas using multiple functions can hardly be extended.
I don't believe this is nice programming. Duplicating code is almost always bad.
I'm not sure what you mean, it is already fork safe. Nevertheless, I've improved it a bit to reduce leaks, but let me state the scope of this change. It is to make opensc work when fork() is used in a program. There can be improvements, reduce the memory leaks (which exist even for the normal usage of opensc), but their fix requires a big change in almost all components involved. Most likely that will also introduce bugs, so I don't believe it should be part of this change set. Eliminating the memory leaks, is a valid goal (and I would really endorse) but should be part of a different patch set, and it should be addressed in both the single process case and the fork case (the latter cannot happen without the former). |
This resolves the issue of C_Login returns CKR_GENERAL_ERROR after a fork.
https://sourceforge.net/p/opensc/mailman/message/34073754/