Re: [xml] structured error from xmlXPathCompile

Daniel Veillard <veillard redhat com> writes:

  No, but that was already discussed.

Please, be patient with me and let me try to convince you otherwise;-)

What my patch fixes is the following:

xmlUnlinkNode(node); /* node has ID id */
xmlRemoveID(id); /* optional - called by xmlFree anyway, but does nothing */
node2 = xmlGetID(doc,id);
if (node2 != NULL) /* YES! */
  do_something(node2);  /* SIGSEGV !!!! */

What I patch in xmlRemoveID is a simple (typo-like) bug, which
prevents it from doing anything ever:

1) xmlRemoveID fetches the id from the ID hash,

    xmlAttrPtr cur;
    cur = xmlHashLookup(table, ID);

But this is wrong, as xmlHashLookup returns xmlIDPtr, which points to
an xmlAttrPtr via ->attr, not xmlAttrPtr itself. It should read:

    xmlIDPtr cur;
    cur = xmlHashLookup(table, ID);

2) xmlRemoveID checkes it fetched the same attribute as it is about to
   remove. This ALWAYS FAILS, because it doesn't have an attribute
   object, but xmlIDPtr object:

    if (cur != attr) { /* ALWAYS TRUE */

sould read

    if (cur->attr != attr) { 

cf. xmlGetID:

    xmlIDPtr id;
    id = xmlHashLookup(table, ID);
    if (id == NULL)

3) removes the ID from the hash, but this never ever happens, because
the above condition is always true and xmlRemoveID returns -1.

I think this patch breaks streaming validation with the reader. Did
you ran the regression tests after applying the patch ?

Ok, the only test it breaks seems to be -- with

../../test/valid/REC-xml-19980210.xml:4147: validity error : attribute
def line 2395 references an unknown ID "dt-match"

and many similar lines follow. All other tests pass.

If there is a problem with xmlRemoveID and reader, it doesn't mean
xmlRemoveID should not be fixed to do what it should do. Maybe
xmlRemoveID should only set id->attr to NULL during streaming
validation, so as the IDs are still hashed. xmlGetID would then
substitute the document node:

#valid.c line 2603 
    if (id->attr == NULL) {
         * We are operating on a stream, return a well known reference
         * since the attribute node doesn't exist anymore
        return((xmlAttrPtr) doc);

But that's a different problem.

In the current version, xmlRemoveID does not work at all, which is
what prevents the from failing. If that is what you want, I
suggest removing xmlRemoveID completely, because it doesn't do
anything anyway. But I think, you want something different and my
patch only reveals there might be other places to fix :-/

If my patch is unacceptable, can you suggest a workaround? How do I
for example unbind a subtree and remove all references to it from the
tree, so that I could free it without fearing a SIGSEGV if somebody
asks for and ID?


-- Petr

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