Hello Kamil!

On Thu, Sep 8, 2016 at 2:18 AM, Kamil Dudka <kdudka_at_redhat.com> wrote:
> On Wednesday, September 07, 2016 21:07:25 K. Frank wrote:
>> Hi Kamil!
>> On Mon, Aug 22, 2016 at 9:27 AM, Kamil Dudka <kdudka_at_redhat.com> wrote:
>> ...
>> > ...
>> > If there is a schedule where one thread has assigned slot->lastTokenPing
>> > but not yet set the CKF_TOKEN_PRESENT flag in slot->ckFlags, all other
>> > threads will keep returning SEC_ERROR_NO_TOKEN (without actually checking
>> > presence of the token) until the first thread continues (or the specified
>> > timeout elapses).
>> >
>> > In order to fix it, we need to add a mutex to nssSlot_IsTokenPresent() or
>> > at least reorder mutation of slot's state such that slot->lastTokenPing is
>> > not updated before the CKF_TOKEN_PRESENT flag in slot->ckFlags is updated.
>>
>> A quick caution:
>> ...
>> Simply requiring -- in the source code -- that (pseudocode):
>> ...
>> will not eliminate your race condition in a multithreaded environment. This
>> is because a second thread is not guaranteed to see these two updates in
>> the desired order.
>
> Yes. That is basically what the comment in the proposed patch [1] is about:
>
> @@ -201,8 +205,11 @@
> if (nssrv != PR_SUCCESS) {
> slot->token->base.name[0] = 0; /* XXX */
> slot->ckFlags &= ~CKF_TOKEN_PRESENT;
> + /* TODO: insert a barrier here to avoid reordering of the assignments */
> + slot->lastTokenPing = PR_IntervalNow();
> return PR_FALSE;
> }
> + slot->lastTokenPing = PR_IntervalNow();
> return PR_TRUE;
> }
>
> So you are correct. The patch does not guarantee the ordering in all
> environments in all possible situations. But compared to status quo (where
> the assignments are far away from each other in a wrong order in the source
> code already), it makes the spurious failure very unlikely to happen.
>
> Kamil
>
> [1] https://bug1297397.bmoattachments.org/attachment.cgi?id=8787677
> ...

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.

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.

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