Re: bugs in HEAD

On 10 May 2002, at 20:54, Henrik Nordstrom <hno@marasystems.com> wrote:

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

 hmm, I can't find any.

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

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

 What you mean by implicit lock? cbdataAlloc only sets valid bit.

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

 How do you distinguish between pointer and reference? Suppose a
 func is called, once with a pointer and once with a reference.
 How can a func determine whether this is a reference, and if so
 if it is valid? By your definition it must know that beforehand,
 or you must not call the same func with different pointers.
 Then perhaps we should enforce that to avoid abuse of cbdata?

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

 peerDigestFetchedEnough seems to need know whether fetch struct is
 valid or not. Removing this validation from there would need quite
 alot of rewrite of that func that I wouldn't like to do because of
 lack of understanding. For now I'd only try to make sure fetch
 validity checks is done under locks as it was before.

diff -u -r1.13 peer_digest.c
--- peer_digest.c 10 May 2002 20:58:28 -0000 1.13
+++ peer_digest.c 17 May 2002 07:03:22 -0000
@@ -356,16 +356,16 @@
 
     assert(fetch->bufofs <= SM_PAGE_SIZE);
 
+ /* Lock our data to protect us from ourselves */
+ cbdataInternalLock(fetch);
+
     /* If we've fetched enough, return */
     if (peerDigestFetchedEnough(fetch, fetch->buf, fetch->bufofs, "peerDigestHandleReply"))
- return;
+ goto finish;
 
     /* Call the right function based on the state */
     /* (Those functions will update the state if needed) */
 
- /* Lock our data to protect us from ourselves */
- cbdataInternalLock(fetch);
-
     /* Repeat this loop until we're out of data OR the state changes */
     /* (So keep going if the state has changed and we still have data */
     do {
----------------
  finish:
    /* Unlock our data - we've finished with it for now */
    cbdataInternalUnlock(fetch);
}

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

 Ok, but why is just innocent "is this ptr valid?" question
 causing "wrong question man, boom, we shot you"? conspiracy? ;)

 Besides, this could be a reference, just at this point in time
 released reference. cbdataReferenceDone() should have NULL'fied
 the pointer? btw, why would cbdataReferenceValid() return valid
 on NULL pointer?

> a) You called cbdataReferenceDone() where actually cbdataFree() was
> intended.
 cbdataReferenceDone() should assert locks>0
> b) incorrect storage of cbdataReference() references, causing the
> same cbdataReferenceDone() to be called more than once for the same
> reference.
 cbdataReferenceDone() should assert locks>0
> c) you managed to screw up the link count by incorrect use of
> cbdataInternalLock/Unlock.
 cbdataInternalUnLock() should assert locks>0

 I feel there is a lack of instrumentation with cbdata.
 cbdataAlloc'ed pointer can be freed under a lock, and there is
 no way to check for that otherwise than with ReferenceValid.
 Locked cbdata in indistinguishable from its Reference.
 We need then cbdataValid() call for that or something.
 As I understand you insist that the two should be kept separate
 strictly: cbdata itself and ref to it. Then we need something
 more than just lock count for that, imho.
 Its pretty confusing now.

 Intuitively, if you "ask" if cbdataReferenceValid you expect to
 get yes or no, instead you can get a bummer.
 One could always assert(cbdataReferenceValid) to get the bummer.

 Also, are there any tricks with pointer types to let compiler
 catch misuses of references inplace of cbdata?

------------------------------------
 Andres Kroonmaa <andre@online.ee>
 CTO, Microlink Online
 Tel: 6501 731, Fax: 6501 725
 Pärnu mnt. 158, Tallinn,
 11317 Estonia
Received on Fri May 17 2002 - 02:24:20 MDT