Re: [xml] structured error from xmlXPathCompile

[Petr Pajas <pajas ufal ms mff cuni cz>]

Daniel Veillard <veillard redhat com> writes:

> >   http://mail.gnome.org/archives/xml/2003-November/msg00098.html
>
>   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 */
xmlFreeNode(node);
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 */
        xmlFree(ID);
        return(-1);

sould read

    if (cur->attr != attr) { 
        xmlFree(ID);
        return(-1);

cf. xmlGetID:

    xmlIDPtr id;
    ....
    id = xmlHashLookup(table, ID);
    if (id == NULL)
        return(NULL);
    .....
    return(id->attr); 

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 -- reader2.py 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 reader2.py 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?

Thanks,

-- Petr