Re: [xml] bug in xmlSetProp?

[Anthony Liguori <ajl13 bellatlantic net>]

Daniel Veillard wrote:

> On Sat, Nov 24, 2001 at 05:24:11PM -0500, Anthony Liguori wrote:
> > this wouldn't be bad, but in the interim it is deleting memory.  It is
> > thereby making many, many assumptions about the nature of the buffer that's
> > being passed in.
>
>   Yes, the assumptions being that this string is safe for access during
> the whole operation. Not the case in the example you raised.

Dangerous assumption when your freeing memory.  The assumption is really that the
string is safe to access even if it is a part of a memory area that is not really
defined to the caller of the function.  There is no way for the API caller to know
if the string is safe or not so it either means that all strings in the xmlDoc
structure are not safe to use in API functions or that the user most have cardinal
knowledge of the inner workings of libxml.  I think this only encourages people to
not use the API which is really not a good thing

> I think your handling of value == NULL is actually broken and would
> break handling of HTML documents. You have a chance to defend your patch

> by stating what you thought was broken and explaining you you think you
> fixed it. Then I will actually be able to tell you if you're wrong or
> right :-)

I just traced through the logic and it seems that the handling of value == NULL
works identically between the patch and unpatched version.  This is the logic as I
follow it when value == NULL

Original
-----------
1) Find property that has same name as name parameter
2) If children are valid for that property, delete them.
3) Set children to NULL (irregardless if they already were NULL)
4) Set last to NULL
5) Return property

Patched
----------
1) Find property that has same name as name parameter
2) If children are valid for that property, delete them and set children to NULL
3) Set last equal to children (which is equal to NULL)
4) Return property

These are the things addressed in my patch.

1) Passing node == NULL would cause a segmentation fault, the logic that seemed to
be trying to achieved was to return NULL in this event.
2) The logic of the function was somewhat redundant.  There were many instances of
values being set to what their value was already set to.  This includes L4506
where prop->last was being set to NULL when it's value was already set to NULL in
L4498.  There are some other instances but they are a little more involved.
3) Most importantly, the function was not nearly as efficent as it could of been.
This is evident in comparing the two sources.  The use of local variables
(specifically tmp) is kind of nasty and makes the function a little confusing.

This is a fix for a simple bug - number 1 - that also involved what at my work we
call an AEM.  Basically, a rewrite to improve performance and mantainability.
There are still some more things I would of done, but it's not really worth
over-analyzing the function :)  If it didn't fix the aforementioned bug, if I were
you I probably wouldn't except it, but it does fix a bug after all (and not even
the one I was trying to fix) ;-)

>   I won't take patches saying "I think something was broken" and not explaining
> why and how, it's a large project as you noticed and I need to be careful,
> I'm sure you understand :-)

Of course :)  I was kind of going for a rework in the logic of the function that
would improve the function as a whole while also addressing some problems that I
encountered.  This patch isn't fixing a show stopping problem but I believe it
does improve the overall code.  It's also my first hack of which will probably be
many hacks to this library ;-)

Regards,
Anthony