[squid-dev] [PATCH] TidyPointer removal

[Amos Jeffries]

On 10/07/2016 2:38 p.m., Alex Rousskov wrote:
> On 07/09/2016 07:10 AM, Amos Jeffries wrote:
>> 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.
> 
>>> 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
> 
> Agreed.
> 
> 
>> 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.
> 

I think you still have some confusion about the LockingPointer API
requirements.

In total:

* For the LockingPointer(T*) ctor and resetWithoutLocking() method
 the caller is responsible for:
 - doing a +1 lock itself, and
 - exactly once for *each time either is used*, and
 - doing it before the call/use.

* For release() the caller is taking on responsibility for exactly 1
un-lock operation.

* For all other methods and operators the recieving Pointer is
responsible for the locking.


Please reconsider your code examples in light of those requirements and
you should see where the bugs in the examples are confusing you.


> We cannot be confident that doing nothing will not cause problems. For
> example, the following well-meaning caller code will assert due to those
> unexpected "side effects" of approach #1:
> 
> void importCertificate(LockingPointer &p, X509 *x)
> {
>     const int referenceCount = x->references;
>     p.resetAndLock(x);
>     // check that we actually locked the new certificate
>     assert(x->references > referenceCount);
> }
> 
> 
> ... if importCertificate() is fed p that already manages certificate x.
> 

The p.resetAndLock() semantically increments the x references and
decrements the existing pointers references before it returns. Making
the assert '>' a bug.

That is true for any proper reference counting. The self-check is an
optimization of that case. Not a requirement.


> 
>> resetWithoutLocking() cannot do #1 without adding the listed side
>> effects for all 'benign' cases.
> 
> In what I call benign cases, approach #1 always works correctly:
> 
>     p.resetWithoutLocking(p->get()); // never a problem with #1!
> 

Bug:  caller of resetWithoutLocking() is not doing a +1 lock increment
itself before that method is called.


> To construct a case where approach #1 leads to problems you need
> non-benign code. For example:
> 
> void importCertificate(LockingPointer &p, X509 *x)
> {
>     const LockingPointer old = p;
>     const int referenceCount = x->references;
>     p.resetWithoutLocking(x);
>     // check that we got rid of the old certificate
>     assert(x->references < referenceCount);
> }
> 

Bug:  caller of resetWithoutLocking() did not do the +1 lock it is
required to do before using that method.



> Needless to say, the above examples are just problem illustrations; real
> code triggering these kind of problems is usually more convoluted.
> 

Yes the bug location failing to do +1 may be somewhere in the caller of
the above importCertificate(). But in essence the whole chain of code
passing 'x' variable down to resetWithoutLocking() is responsible.

By not doing the +1 lock beore calling ResetWithoutLocking() its still a
bug in the caller(s).

> 
>> 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.
> 
> Unfortunately, #3 actually leads to leaks or crashes in benign cases
> where caller _is_ doing its +1 (as well as in some non-benign cases, of
> course). For example:
> 
>   x = newX509(); // creates and locks certificate x
>   assert(x->references == 1); // OK, newX509() +1ed as expected
>   LockingPointer p(x);

NP: the caller has delegated its lock to p constructor.
Making 'x' effectively a raw-pointer with *no* lock associated.

>   assert(x->references == 1); // OK, our constructor did nothing

That 1 reference is held by p now. *not x*.

>   p.resetWithoutLocking(x); // Opos, x is deleted here due to #3

Bug: using resetWithoutLocking() instead of resetAndLock(), when 'x' has
no caller lock associated.

>   std::cout << *p->get(); // crash
> 
> The same is true for resetAndLock(), of course

No. If it was, then Reference Counting (the universal concept) would not
work anywhere.

resetAndLock() is the method by which regular normal reference counting
is done. The recipient smart-pointer taking care of any +1/-1 that needs
to happen and ordering them to ensure the pointer received stays valid.


> -- the self-reset problem
> is universal. The only possible difference between the two reset*()
> methods

... is their entire semantic behaviour design.


> is whether a conservative unlock() implementation asserts before
> the cout usage crash illustrated above:
> 
>   unlock() {
>       assert(x->references > 0); // #3 resetAndLock() may assert here
>       --x->references;
>       if (!x->references) // we were the last one with a lock
>           delete x; // of free(x), etc.
>   }
> 
> 
>> I argue that these are bugs in the callers. 
> 
> Yes, we agree that these self-resets are usually caller "bugs" or at
> least "problems" of one kind or another.
> 
> 
>> Bugs we need to see (via those side effects) and fix.
> 
> If you want to see bugs, you need approach #2. Neither #1 nor #3 are
> reliable in exposing bugs (bad side effects may or may not happen,
> depending on the caller).
> 
> 
>> 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.
> 
> The assert in #2 is just the guard condition. It works fine for regular
> cases and only triggers in case of a self-assignment:
> 
> LockingPointer::reset*()
> {
>     // consider all self-resets to be bugs and expose them
>     assert(t != get());
> 
>     ... do what is needed ...
> }

self-reset is perfectly fine IFF the caller meets the requirements I
outlined at the top. You can't make an assert that tests that reliably.


> 
>>>> 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.
> 
> Sorry, but [OpenSSL] locking has nothing to do with self-assignment or
> self-reset. By definition, self-assignment is "p=p" or equivalent.
> Similarly, self-reset is, by definition:

Then don't call it OpenSSL, call it Fubar for an appropriate acronym.

It has the properties of some calls externally doing a +1 to 'help' our
reference counting Pointers. But not doing the matching unlock.


>     p.reset*(p->get())
> 
> or equivalent. That is, you are resetting a smart pointer using the
> smart pointer itSELF (or equivalent). That is what resetAndLock()
> if-statement checks, and that is the check that resetWithoutLocking()
> currently lacks.

For resetAndLock() that above code is fine. p ensures that pointer stays
valid through the locking/unlocking.

For resetWithoutLocking() its bug: caller not doing +1 itself before use
of the method.

> 
> If you were actually talking about a completely different problem of
> checking whether resetWithoutLocking() caller incremented the lock, then

I dont mean *checking* the lock. I mean the LockingPointer requirements
as listed at the top of this mail must be met.
 The callers which use non-locking interface must meet the lock +1
requirements in order to use that interface.


If you consider regular Refcount Pointers. If a caller did its own -1
reference operation you would call that a bug yes?

It is nasty that LockingPointer has to deal with external lock fiddling.
But we can't get away from that. All we can do is be very careful that
the alternative API is used correctly (in ways that meet the above
requirements).

> 
>> A buggy caller code is one that is not adding that lock and still using
>> resetWithoutLocking()
> 
> That would be indeed a bug, but it has nothing to do with self-reset.
> 

All the presented examples of self-reset are demonstrations of callers
failing that one requirement.

I am completely certain that if the callers using the non-locking API
meet that requirement the locking math works out okay with the current
trunk code.

I am also completely certain that if we add the if-statement to prevent
self-reset/assign unlock() happening *inside* the resetWithoutLocking()
that callers will become unable to use it reliably in those cases.
 We would have to make all callers detect that self-reset/assign case
externally and have extra code to work around the non-unlocking behaviour.


How can I be so certain?

 1) The same behaviour can be implemented by ordering the new values +1
before the old values -1. With no self-protecting if-statement.
 --> always both operations happen even on self-reset/assign.

 2) The normal pattern we are all familiar with wraps BOTH the lock and
unlock operations inside the if-statemet.
 --> either both happen or neither.

When we come to the resetWithoutLocking() it is behaviourally only an
unlock for the old value.

Critically it does not lock on the new value. That means we CANNOT be
doing the 'BOTH' behaviour of (2) inside the method.

Therefore (1) has to be the implementation for use of this part of the
API. With the +1 step happening in caller, and the -1 inside
resetWithoutLocking().
 --> this is where the requirements list I wrote at the top come from.


Is that clear enough now?

Amos