Re: [xml] xmlRemoveID bug and a patch

Hello again

I would like to draw your attention to the xmlRemoveID function in the
valid.c file of the libxml2 tree. I have noticed a bug in that this
function does not actually do what it is intended for and remove the ID
from the internal ID hash table. I have attached a patch that rectifies

  No that's intended. The IDness is defined by the presence of the attribute.
I think your patch is wrong w.r.t. the definition of IDness in the XML spec.

I appreciate that the 'IDness' is only defined by the presence of the
attribute and the attribute's declaration as an ID type in the DTD.  
However, the problem is that when I rename an attribute with xmlSetProp
that is an ID, the value is changed in the xml tree, but the respective
document's ID hash table is not updated to the correct value. As a result
of this, future xmlGetID lookups may give false positives for attributes
that were removed. So, it seems that I am supposed to remove the ID
explicitely from the documents IDs hash table with xmlRemoveID, which is
documented as removing "the given attribute from the ID table maintained
internally." In the function's present form, this appears to always fail
due to a miscast pointer:

    xmlAttrPtr cur;
    cur = xmlHashLookup(table, ID);
    if (cur != attr) {

The 'table' pointer, which refers to a document's 'ids' table has inserted
into it xmlIDPtrs by the xmlAddID function. Here, they are being cast into 
a xmlAttrPtr and compared with the xmlAttrPtr parameter 'attr'.

Also, I would like to request either some documentation or functionality 
changes of the following functions:

xmlSetProp (and xmlNewProp) - these functions will NOT call xmlRemoveID or 
xmlAddID on any ID properties. I think that they either should, if 
necessary, or it be explicitly documented that they do not.

  Substanciate why you think it's wrong. I do think the current behaviour
is the right one w.r.t. the XML spec. The fact that an attribute is of type
ID is dependant on the DTD, this does not change with xmlSetProp/xmlNewProp.
So the IDness property is directly related to 1/ the definition in the DTD
2/ the presence of the attribute on the element.

I understand that an attribute being an ID is dependent on the DTD. What I
am trying to get at is that xmlUnsetProp and xmlFreeProp will _check_ the
property to see if it as an ID with xmlIsID and then (attempt to) remove
the internal hash table reference with xmlRemoveID, whereas xmlSetProp
will not perform any such check. To me, it would seem logical to make this
behavior symmetrical with xmlUnsetProp, or document what each does and
doesn't do more clearly. Such an addition would involve checking the
document for DTD, checking the target elements 'ATTLIST's for the
attribute named and using xmlAddID if it would be required.

I can appreciate that perhaps not many people would find much use in this 
additional convenience. It is not really a problem for me either way.

Please find attached a sample program and xml file that should demonstrate
the xmlRemoveID IDs hash table problem. I have hopefully included enough
commentary in the source code for you to be able to comprehend its
behavoiral objectives. If I have misinterpreted the desired use of any of
the function calls, then please accept my apologies and my polite request
that you point out where I have gone wrong. If you need any further 
clarifications of the example, please don't hesitate to contact me.

If you find the behavior to be incorrect aswell, then please refer to my 
original posting for a patch which should fix (or at the very least help 
you locate) the problem.

I can well understand how this error has come to be present, as I have 
found little evidence for the xmlRemoveID function's usage in any 
programs. So, I hope this is of help.

Thank you



William Robinson
baggins elitemail org

Attachment: libxml-ids-error.c
Description: Text document

Attachment: libxml-ids-error.xml
Description: Text document

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]