[squid-dev] [PATCH] TidyPointer removal

[Alex Rousskov]

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.

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.

Needless to say, such self-resets ought to be "rare", but they are far
from impossible in complex programs where the original relationship
between two pointers can be unknown or lost/forgotten. This is no
different than self-assignment AFAICT.



> 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!

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);
}

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


> 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);
  assert(x->references == 1); // OK, our constructor did nothing
  p.resetWithoutLocking(x); // Opos, x is deleted here due to #3
  std::cout << *p->get(); // crash

The same is true for resetAndLock(), of course -- the self-reset problem
is universal. The only possible difference between the two reset*()
methods 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 ...
}




>>> 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:

    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.

If you were actually talking about a completely different problem of
checking whether resetWithoutLocking() caller incremented the lock, then
I agree that it is a goo idea for OpenSSL-specific build to check that,
provided this works for all OpenSSL objects fed to LockingPointer:

    void resetWithoutLocking(T *t) {
#if USE_OPENSSL
        assert(t->references > 0); // the caller must lock!
#endif
         ... the reset code goes here ...
    }

Again, this is completely unrelated to the self-reset problem.


>> 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.

There is no "*second* lock" guarantee in this context -- all external
resetWithoutLocking() callers believe they are supplying us a raw
pointer pre-locked by OpenSSL (and that OpenSSL expects us to unlock
some time later to avoid leaks), nothing more.


> A good caller code is one that has actually done the locking it should
> have and delagating us the unlock() task. 

Yes.


> There will be no problem from using UnLocker() on the previously stored value. 

There may be. See above for specific self-reset examples that lead to
assertions in unlocker or right after it, depending on the unlocker
implementation.


> 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.


>> 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.

OpenSSL has nothing to do with the self-reset problem. The same
self-reset problem would exist for any TLS library implementation,
pre-locking or not.

OpenSSL inconsistencies force us to have two reset*() methods, but that
is where OpenSSL responsibilities for our problems end. We are
responsible for self-assignment and self-reset handling.



>> 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.

No, the above applies to both reset*() methods.


> That code pattern is buggy.

Yes, and that is what self-reset handling is all about.



> Correct 'p = p' code is:
> 
> for 3.5:
>   p.reset(p.release());

The above code has nothing to do with self-reset, just like the
following code is _not_ self-assignment, by *definition*:

  p1 = p;
  p = p1;



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

That is self-assignment AFAICT, but let's finish with non-optimized code
before we tackle optimizations.




>>> 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.

That is good but not sufficient because resetWithoutLocking() and
LockingPointer constructor does not call lock(). Adding an assertion to
resetWithoutLocking() would address that; no need to assert in UnLocker
if you do that, but it would not hurt either (in case we missed another
spot where resetWithoutLocking should have been called but was not).


Please resolve self-assignment/reset consistency. I hope the above
clarifies why the already committed code should be fixed.


Thank you,

Alex.
P.S. Please note that, just like with self-assignment, it is probably
possible to implement #1 without if-statements that check for self-reset
explicitly. However, that is a code style question, not functionality
issue, so I am ignoring it for now since we have a bigger functionality
problem to solve first.