[squid-dev] [PATCH] LockingPointer API update

[Amos Jeffries]

On 22/06/2016 1:02 p.m., Alex Rousskov wrote:
> On 06/21/2016 04:00 AM, Amos Jeffries wrote:
>> With GnuTLS support it is sometimes more useful to use a TidyPointer
>> where a LockingPointer is used by OpenSSL.
> 
> Are there examples of such code in Squid trunk already? If yes, please
> point to one or two to set the context. I could not quickly identify
> them while looking at the patch... If not, perhaps you can cut-and-paste
> them from your GnuTLS branch?

There are no examples in trunk that I know of. The partial GnuTLS
conversion does okay so far using -1 as LockingPointer lock type.

But in order to create actual locking for GnuTLS session details we have
to implement LockingPointer properly. Which will require either a nasty
looking hack to get LockingPointer to do what its supposed to in the one
case, or change the API and use the more appropriate pointer in all cases.

The session resume patch makes use of this. But does not technically
have to if we continue to define -1 or such locking type value as being
a special non-locking LockingPointer (yuck).

> 
> IMHO, the following rule of thumb is relevant to this discussion:
> Library-agnostic APIs (e.g., Security) must be the same, regardless of
> the library Squid is being compiled with.
> 
> The implementation of those Library-agnostic APIs may depend on the
> library, of course. It is also perfectly fine for GnuTLS-specific code
> to use types that are different from types used by OpenSSL-specific
> code. However, the library-agnostic APIs must remain the same.
> 
> For example, do not add and/or rely on something like this:
> 
>     namespace Security {
>         #if USE_OPENSSL
>             typedef LockingPointer<...> SessionPointer;
>         #elif USE_GNUTLS
>             typedef TidyPointer<...> SessionPointer;
>         #endif
>     };
> 
> unless LockingPointer and TidyPointer have the same API.
> 
> Violating this rule will lead to library-agnostic code that compiles and
> works fine with one library but either does not compile or, worse, does
> not work with the other library.
> 
> Please note that I am not trying to accuse the proposed patch of
> violating the above rule! I just feel it is important to mention that
> rule here because trunk code already violates that rule, the patch might
> be trying to work around existing violations, and future patch
> modifications may have to work with this rule.
> 

Nod. Agreed.

> 
> Now to the specific change being proposed:
> 
>> This patch converts the LockingPointer resetAndLock() to a virtual
>> reset() so callers can use the right one without needing to care which
>> type of pointer they are handling.
> 
> The two LockingPointer methods, inherited reset(X) and "direct"
> resetAndLock(X) give different results: The former does not lock X while
> the latter does. Proposed renaming essentially removes one of the
> methods. That can be done only if either
> 
>   A) nobody outside LockingPointer calls its inherited reset() method OR
>   B) all such calls are buggy.
> 
> (A) is false -- there are ~15 external LockingPointer::reset() calls,
> without even counting those that simply reset the pointer to nil. You
> have not asserted (B) AFAICT, and I am worried that it might also be false.
> 
> I have attached a list of relevant trunk calls. It may be incomplete.
> 

Thank you.

The two I saw were:

1) PeekingPeerConnector::handleServerCertificate() doing
serverBump->serverCert.reset(serverCert.release())

On much closer inspection it appears not a bug. But is doing move
semantics without a move operator relying heavily on specific
implementation detail of TidyPointer API exposure.


2) PeekingPeerConnector::serverCertificateVerified() doing two different
reset() vs resetAndLock() depending on the data source.

The OpenSSL docs do say the call increments a reference count itself.
But I still think this is leaking. Since we explicitly unlock on
destruct as well as calling the necessary free API method in OpenSSL.


> 
>> Doing this has already uncovered two instances of the
>> TidyPointer::reset() being wrongly used on cert LockingPointer objects.
> 
> AFAIK, a LockingPointer::reset(X) call is legitimate if something locked
> X for us and/or the previous owner is losing ownership without
> decreasing the lock count. Can you point to the two instances you found
> please? If those two are indeed bugs, they should be fixed separately,
> before your changes go in IMO -- we should record where those bugs were.
> 
> There are also LockingPointer::reset(nil) calls but those are benign and
> should be replaced with clear() calls that are faster and, well, clearer.
> 

Nod.


> 
>> There may be more hidden away
> 
> Indeed! The ability to _accidentally_ call LockingPointer::reset() is a
> major LockingPointer API bug IMO. And the fact that we cannot be sure
> whether the existing external LockingPointer::reset() calls are
> accidental is a lesser LockingPointer API bug.
> 
> 
> There are two sane ways to proceed AFAICT:
> 
> 1. Assert that all external LockingPointer::reset() calls are bugs.
>    Hide LockingPointer::reset() from external callers and then
>    fix "LockingPointer<>::reset(T*) is private" compilation errors,
>    arriving at LockingPointer with a single reset method.
>    Revisit your patch: Do other differences in two classes APIs matter?
> 
> 2. Assert that some external LockingPointer::reset() calls are needed.
>    Provide LockingPointer::resetWithoutLocking() instead,
>    hide LockingPointer::reset() from external callers, and
>    fix "LockingPointer<>::reset(T*) is private" compilation errors.
>    Reject your patch (because LockingPointer has two reset methods) and
>    study Security/GnuTLS needs to propose a different solution.
> 
> If it turns out that some external LockingPointer::reset() calls are
> needed, I would be happy to try to help you find a different solution.
> 

I'm trying #1, with an eye on #2 if it runs into trouble or much
complexity getting down to one reset() method.

Amos