Re: bugs in HEAD

On Thursday 09 May 2002 13:03, Andres Kroonmaa wrote:

> Yes, and my concern is not "fetch->pd" but fetch itself.
> It is not locked or cbdataReference()'ed anymore.

The fetch structure is cbdataReference()'ed a lot. Not that much in
peer_digest.c, but in all calls out of peer_digest.c.

But I think you are on to something here.. "fetch" is "ours", and we
should not really be calling cbdataReferenceValid on it as it isn't a
reference..

The problem here seems to be that peer_digest is using cbdataFree()
to cancel himself. This is why there is a cbdataInternalLock() call
(which also happens to make cbdataReferenceValid() a valid call on
the pointer). A quick investigation immediately reveals that not all
uses of cbdataReferenceValid(fetch) is within cbdataInternalLock
sections.

> hmm, how comes? afaik whole idea of cbdata was to be able to
> Free it before last reference is gone. At a time of cbdataFree
> cbdataReference(fetch) is still holding a lock, so Free will be
> a no-op until cbdataReferenceDone(fetch) is called. Equally one
> could use cbdataInternalUnlock(fetch) before calling cbdataFree,
> but I have impression "internal" versions shouldn't be used
> freely. As cbdataReferenceDone NULL'ifies the var, it can't be used
> before Free, thats why I put it after a Free.

True, with the limitation that only the pointer returned by
cbdataAlloc may be cbdataFreed (which also nullifies the pointer),
and only pointers returned by cbdataReference() may be checked for
reference validity.

> Here is a problem. cbdataAlloc() does not lock the data anyhow,
> and cbdataReferenceValid() asserts (c->locks > 0), meaning that
> we can't use cbdataReferenceValid() call unless we have at least
> one explicit reference.

The pointer returned by cbdataAlloc has an implicit lock. The same
pointer should be used for cbdataFree() later on.

There is no need to lock this pointer or to call cbdataValid(), as it
is the only pointer you are allowed to call cbdataFree() and hence it
is guaranteed to always be valid.

Here is the problem. peer_digest.c do not know when it will call
cbdataFree() on it's own pointer, and therefor needs to verify it's
own pointer for validity alot.. this is not directly the scope cbdata
is intended for.

When doing this you must manually lock/unlock the pointer whenever
calling an internal section that may cause cbdataFree() to be called,
like what is done around the loop in peerDigestFetchReply().

My suggestion on how to try to deal with it is to remove the
cbdataReferenceValid(fetch) call from peerDigestFetchedEnough, and
instead ensure that this is only called when it is known the
reference is valid. From what it looks this is only entitles adding a
cbdataReferenceValid() call within the loop in peerDigestFetchReply()
to abort the loop if fetch becomes invalid.

Note: the fact that you are being called as a callbach with fetch as
an argument (via void *data) tells you that fetch is valid, or else
the callback would not have taken place.

> From your comment I only deduce that we have to add a new pointer
> fetch->fetchp = cbdataReference(fetch);
> and then:
> cbdataReferenceDone(fetch->fetchp);
> cbdataFree(fetch);
> But that seems needless here.

As I said, the use of cbdata in the digest retreival code needs to be
analyzed a bit..

> And I still have trouble understanding why checking
> cbdataReferenceValid() for an object that has zero locks on
> it is a globally fatal situation.

Because you are only allowed to check the validity of a reference
created by cbdataReference(). If the lock count is zero then there is
no references created by cbdataReference(). Examples of cases this
traps is:

 a) You called cbdataReferenceDone() where actually cbdataFree() was
intended.

 b) incorrect storage of cbdataReference() references, causing the
same cbdataReferenceDone() to be called more than once for the same
reference.

 c) you managed to screw up the link count by incorrect use of
cbdataInternalLock/Unlock.

Regards
Henrik
Received on Fri May 10 2002 - 14:57:05 MDT