RE: Experimental PATCH (+ question): Problem with HTChannel_delet e

Yes, I agree 100% (routines in a library should not be so "clever").  Check
the library routine that sets the maximum number of pipelined requests.
That one irked me quite a bit...

Taylor Gautier
Senior Software Engineer
Scale Eight, Inc.

-----Original Message-----
From: Peter Stamfest [mailto:peter.stamfest@eunet.at]
Sent: Saturday, September 02, 2000 10:20 PM
To: www-lib@w3.org
Subject: Experimental PATCH (+ question): Problem with HTChannel_delete



Please find attached an experimental patch that solves a HUGE problem for
me. 

It regards HTChannel_delete, which cowardly refuses to free (and
delete from hash tables) HTChannel objects with a socket set to
INVSOC. However, such a socket is set by libwww itself (in
HTDoClose). This causes HTChannel_deleteAll (which does not check for
"proper sockets") to really do the deletion of HTChannels. However, this
function is only called in HTLibTerminate (or a similar function), at a
point of time where many of the referenced objects have already been
freed, causing accesses to previously freed memory - a VERY BAD thing -
specially, as in my case, if the application has to continue to run...

The attached patch corrects this problem - it allows to delete HTChannels
with a socket set to INVSOC, using the same hash-value strategy as
HTChannel_setSocket.

I would like to add one question: Many of the *_setSomething routines in
the library not only check for a proper "me" pointer, but they also check
for non-NULL value pointers (if a pointer should be set). How is that
so? That way it gets impossible to break a link to an object. For
example: in HTNet.c:


PUBLIC BOOL HTNet_setChannel (HTNet * net, HTChannel * channel)
{
    return (net && channel) ? HTHost_setChannel(net->host, channel) : NO;
}

I think it should be 

PUBLIC BOOL HTNet_setChannel (HTNet * net, HTChannel * channel)
{
    return (net) ? HTHost_setChannel(net->host, channel) : NO;
} 


This would allow to properly unset the channel for the net. Similar
problems exist in many other parts of the library.

Such "clever" routines always tend to complicate things, as for the ONE
time it is really needed to set some value of some object to NULL (because
a newly implemented protocol needs to do this, for example), the request
is not carried out. OK, the routine returns NO, but who would check this
(and react properly) when doing such a call [And, BTW, does the NO mean:
"you illegally passed a NULL pointer" or "HTHost_setChannel failed for
whatever reason" - The code handling such a situation has to check
for NULLness of the passed pointer to find THAT out - So if the
function-caller has to check it anyway, the caller can do it in
advance if it really is that forbidden. But at least this scheme
would allow to set a pointer to NULL if it is really, really
necessary.]? In my opinion such routines should not try to be
"clever" and to guess that the programmer did something wrong: It is not
up to a function to decide if a given argument is valid or not _in such
cases_.

Any comments on this? Should we change this? I think we should, however
there is the old problem that those (IMHO) bugs might be used as features
in some code out there. I would call such code bogus, though.

peter

Received on Tuesday, 5 September 2000 12:44:15 UTC