Re: bugs in HEAD

storeClientCopy is one..

Regards
Henrik

Andres Kroonmaa wrote:
> 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 - 12:28:30 MDT