fix: address memory leak when xpath evaluation raises an error

also ensure CRuby raises an XML::XPath::SyntaxError on xpath function errors (like JRuby). previously CRuby raised a RuntimeError. Related to #1610 and #1882.
sparklemotion · Oct 13, 2020 · 426fd89 · 426fd89
1 parent b76ee14
commit 426fd89
Show file tree

Hide file tree

Showing 4 changed files with 38 additions and 42 deletions.
diff --git a/ext/nokogiri/xml_syntax_error.c b/ext/nokogiri/xml_syntax_error.c
@@ -12,15 +12,34 @@ void Nokogiri_error_raise(void * ctx, xmlErrorPtr error)
   rb_exc_raise(Nokogiri_wrap_xml_syntax_error(error));
 }
 
+void Nokogiri_generic_error_array_pusher(void * ctx, const char *msg, ...)
+{
+  va_list args;
+  char * message;
+  VALUE error_list = (VALUE)ctx;
+  VALUE message_rb;
+
+  Check_Type(error_list, T_ARRAY);
+
+  va_start(args, msg);
+  vasprintf(&message, msg, args);
+  va_end(args);
+
+  message_rb = NOKOGIRI_STR_NEW2(message);
+  rb_ary_push(error_list,
+              rb_class_new_instance(1, &message_rb, cNokogiriXmlXpathSyntaxError));
+
+  free(message);
+}
+
 VALUE Nokogiri_wrap_xml_syntax_error(xmlErrorPtr error)
 {
   VALUE msg, e, klass;
 
   klass = cNokogiriXmlSyntaxError;
 
   if (error && error->domain == XML_FROM_XPATH) {
-    VALUE xpath = rb_const_get(mNokogiriXml, rb_intern("XPath"));
-    klass = rb_const_get(xpath, rb_intern("SyntaxError"));
+    klass = cNokogiriXmlXpathSyntaxError;
   }
 
   msg = (error && error->message) ? NOKOGIRI_STR_NEW2(error->message) : Qnil;
@@ -49,16 +68,15 @@ VALUE Nokogiri_wrap_xml_syntax_error(xmlErrorPtr error)
 }
 
 VALUE cNokogiriXmlSyntaxError;
+VALUE cNokogiriXmlXpathSyntaxError;
 void init_xml_syntax_error()
 {
   VALUE nokogiri = rb_define_module("Nokogiri");
   VALUE xml = rb_define_module_under(nokogiri, "XML");
 
-  /*
-   * The XML::SyntaxError is raised on parse errors
-   */
   VALUE syntax_error_mommy = rb_define_class_under(nokogiri, "SyntaxError", rb_eStandardError);
-  VALUE klass = rb_define_class_under(xml, "SyntaxError", syntax_error_mommy);
-  cNokogiriXmlSyntaxError = klass;
+  cNokogiriXmlSyntaxError = rb_define_class_under(xml, "SyntaxError", syntax_error_mommy);
 
+  VALUE xml_xpath = rb_define_class_under(mNokogiriXml, "XPath", rb_cObject);
+  cNokogiriXmlXpathSyntaxError = rb_define_class_under(xml_xpath, "SyntaxError", cNokogiriXmlSyntaxError);
 }
diff --git a/ext/nokogiri/xml_syntax_error.h b/ext/nokogiri/xml_syntax_error.h
@@ -6,8 +6,10 @@
 void init_xml_syntax_error();
 VALUE Nokogiri_wrap_xml_syntax_error(xmlErrorPtr error);
 void Nokogiri_error_array_pusher(void * ctx, xmlErrorPtr error);
+void Nokogiri_generic_error_array_pusher(void * ctx, const char *msg, ...);
 NORETURN(void Nokogiri_error_raise(void * ctx, xmlErrorPtr error));
 
 extern VALUE cNokogiriXmlSyntaxError;
+extern VALUE cNokogiriXmlXpathSyntaxError;
 #endif
 
diff --git a/ext/nokogiri/xml_xpath_context.c b/ext/nokogiri/xml_xpath_context.c
@@ -166,19 +166,6 @@ static xmlXPathFunction lookup( void *ctx,
   return NULL;
 }
 
-NORETURN(static void xpath_generic_exception_handler(void * ctx, const char *msg, ...));
-static void xpath_generic_exception_handler(void * ctx, const char *msg, ...)
-{
-  char * message;
-
-  va_list args;
-  va_start(args, msg);
-  vasprintf(&message, msg, args);
-  va_end(args);
-
-  rb_raise(rb_eRuntimeError, "%s", message);
-}
-
 /*
  * call-seq:
  *  evaluate(search_path, handler = nil)
@@ -192,6 +179,7 @@ static VALUE evaluate(int argc, VALUE *argv, VALUE self)
   xmlXPathContextPtr ctx;
   xmlXPathObjectPtr xpath;
   xmlChar *query;
+  VALUE error_list = rb_ary_new();
 
   Data_Get_Struct(self, xmlXPathContext, ctx);
 
@@ -206,20 +194,15 @@ static VALUE evaluate(int argc, VALUE *argv, VALUE self)
     xmlXPathRegisterFuncLookup(ctx, lookup, (void *)xpath_handler);
   }
 
-  xmlResetLastError();
-  xmlSetStructuredErrorFunc(NULL, Nokogiri_error_raise);
-
-  /* For some reason, xmlXPathEvalExpression will blow up with a generic error */
-  /* when there is a non existent function. */
-  xmlSetGenericErrorFunc(NULL, xpath_generic_exception_handler);
+  xmlSetStructuredErrorFunc((void*)error_list, Nokogiri_error_array_pusher);
+  xmlSetGenericErrorFunc((void*)error_list, Nokogiri_generic_error_array_pusher);
 
   xpath = xmlXPathEvalExpression(query, ctx);
   xmlSetStructuredErrorFunc(NULL, NULL);
   xmlSetGenericErrorFunc(NULL, NULL);
 
   if(xpath == NULL) {
-    xmlErrorPtr error = xmlGetLastError();
-    rb_exc_raise(Nokogiri_wrap_xml_syntax_error(error));
+    rb_exc_raise(rb_ary_entry(error_list, -1));
   }
 
   assert(ctx->doc);

diff --git a/test/xml/test_document.rb b/test/xml/test_document.rb
@@ -536,25 +536,18 @@ def test_namespace_should_not_exist
         }
       end
 
-      def test_non_existant_function
-        # WTF.  I don't know why this is different between MRI and Jruby
-        # They should be the same...  Either way, raising an exception
-        # is the correct thing to do.
-        exception = RuntimeError
-
-        if !Nokogiri.uses_libxml? || (Nokogiri.uses_libxml? && Nokogiri::VERSION_INFO["libxml"]["platform"] == "jruby")
-          exception = Nokogiri::XML::XPath::SyntaxError
-        end
-
-        assert_raises(exception) {
+      def test_non_existent_function
+        e = assert_raises(Nokogiri::XML::XPath::SyntaxError) do
           @xml.xpath("//name[foo()]")
-        }
+        end
+        assert_match(/foo/, e.message)
       end
 
       def test_xpath_syntax_error
-        assert_raises(Nokogiri::XML::XPath::SyntaxError) do
-          @xml.xpath('\\')
+        e = assert_raises(Nokogiri::XML::XPath::SyntaxError) do
+          @xml.xpath('[asdf]')
         end
+        assert_match(/\[asdf\]/, e.message)
       end
 
       def test_ancestors