On Thursday, September 08, 2016 16:59:56 K. Frank wrote:
> Hi Kamil!
>
> On Thu, Sep 8, 2016 at 3:23 PM, Kamil Dudka <kdudka_at_redhat.com> wrote:
> > On Thursday, September 08, 2016 14:55:12 K. Frank wrote:
> >> I have a suggestion ...,
> >>
> >> First some disclaimers: I have no knowledge of the libcurl
> >> code base.
>
> Well, it looks like I have confirmed my lack of knowledge of the
> libcurl code by mistaking NNS code for libcurl code (and then
> commenting on it).
>
> > ...
> > Thanks for the suggestion! This bug is already worked around in libcurl:
> >
> > https://github.com/curl/curl/commit/3a5d5de9
>
> I took a look at this commit. Making some plausible assumptions about
> what PR_Lock does, this libcurl fix looks to me to be straightforward,
> sensible, and sound.

Thanks for review!

> > ...
> > I believe it would be better to take suggestions about the actual NSS fix
>
> > directly to the upstream Bugzilla:
> Indeed.
>
> But with discretion being the better part of valor, perhaps I won't, and
> save myself from further embarrassment.
>
> > ...
> > ... since there are not many people that understand NSS internals on the
> > libcurl-devel mailing list.
>
> Heh. Me included.

Nothing embarrassing IMO. The code in question is just written in a way
that is difficult to understand and difficult to maintain. It clearly does
not work as the author intended.

One would not expect a function named within_token_delay_period() to mutate
the given object. So I started from there but there is probably more such
traps all over the code.

I agree that a proper synchronization mechanism should be added. But first
we need to shrink the protected code to minimum in order not to introduce
unnecessarily long blocking. Otherwise it would defeat the only purpose
of the optimization that introduced the bug.

I wish NSS maintainers cared to comment on the patch to save us discussions
about code we do not know much about...

Kamil

> > Anyway, thanks for the thorough analysis of the possible threading issues!
>
> Your gentle words are appreciated.
>
> > Kamil
>
> Best regards.
>
>
> K. Frank
> -------------------------------------------------------------------
> List admin: https://cool.haxx.se/list/listinfo/curl-library
> Etiquette: https://curl.haxx.se/mail/etiquette.html
-------------------------------------------------------------------
List admin: https://cool.haxx.se/list/listinfo/curl-library
Etiquette: https://curl.haxx.se/mail/etiquette.html
Received on 2016-09-09