opencog / atomspace

The OpenCog (hyper-)graph database and graph rewriting system
https://wiki.opencog.org/w/AtomSpace
Other
818 stars 232 forks source link

Potential illegal memory access in Node::toString called by python binding #123

Closed kim135797531 closed 9 years ago

kim135797531 commented 9 years ago

I found this behavior during writing unit test of my python conceptual blending project.

When I make new node with no name, AtomSpace returns wrong node with name "(".

Furthermore when I try to make new node with new AtomSpace, this problem getting worse, because it prints machine's memory contents.

This code:

from opencog.atomspace import AtomSpace
from opencog.type_constructors import *

my_as = AtomSpace()
# Make new node with **blank name**
print my_as.add_node(types.ConceptNode, "")
del my_as

my_as = AtomSpace()
print my_as.add_node(types.ConceptNode, "")
del my_as

my_as = AtomSpace()
print my_as.add_node(types.ConceptNode, "")
del my_as

...(skip)

makes this problem.

(ConceptNode "(") ; [2]

(ConceptNode " "") ; [4]

(ConceptNode "") ; [6]

...(skip)...

(ConceptNode "]
") ; [14]

(ConceptNode "") ; [16]

(ConceptNode "") ; [18]

(ConceptNode "%s "%s" (av %d %d %d) %s) ; [%lu]
") ; [20]

(ConceptNode " "%s" (av %d %d %d) %s) ; [%lu]

...(skip)...

(ConceptNode "") ; [58]

(ConceptNode "asic_string::_S_construct null not valid") ; [60]

(ConceptNode "ic_string::_S_construct null not valid") ; [62]

(ConceptNode "_string::_S_construct null not valid") ; [64]

(ConceptNode "tring::_S_construct null not valid") ; [66]

It seems problem was caused by c_str() method in Node::toString() at Node.cc:68 but I'm not sure.

My unit test makes new AtomSpace instance for each test case, and my blender has 'blank' ConceptNode to express default prefix name of new blend node.

Now I'm trying to avoid using the 'blank' ConceptNode because now I think it seems little wrong design conceptually. And the method AtomSpace::atomAsString() at AtomSpace.h:680, which was deprecated:

   /** DEPRECATED! Do NOT USE IN NEW CODE!
     * If you need this, just copy the code below into your app! */
    std::string atomAsString(Handle h, bool terse = true) const {
        if (terse) return h->toShortString();
        return h->toString();
    }

makes this problem because it calls toString() method. But since python users use this method very often because python binding of Atom uses it in __str__, so I think I have to report this problem to community.

jswiergo commented 9 years ago

This is because of a small bug in casting. I've sent a fix.

bgoertzel commented 9 years ago

Thanks Jacek!

On Sat, Jul 4, 2015 at 11:47 PM, Jacek Świergocki notifications@github.com wrote:

This is because of a small bug in casting. I've sent a fix.

— Reply to this email directly or view it on GitHub https://github.com/opencog/atomspace/issues/123#issuecomment-118525296.

Ben Goertzel, PhD http://goertzel.org

"The reasonable man adapts himself to the world: the unreasonable one persists in trying to adapt the world to himself. Therefore all progress depends on the unreasonable man." -- George Bernard Shaw

kim135797531 commented 9 years ago

My code passes the unit tests after applying your fix. Thanks for the quick fix!