Merge ~info-martin-konrad/epics-base:dont-nuke-global-cac-thread-id-in-exit-handler into ~epics-core/epics-base/+git/epics-base:3.15

[Martin Konrad]

[mdavidsaver (mdavidsaver)]

epicsThreadPrivateDelete() has invalided the storage pointed to by caClientCallbackThreadId. Wouldn't this change be replacing a NULL dereference with a use after free()?

review: Needs Fixing

[Martin Konrad (info-martin-konrad)]

Oops, of course you're right. I guess taking out the epicsThreadPrivateDelete() call is not really an option either because then the object is leaked...

I guess the right approach would be to use a shared pointer to get it cleaned up when it's not used anymore. But that would obviously break the interface (not sure if we can avoid this here, though). Any thoughts?

[mdavidsaver (mdavidsaver)]

Yes, ref. counting would probably be the best way to avoid an exit handler here. I think this could be done with an atomic inc. in ca_context_create() and dec. in ca_context_destroy().

And then I notice that caClientContextIdOnce in access.cpp is already being leaked... So maybe just remove "cacExitHandler()"? This would be far from the only one-time leak in Base.

[Andrew Johnson (anj)]

RTEMS and VxWorks both use the same basic implementation of epicsThreadPrivate variables for which the delete function does nothing:

> void epicsThreadPrivateDelete (epicsThreadPrivateId id)
> {
> /* empty */
> }

I thought that we used this code everywhere, (I'd forgotten that we even provided a Delete() function for the API) but I was obviously wrong; both the Posix and Windows implementations /are/ different and rely on the OS to allocate IDs. On Posix reusing a pthread_key_t value after deleting it results in undefined behavior, but I suspect that in practice the storage in any other threads doesn't get deleted immediately, which is why Bruno's test worked.

The rationale section in the Linux pthread_key_delete() man-page describes how fraught with difficulty deleting the ID/key of a thread-private variable is in practice, and in Base most code doesn't delete the ID at all. Both the 3.15 and 7.0 branches contain 10 calls to epicsThreadPrivateCreate() and only 2 calls to epicsThreadPrivateDelete(), one of which is in the destructor for the C++ epicsThreadPrivate<T> template class (which only seems to be used in test programs), the other is here in CA's cacExitHandler().

I agree with Michael that removing the cacExitHandler() is probably the best solution. My previous commit already added a Release Note, the wording of which still applies, so it would make sense to modify this merge to finish off the necessary changes.

review: Needs Fixing

[Martin Konrad (info-martin-konrad)]

Thanks for your input! I amended my commit to eliminate cacExitHandler().

[mdavidsaver (mdavidsaver)]

I think that either the "caClientCallbackThreadId = 0;" needs to be restored, or cacExitHandler() completely removed.

Also, the epicsAtExit() callback ordering in pyDevSup has separately been addressed.

https://github.com/mdavidsaver/pyDevSup/pull/20

[Andrew Johnson (anj)]

@MAD: Huh?!

@Martin: You missed the function declaration for cacExitHandler() that appears in one of the CA header files. Could you also rebase this branch against the latest 3.15 commit so this Merge doesn't show a conflict, which I suspect is what's confusing Michael.

[mdavidsaver (mdavidsaver)]

Ah, yes. The MR diff is total wrong. Way to go LP. b905218cdccf1618c5c685063267564edc94f27f looks fine.

> You missed the function declaration for cacExitHandler() that appears

Also the 'friend' line in oldAccess.h. Looks fine once this is done.

[Martin Konrad (info-martin-konrad)]

I removed the declaration and the "friend" line and rebased.

[mdavidsaver (mdavidsaver)]

Looks ok now.

review: Approve

[mdavidsaver (mdavidsaver)]

Andrew, are there still changes you want?

[Ralph Lange (ralph-lange)]

Looks fine to me.

review: Approve

[Andrew Johnson (anj)]

Clean out this old disapproval, I originally approved this proposal on 5/20.

review: Approve

diff --git a/src/ca/client/ca_client_context.cpp b/src/ca/client/ca_client_context.cpp
index 95cd6cb..ce8de1e 100644
--- a/src/ca/client/ca_client_context.cpp
+++ b/src/ca/client/ca_client_context.cpp
@@ -45,19 +45,12 @@ static epicsThreadOnceId cacOnce = EPICS_THREAD_ONCE_INIT;
 
 const unsigned ca_client_context :: flushBlockThreshold = 0x58000;
 
-extern "C" void cacExitHandler ( void *)
-{
-    epicsThreadPrivateDelete ( caClientCallbackThreadId );
-    delete ca_client_context::pDefaultServiceInstallMutex;
-}
-
 // runs once only for each process
 extern "C" void cacOnceFunc ( void * )
 {
     caClientCallbackThreadId = epicsThreadPrivateCreate ();
     assert ( caClientCallbackThreadId );
     ca_client_context::pDefaultServiceInstallMutex = newEpicsMutex;
-    epicsAtExit ( cacExitHandler,0 );
 }
 
 extern epicsThreadPrivateId caClientContextId;
diff --git a/src/ca/client/oldAccess.h b/src/ca/client/oldAccess.h
index 04f5518..daa4cc9 100644
--- a/src/ca/client/oldAccess.h
+++ b/src/ca/client/oldAccess.h
@@ -288,7 +288,6 @@ private:
 };
 
 extern "C" void cacOnceFunc ( void * );
-extern "C" void cacExitHandler ( void *);
 
 struct ca_client_context : public cacContextNotify
 {
@@ -428,7 +427,6 @@ private:
     ca_client_context & operator = ( const ca_client_context & );
 
     friend void cacOnceFunc ( void * );
-    friend void cacExitHandler ( void *);
     static cacService * pDefaultService;
     static epicsMutex * pDefaultServiceInstallMutex;
     static const unsigned flushBlockThreshold;