After a fork do not release resources shared with parent

[nmav]

This resolves the issue of C_Login returns CKR_GENERAL_ERROR after a fork.
https://sourceforge.net/p/opensc/mailman/message/34073754/

[frankmorgner]

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

[frankmorgner]

following my suggestion above, you would

1. keep your changes of pkcs11-global.c
2. change the initialization in the PC/SC reader to save the old pid in gpriv
3. check the old pid against the current pid when finishing

That would be it. No other reader driver needs to be touched and the ABI stays the same.

[nmav]

I thought about keeping the ABI stable, but does it matter at all? Is that really an ABI, or just an internal library?

[frankmorgner]

this also leaks memory in the forked process...

[frankmorgner]

card_removed also frees some internal resources that should be freed in the forked process

[frankmorgner]

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:https://programmers.stackexchange.com/questions/147977/is-it-wrong-to-use-a-boolean-parameter-to-determine-behavior) to:

• use a proper call back "terminate" function in the reader ops
• add a flag to sc_context_t which says that no further operations should be performed (e.g. "terminated"). sc_context_t has already threading information, you might as well add infos about the process...
• solve the issue locally by evaluating the "fork" state multiple times (see After a fork do not release resources shared with parent #493 (comment))

Note that you need to rework C_Finalize (and the operations it performs), too, to be "fork-safe"

[nmav]

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:https://programmers.stackexchange.com/questions/147977/is-it-wrong-to-use-a-boolean-parameter-to-determine-behavior) to:

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.

solve the issue locally by evaluating the "fork" state multiple times (see #493 (comment))

I don't believe this is nice programming. Duplicating code is almost always bad.

Note that you need to rework C_Finalize (and the operations it performs), too, to be "fork-safe"

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).

[frankmorgner]

@nmav could you have a look at #494 ?