[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: rev 7961 - trunk/subversion/libsvn_wc

From: John Szakmeister <john_at_szakmeister.net>
Date: 2003-12-10 02:32:44 CET

On Tuesday 09 December 2003 13:09, Philip Martin wrote:
> Julian Foad <julianfoad@btopenworld.com> writes:
> > Philip Martin wrote:
> >> If using the access baton is wrong, why is using memory allocated
> >> by the access baton OK? If using the memory is OK why should I not
> >> be able to use the access baton to get it?
> >
> > Using that memory was not OK in the absence of any interface
> > promises regarding it. I should have made a duplicate of that
> > string so that it (the duplicate) is definitely not associated with
> > the adm_access baton.
>
> The memory remains valid until the pool is cleared, which is perfectly
> normal in Subversion. I see no reason for that memory not to be used.
> I'm not convinced that imposing non-standard rules on the memory
> associated with access batons is a good idea.

It is seems normal for operations in which you expect that to happen. The
problem is that while the API says that you can call svn_wc_adm_close()
again, it does not say that you are allowed to use it in any other
operations. If we're going to do that, then we should explicitly state that
it is allowed. Otherwise, we should avoid doing so.

> >> Can you explain the constraints you are attempting to enforce?
> >
> > The principle was:
> >
> > Do not use an adm_access baton after it has been closed.
> >
> > "Use" means, for example, dereference or pass to another function.
> > I think I would now include the path string and any resource that
> > was owned by the adm_access baton and not explicitly disowned by it.
>
> Are you planning to poison the entries pointed to be the entries hash?
> What about the memory pointed to by the entries themselves? I'd worry
> about the impact on performace.
>
> > The reason for trying to enforce that principle (both by trying to
> > fix some current violations and by trying to add automatic
> > detection) is, of course, that use of any resource after it is
> > closed is hard to detect in other ways and is often a source of
> > subtle bugs.
>
> Tools like valgrind, and purify will detect attempts to use the memory
> after a pool has been cleared. I suspect that attempting to write
> your own memory detection stuff is going to be less effective than
> those existing tools. Further, any additional problems that your own
> system detects will be problems introduced by your new rules.

You are right, tools like valgrind will help to point out problems where we
actually try to use freed memory. However, it doesn't necessarily find
violations in our API, which is what I think this is. The point here is not
to catch a memory problem per se, but a problem in the way the API is being
used.

> At present access batons are bound by pool lifetime, just like most
> other things in Subversion. Access batons with write locks are a bit
> special, because they must prevent attempts to modify the entries file
> once the lock file has been removed. The current code already detects
> and prevents such modification. Access batons without write locks are
> just memory structures, there is not even any guarantee that they
> represent the working copy (an access baton with a write lock could
> have modified it). As far as I am concerned there is no need to
> explicitly close read-only batons, simply abondoning them to pool
> cleanup is sufficient.

Again, whether or not it was allocated in the pool or not isn't the issue
here. It attempting to reuse a resource that you said you were finished
with. It may be just a memory structure now, but that doesn't mean that it
always will be, and letting these sorts of mistakes slide only leads to
problems down the road.

I realize this sort of thing may be nit-picky, but I've been in this situation
before and it's a nightmare to go back and fix when many other clients have
come to depend on a 'feature' that wasn't promised.

I'll admit that the first attempt at this was a bit of hack, and some of that
is my fault as well. We should find a better way to do this, but I don't
feel we should let it slide. We should either promise this behavior, or fix
it and find a way to help detect these subtle errors.

-John

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Dec 10 02:28:22 2003

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.