moovweb/gokogiri

Crash with custom XPath resolver

Opened this issue · 3 comments

magnushiie commented

There seems to be some memory corruption happening when using custom XPath resolver using XPath.SetResolver(VariableScope):

unexpected fault address 0x676e69727493
fatal error: fault
[signal 0xb code=0x1 addr=0x676e69727493 pc=0x4a3c79]

goroutine 9 [running, locked to thread]:
runtime.throw(0x58c660, 0x5)
        /usr/local/go/src/runtime/panic.go:527 +0x90 fp=0xc820036ab8 sp=0xc820036aa0
runtime.sigpanic()
        /usr/local/go/src/runtime/sigpanic_unix.go:27 +0x2ab fp=0xc820036b08 sp=0xc820036ab8
github.com/moovweb/gokogiri/xpath.go_can_resolve_function(0xc820091470, 0x1305830, 0x0, 0x0)
        /home/ubuntu/gopath/src/github.com/moovweb/gokogiri/xpath/util.go:114 +0x99 fp=0xc820036b70 sp=0xc820036b08

I've reduced the repro to the minimal source I could, see crashtest.go and the full crash log for Linux with 4 goroutines at https://gist.github.com/magnushiie/4587ba63b3a37bc9e76e

I could reproduce the crash on both Linux and Windows, using various libxml versions. On Windows I got crashes even when I changed the parallelism to 1 (the crash location was then in Go runtime semaphore code); on Linux I couldn't reproduce this with less than 4 goroutines.

The crash does not happen when the SetResolver line is commented out in the source.

The crash usually happens at the above-mentioned location (go_can_resolve_function), when it's trying to dereference the address of the IsFunctionRegistered VariableScope interface method, but the interface pointer is corrupt. In one debugging session, it contained an 8-byte character string that was the result of the XPath expression.

magnushiie commented

Seems I found the issue, submitted PR #89 with a fix.

jbowtie commented

Fantastic work finding that so quickly. +1 to merge.

You might want to submit the pull request to the ThomsonReutersEikon/gokogiri branch as well (they seem to be more responsive)

magnushiie commented

Thanks for the pointer, and actually #79 by @grahamking already contains a fix by @kisielk almost identical to mine (didn't know to check it because the title of the PR is "Fixes for Go 1.4" - I pretty much started with Go 1.5, so didn't think it was relevant). I'm keeping this PR open for now - seems #79 is somewhat controversial (according to @twojtasz comment) but I don't think this one is.