[squid-dev] [PATCH] TidyPointer removal

[Amos Jeffries]

On 9/07/2016 11:18 a.m., Alex Rousskov wrote:
> On 07/08/2016 12:44 AM, Amos Jeffries wrote:
>> On 8/07/2016 11:24 a.m., Alex Rousskov wrote:
>>>>
>>>> +    void resetWithoutLocking(T *t) {
>>>> +        unlock();
>>>> +        raw = t;
>>>>      }
>>>>  
>>>>      void resetAndLock(T *t) {
>>>> +        if (t != get()) {
>>>> +            resetWithoutLocking(t);
>>>> +            lock(t);
>>>> +        }
>>>> +    }
>>>
>>> We should probably add self-reset protection to both cases or to none.
> 
> 
>> Strange as it seems doing that would violate the non-locking guarantee
>> on the first one.
> 
> How does the following code violate the non-locking guarantee?
> 
>     void resetWithoutLocking(T *t) {
>         if (t != get() {
>             unlock();
>             raw = t;
>         }
>     }
> 
> 
>> Both reset*() methods (and assignments) guarantee that the UnLocker is
>> called on any previously stored value.
> 
> Self-assignment and self-reset is a gray area. Each
> assignment/reset-handling class can define what should happen under
> those circumstances. The possible sane definitions are:
> 
>   1. Do nothing (side effects lead to leaks/crashes in non-benign cases)
>   2. assertion/exception (explicit error in all cases)
>   3. Do the usual (side effects lead to leaks/crashes in most cases)
> 
> Which definition to pick is our choice, but we should be _consistent_ in
> that choice. I recommend #1, but it is a weak recommendation.
> 
> In the latest patch I have seen, resetAndLock() was using #1 but
> resetWithoutLocking() was using #3.
> 

resetAndLock() is doing #1 and there are no side effects. Because the
new lock() is performed by us, not the caller. So we can confidently do
nothing with no side effects.

resetWithoutLocking() cannot do #1 without adding the listed side
effects for all 'benign' cases.

The words "most cases" is incorrect on #3, because most cases will be
caller acting correctly and doing its +1 on the locks before our
unlock() is used. Only buggy callers will lead to the listed side effects.

I argue that these are bugs in the callers. Bugs we need to see (via
those side effects) and fix.

We cannot assert in the self-reset case because that is a legitimate
input from good callers. We can trust them to have locked. There is
nothing we can reliably use as assert() parameter.


> 
>> If that means the raw-pointer is
>> about to go invalid then its a caller bug for using that method of the
>> API. Because resetWithoutLocking() fully delegates lock control to the
>> caller this is not something we can make assumptions about in the
>> resetWithoutLocking() variant.
> 
> Self-assignment and self-reset is always a caller problem and is always
> a gray area -- logical reasoning cannot arrive at a single "correct" or
> "best" solution here.

In the generic case you might be right.

However, we have a situation here where OpenSSL functions either lock or
they don't lock. That black-and-white limitation works in our favour
here so we can calculate with code being either good or buggy and reason
from that.

> 
> A caller has delegated control of the already managed object locks to
> us.
> Now the caller wants us to import the same object that was allegedly
> pre-locked for our import. It is our decision whether to unlock/free the
> object (#3), assert/throw (#2), or do nothing (#1).

This is a new caller allegedly with a pointer it has added a second lock
to. It doesn't matter that the value is self because that second lock
exists. resetWithoutLocking() is just discarding that old lock, and
taking responsibility for the new one.


A good caller code is one that has actually done the locking it should
have and delagating us the unlock() task. There will be no problem from
using UnLocker() on the previously stored value. This is the common case
for self-assignment/reset in good code.

A buggy caller code is one that is not adding that lock and still using
resetWithoutLocking(), so use of UnLocker will possibly lead to
deallocation and invalid pointer value stored.

This tells me that skipping the unlock() on self-assignments for
resetWithoutLocking() will hide bugs in bad code and add leaks in good
code. Which are not desirable results.

Yes the bugs are likely to be hard to track down. But they will be
impossible to trackdown if we actively add code to hide them. We will
instead just get bug reports about the leaks and waste time tracking
those down in what is actually good code.

> 
> I doubt we should pick #3, but that is not what I am complaining about
> right now. I am complaining about inconsistency. If you are sure that
> crashing the program in most cases is the best solution in the
> LockingPointer context, then you should use that solution for all
> reset*() and assignment methods.

Crashing is *not* okay in the resetAndLock() pathway self-assignment.
That code is working correctly.

The inconsistency is forced by OpenSSL behaviour.

> 
>> Doing a self-check to retain raw-pointer validity would behave the same
>> as if it had locked the pointer given.
> 
> You cannot know what self-check side effects will be because they depend
> on the caller. Thus, any assertion that "do nothing" would behave the
> same as "do something" is wrong.
> 

We can and do. There are exactly two cases. One where the caller adds a
lock, and one where it doesn't.

Which callers are good and which are broken is something we can figure
out with a code audit of callers. Which Christos did earlier before and
informed us the uses of reset() vs resetAndLock() were all correct.


> For example, the following benign code triggers self-reset but has no
> side effects at all if self-reset is handled using the "do nothing"
> approach:
> 
>     p.reset(p->get()); // p = p
> 

I assume you meant to write resetWithoutLocking() there.

That code pattern is buggy. If you find any such uses please point them
out to be fixed.

Correct 'p = p' code is:

for 3.5:
  p.reset(p.release());

for trunk:
  p = std::move(p);


> 
>> the non-TLS builds UnLocker will do
>> nothing and T will just be a void or int placeholder.
> 
> That does not sound quite right to me: If we want TLS-incapable code to
> use LockingPointer for TLS stuff that cannot exist in such code, then
> UnLocker and lock() ought to assert! In other words, only nil
> LockingPointers should be allowed in that special case.
> 

The lock() method asserts if it is ever used on those builds. So its
caught on assignment, not destruct.


> 
>>> I do not recall any remaining in-scope LockingPointer issues except that
>>> the constructor does not force the caller to pick the right importing
>>> route. What issues did I forget about?
> 
>> The absence of locks for GnuTLS 
> 
> GnuTLS support is out of this change scope, I hope.
> 

Since some of its types used TidyPointer and LockingPointer that code is
as much under consideration as the OpenSSL build, and in the same ways.

Each change in the patch was and continues to be tested for x3 builds;
* layer-00-default = GnuTLS
* layer-01-minimal = none
* layer-02-maximus = OpenSSL

> 
>> and the need to use LockingPointer without actual locking 
> 
> Inside GnuTLS builds, you mean? Or is that a separate problem from the
> insufficient GnuTLS support in Squid? If it is the latter, please detail it.
> 

Yes I mean for the GnuTLS builds in their current state.

> 
>> Okay. Build testing the final version underway and will merged to trunk
>> when thats done.
> 
> Please resolve self-assignment/reset consistency, before or after that
> merge.
> 
> 
> Thank you,
> 
> Alex.
>