[squid-dev] [RFC] [PREVIEW] LockingPointer round 3.

[Alex Rousskov]

On 07/19/2016 10:45 PM, Amos Jeffries wrote:
> On 19/07/2016 7:14 p.m., Amos Jeffries wrote:
>> On 19/07/2016 6:58 a.m., Christos Tsantilas wrote:
>>> On 07/18/2016 08:32 PM, Alex Rousskov wrote:
>>>> I can only repeat my earlier suggestions to hide that dangerous
>>>> constructor behind an explicit static method like
>>>> LockingPointer::NewWithoutLocking() and to assert that
>>>> resetWithoutLocking() method is fed a previously locked object.

>>> something like that.

>> Nod. I'm about to try a build with dropped construtor to see what
>> breaks. That should highlight if a builder function is actually
>> necessary. I hope we can avoid it.


> Attached is a patch showing what that constructor removal would look
> like for current Squid trunk.

Creating a nil pointer just to reset it on the next line looks like bad
style to me. It makes it impossible to make the pointer object constant
and increases the chances that the two lines will get separated in the
future for now good reason, possibly causing bugs.

Why resist naming an unusual/special construction method?


> I've added "// X locks" comments on new resetWithoutLocking() to be
> clearer where the non-locking comes from. OpenSSL API explicitly
> mentions that it "increments a reference" / locks.

The "resetWithoutLocking(X()); // X locks" approach creates a redundant
distraction IMO. If you feel that we need those comments, then perhaps
the reset method needs a new name, like resetUsingLocked(X()) or
resetToLocked(X()).

Note that you actually did not add a comment where one would be useful:

ctx->resetWithoutLocking(sslContext); // XXX: how do we know its locked?




> // XXX: why not set fd_table if that add() succeeded ?

The simple answer is that if you set fd_table after cache add()ing
succeeded, then both fd_table and the cache will delete the same
pointer, crashing Squid. Note that ctx is a raw pointer [to a smart
pointer]. However, I doubt this is the correct/best answer.

AFAICT, the poor-quality comments in the original code were supposed to
answer that question like this: The _only_ reason we add ctx to the
table is so that it gets eventually destroyed. The context object is not
needed in the table and should not be accessed from there. If we
successfully add ctx to the cache, then the cache becomes responsible
for its destruction and we do not need to add it to the table.


> +            if (!ssl_ctx_cache)
> +                // if there is no storage, just use
>                  fd_table[clientConnection->fd].dynamicSslContext = sslContext;
> +            else {

Missing {} around the multiline if-statement body.

> +    explicit LockingPointer() : raw(nullptr) {}

Default explicit constructors is a convoluted C++ issue. Unless you have
a specific need for it, I suggest removing "explicit" here. Otherwise,
document that need in a source code comment.


> +     * Our destructor will do the matching unlock.

This comment is inaccurate because we may unlock in places other than
the destructor. You can say "We become responsible for the matching
unlock()." or something like that.



> +        tmp.resetWithoutLocking(crl); // XXX: does PEM_read_bio_X509_CRL lock ??

One more reason to assert that resetWithoutLocking() is fed a locked
OpenSSL object and also assert that unlock() has a locked OpenSSL object
(if any).

To answer your question, yes, I think it does lock because the BUGS
section of the PEM_read_bio_X509_CRL manual page contains an example
that frees the result:

>         X509_free(x);
>         x = PEM_read_bio_X509(bp, NULL, 0, NULL);


HTH,

Alex.