ofXml error on comment

[turowskipaul]

In the ofXml class, when I try to read through an XML file sequentially using the setToSibling() method, I get EXC_BAD_ACCESS from getValue() when a commented line, like
<!-- <pRangeMin>36</pRangeMin>-->
is encountered.

[bilderbuchi]

@joshuajnoble , any idea?

[tpltnt]

@turowskipaul I failed to replicate the bug as of c214832. I have tested different scenarios. Can you post example data and code which triggers the bug?

[turowskipaul]

Thanks for testing. Here is some code:

testing.xml

<testing>
    <one>1</one>
    <two>2</two>
    <!-- <three>3</three> -->
    <four>4</four>
    <five>5</five>
</testing>

ofApp.h

#pragma once

#include "ofMain.h"

class ofApp : public ofBaseApp{
    ofXml myXml;
public:
    void setup();
};

ofApp.cpp

#include "ofApp.h"
void ofApp::setup(){
    if (myXml.load("testing.xml") ) {
        int numFields = myXml.getNumChildren();

        vector<float> xmlStorage(numFields);

        ofLogNotice() << "xmlStorage size: " << xmlStorage.size();

        myXml.setToChild(0);
        for (int x=0; x<numFields; x++) {
            xmlStorage[x] = myXml.getFloatValue();
            ofLogNotice() << x << " was set to: " << xmlStorage[x];

            myXml.setToSibling();
        }
    }
}

Console output

[notice ] xmlStorage size: 4
[notice ] 0 was set to: 1
[notice ] 1 was set to: 2
(lldb) 

stops on string ofXml::getValue() const, with error:

Thread 1: EXC_BAD_ACCESS (code=2, address 0x0)

The crash can be avoided with a simple test, like so:

ofApp.h

#pragma once

#include "ofMain.h"

class ofApp : public ofBaseApp{
    ofXml myXml;
    static const string xmlNames[];

public:
    void setup();
};

ofApp.cpp

#include "ofApp.h"

const string ofApp::xmlNames[] = {
    "one",
    "two",
    "three",
    "four",
    "five"
};

//--------------------------------------------------------------
void ofApp::setup(){
    if (myXml.load("testing.xml") ) {
        int numFields = myXml.getNumChildren();

        vector<float> xmlStorage(numFields);

        ofLogNotice() << "xmlStorage size: " << xmlStorage.size();

        myXml.setToChild(0);
        for (int x=0; x<numFields; x++) {
            string mName = myXml.getName();
            if (mName == xmlNames[x]) {
                xmlStorage[x] = myXml.getFloatValue();
                ofLogNotice("ofApp") << xmlNames[x] << " was set to: " << xmlStorage[x];
            }
            else {
                xmlStorage[x] = 0;
                ofLogError("ofApp") << "No value for " << xmlNames[x] << " in XML; setting to default: " << xmlStorage[x];
            }

            myXml.setToSibling();
        }
    }
}

Console output

[notice ] xmlStorage size: 4
[notice ] ofApp: one was set to: 1
[notice ] ofApp: two was set to: 2
[ error ] ofApp: No value for three in XML; setting to default: 0
[notice ] ofApp: four was set to: 4

However, it's still not storing the correct number of values because setToSibling() is stopping on the commented line. In short, getNumChildren() and setToSibling() seem to be inconsistent.

[tpltnt]

@turowskipaul thank you for the data. I can replicate the bug for a7a6e5b.

[tpltnt]

The data provided triggers multiple bugs . I fixed the NULL pointer dereference in getValue() (see my local branch). I am currently looking into the setToSibling() and getNumChildren() inconsistency.
@joshuajnoble what are you working on?

[joshuajnoble]

@tpltnt Ah, I was going to look at this a little later today but it seems like you've got it handled so maybe I'll leave it to you?

My guess is that the setToSibling() and getNumChildren() need better filtering because POCO reports everything as a child, whitespace, etc. It's probably just a matter of tracking down the type for commented nodes and ensuring that those don't get included in the count. I don't remember the documentation for poco::xml being all that thorough but it might be better now.

[tpltnt]

I think i fixed this issue. I do have a personal test suite and ran the code posted which yields the following output:

[notice ] xmlStorage size: 4
[notice ] 0 was set to: 1
[notice ] 1 was set to: 2
[notice ] 2 was set to: 4
[notice ] 3 was set to: 5

@turowskipaul Can you check out my branch and check if your (other) code works now? (I want your ok as a basic quality check before issuing a pull request.)

[turowskipaul]

Worked for me. Cheers!

[tpltnt]

@turowskipaul please reopen since the issue still exits. My stuff has not been merged yet.

[turowskipaul]

sorry!

[tpltnt]

@turowskipaul no problem

[bilderbuchi]

closed by #4151