xpath //namespace::* returns garbage -> can lead to segfault

[etm]

warnings: []
nokogiri: 1.6.3.1
ruby:
  version: 2.0.0
  platform: x86_64-linux-gnu
  description: ruby 2.0.0p384 (2014-01-12) [x86_64-linux-gnu]
  engine: ruby
libxml:
  binding: extension
  source: packaged
  libxml2_path: "/var/lib/gems/2.0.0/gems/nokogiri-1.6.3.1/ports/x86_64-pc-linux-gnu/libxml2/2.8.0"
  libxslt_path: "/var/lib/gems/2.0.0/gems/nokogiri-1.6.3.1/ports/x86_64-pc-linux-gnu/libxslt/1.1.28"
  compiled: 2.8.0
  loaded: 2.8.0

---

Execute the following code for the file http://gruppe.wst.univie.ac.at/~mangler/SimTest.xml:

require 'nokogiri'
a = Nokogiri::XML::parse(File.open('SimTest.xml')){|config| config.noblanks.noent.nsclean.strict }
b = a.xpath('//namespace::*').to_a
puts b.length 

It tells that it finds 6464 namespaces. Most of them are binary garbage. i.e.

#(Namespace:0x13bad48 {
   prefix = "pK\xC8\u0002",
   href = "\x90\xE6\x83\u0002"
   }),

Accessing them leads to segfaults. The first couple of namespaces are correct, but then (i suppose) it progresses into uncharted memory territory :-)

Funnily it works correctly for some XML files with namespaces. Funnily it works for some XML files with namespaces sometimes (heisenbug). The XML http://gruppe.wst.univie.ac.at/~mangler/SimTest.xml is the first one that i found that consistently lets me reproduce the error.

[etm]

Forgot another fun thing:

require 'nokogiri'
a = Nokogiri::XML::parse(File.open('SimTest.xml')){|config| config.noblanks.noent.nsclean.strict }
a.xpath('//namespace::*')
b = a.xpath('//namespace::*').to_a
puts b.length

If you do the xpath twice, the binary garbage is gone and it works correctly. (the number 6464 is correct because it does it for all nodes and attributes).

[knu]

I was able to reproduce this in an irb session and a standalone program, but putting the code in the test suite it didn't cause SEGV. There could be some kind of a problem with initialization.

[knu]

I guess this is caused by an unguarded VALUE swept by GC. It will take some time to investigate...

[flavorjones]

Looking into this now.

On Thu, Nov 6, 2014 at 6:57 PM, Akinori MUSHA [email protected]
wrote:

I guess this is caused by an unguarded VALUE swept by GC. It will take
some time to investigate...

—
Reply to this email directly or view it on GitHub
#1155 (comment)
.

[flavorjones]

I can see the memory error introduced by this code by running valgrind on it ... trying to nail down when it was introduced (or if it's been there all along, which is possible).

[flavorjones]

OK, I guess the good news is that this bug appears to have been present as long ago as June 2012 (v1.5.3), and this is the first time someone's noticed.

The bad news is that this bug appears to have been present as long ago as June 2012, and this is the first time someone's noticed.

Given this fact, I'm not going to try to rush a fix into 1.6.4.1. Give me some time (when I'm back from vacation next week) to dig in and try to figure out what's going on, and carefully try to fix it.

[flavorjones]

(I forgot to mention, that I suspect this bug has been around since the dawn of time. I just got tired of building old versions of Nokogiri once I got to two years ago.)

[flavorjones]

I concur with @knu that this is likely to be an initialization problem. I have not been able to reproduce the valgrind errors when I run the code in the test suite alongside other tests (though I can if I run the single test).

Even setting GC.stress = true won't emit the valgrind errors in the test suite, which indicates to me that it's likely an initialization issue.

[flavorjones]

I've put the test case on a branch: flavorjones-issue-1155-namespace-gc

[flavorjones]

Hey, travis for the win. Caught Ruby 2.0 crashing in this test:

• https://travis-ci.org/sparklemotion/nokogiri/builds/40439499
• https://travis-ci.org/sparklemotion/nokogiri/jobs/40439501

[flavorjones]

Looking at this today with @akshaysawant ... think we have a fix.

[flavorjones]

There's some history here, which I'll relate.

Originally @tenderlove committed dbda52b to try to plug a memory leak, but that was dereferencing possibly-invalid pointers during deallocate and was reverted.

Then, @ender672 committed 2675a75 which, though it didn't leak memory, introduced the bug you're seeing, which is that we're accessing the namespace nodes returned from an XPath query after the NodeSet has been GCed and freed the underlying namespace structs.

What I'd like to do is closer to what @tenderlove originally proposed, which is to wrap a Nokogiri::XML::Namespace ruby object around the xmlNs struct; except that we'll force the creation of the Namespace object and then let normal Ruby GC figure out when to free that node.

If we do this, then I think we'll have no invalid memory access and no leaks, and we'll be able to remove not only the nokogiriNodeSetTuple but all the namespace-related code in xml_node_set.c; the tradeoff is that we'll incur Object creation costs up front when we search for namespace nodes, but that seems acceptable, because why would you search for a thing and then not use it?

Comments welcome, I'll probably drop this on a branch today and target it for 1.6.8.

[flavorjones]

Please see PR #1362 for my proposed fix, which is clean in valgrind and does not leak memory related to namespace xpath queries.

[flavorjones]

PR merged onto master, will be fixed in 1.6.8.