Fix hangs reading xml #27
Conversation
The xml library functions return -1 on error and 0 on EOF. A number of loops of the form "while (result)" would loop forever on an error. Change this to "while (result > 0)", so that they exit on an error. The return value of the functions in xml.c that did this will be 1 on success and 0 on EOF *or error*. So the code using the xml.c functions will be ok as is. Example that hangs: <listen> <port> 123 <port> </listen> The 2nd <port> is missing the slash and doesn't terminate the element, so </listen> is unmatched and it will hang on that error.
The code that parsed a listen section would go into an infinite loop if it encountered a element with an unknown tag. Example: <listen> <typo> hostname </typo> </listen> It will loop forever on the typo element and not skip it. Change this to print an error and skip the element.
|
Great Find! Thanks. |
| if (xmlTextReaderNodeType(reader) == XML_READER_TYPE_TEXT) { | ||
| *text = xmlTextReaderValue(reader); | ||
| } | ||
| result = stepOut(reader); | ||
| } | ||
| return result; | ||
| return result > 0; |
There was a problem hiding this comment.
I don't know why I missed error handling in this code, that's a bit of a mystery.
However the fix shouldn't deviate from the return codes in libxml2:
Returns: 1 if the node was read successfully, 0 if there is no more nodes to read, or -1 in case of error
In other words 0 means EOF and -1 mean error. For the line return result > 0 fixes the immediate problem of other calls failing to check error, the fix should really be to simply return result and fix those other lines. That lets the code exit(1) at an appropriate point.
There was a problem hiding this comment.
The existing code handles it ok, it'll skip the problem section and continue parsing. If this returns -1, then all the code using this needs to be updated too, so it detects errors and handles them specially.
There was a problem hiding this comment.
Yeah. Sadly that's what it needs. :-(
I'll try to assess how much damage this will do to the rest of the code. It might not be so bad.
There was a problem hiding this comment.
I've tested a bunch of different errors and it's decent what happens. It doesn't abort, but the parts that aren't bad work ok and it starts. IMHO, it would be better to abort on any error and produce a clear message about it.
webdavd will hang in an infinite loop on startup for various simple mistakes in the xml.
Anything malformed, like missing a closing tag for an element.
Any unknown element in a listen:
<listen> <typo> 1234 </typo> </listen>Unknown elements in a server were handled.