On Thursday, September 08, 2016 14:55:12 K. Frank wrote:
> I have a suggestion for how you might synchronize your slot
> code for thread safety.
>
> First some disclaimers: I have no knowledge of the libcurl
> code base. (I have only used libcurl a little bit with a
> pre-compiled libcurl library.) I have only very superficial
> knowledge of pthreads.
>
> Some assumptions: libcurl uses pthreads for threading
> (at least on unix). You would prefer portable code over
> platform-specific code unless that would produce a significant
> performance bottleneck.

Thanks for the suggestion! This bug is already worked around in libcurl:

https://github.com/curl/curl/commit/3a5d5de9

... based on the following pull request:

https://github.com/curl/curl/pull/985

> Given this, I think there is no way to avoid adding synchronization
> code to both any code that modifies the slot structure and to any
> code that reads the slot structure.
>
> A mutex (pthread_mutex) will certainly do. You might get a little
> more efficiency using a reader-writer lock.
>
> Here is some pseudocode for one approach:
>
> struct slot_wrapper {
> pthread_rwlock_t *slot_rwlock;
> slot_t *slot;
> };
>
> slot_t *access_slot_for_reading (struct slot_wrapper *slot_wrapper) {
> pthread_rwlock_rdlock (slot_wrapper->slot_rwlock);
> if ( /* good */ )
> return slot_wrapper->slot;
> else /* bad */
> return NULL;
> }
>
> slot_t *access_slot_for_writing (struct slot_wrapper *slot_wrapper) {
> pthread_rwlock_wrlock (slot_wrapper->slot_rwlock);
> if ( /* good */ )
> return slot_wrapper->slot;
> else /* bad */
> return NULL;
> }
>
> void release_slot_from_both_reading_and_writing (struct
> slot_wrapper *slot_wrapper) {
> pthread_rwlock_unlock (slot_wrapper->slot_rwlock);
> }
>
> Then, for example, in the code fragment from your patch that
> modifies the slot:
>
> int retval = PR_NOTHING;
> slot = access_slot_for_writing (slot_wrapper);
> if (nssrv != PR_SUCCESS) {
> slot->token->base.name[0] = 0; /* XXX */
> slot->ckFlags &= ~CKF_TOKEN_PRESENT;
> slot->lastTokenPing = PR_IntervalNow();
> retval = PR_FALSE;
> }
> else {
> slot->lastTokenPing = PR_IntervalNow();
> retval = PR_TRUE;
> }
> release_slot_from_both_reading_and_writing (slot_wrapper);
> return retval;
>
> Some hypothetical pseudocode that uses the slot but doesn't
> modify it:
>
> slot = access_slot_for_reading (slot_wrapper);
> if (slot->ckFlags == some_special_flags) /* do stuff */;
> if (slot->lastTokenPing == whatever) /* do more stuff */;
> release_slot_from_both_reading_and_writing (slot_wrapper);
>
> Some comments:
>
> The slot_wrapper could clearly be overkill (depending on your
> taste in such things); slot_rwlock could be moved into the
> slot structure itself, and the calls to pthread_rwlock_wrlock(),
> etc., could be added directly to the code fragments that access
> the slots. (If you have many code fragments / functions that
> access the slot, the slot_wrapper might be worth the bother.
> If you have only one or two places that read or write the slot,
> I would probably just put the lock into the slot structure and
> put the pthread calls in-line where they are used.)
>
> Using the rwlock is only likely to be beneficial if there's a
> good chance that multiple threads will try to read the slot at
> the same time (and reads are more common than writes). On
> the other hand, there really isn't any downside to using a rwlock,
> so this seems like an overall sensible solution.
>
> Lastly, introducing the retval variable and moving the first
> return out of the if block was done so that there would be
> only one return statement, and therefore only one call to
> release_slot_from_both_reading_and_writing(). This is certainly
> not necessary, but it's perhaps a bit less error-prone than
> having to remember to unlock the rwlock separately for each
> return path.

I believe it would be better to take suggestions about the actual NSS fix
directly to the upstream Bugzilla:

https://bugzilla.mozilla.org/1297397

... since there are not many people that understand NSS internals on the
libcurl-devel mailing list.

Anyway, thanks for the thorough analysis of the possible threading issues!

Kamil

> Happy Multi-Threaded Hacking!
>
>
> K. Frank
-------------------------------------------------------------------
List admin: https://cool.haxx.se/list/listinfo/curl-library
Etiquette: https://curl.haxx.se/mail/etiquette.html
Received on 2016-09-08