Merge pull request sparklemotion#2489 from sparklemotion/flavorjones-…

…remove-vasprintf

remove calls to vasprintf

CHANGELOG.md

@@ -28,6 +28,8 @@ This version of Nokogiri uses [`jar-dependencies`](https://github.com/mkristian/
 
 * Compare `Encoding` objects rather than compare their names. This is a slight performance improvement and is future-proof. [[#2454](https://github.com/sparklemotion/nokogiri/issues/2454)] (Thanks, [@casperisfine](https://github.com/casperisfine)!)
 * Avoid compile-time conflict with system-installed `gumbo.h` on OpenBSD. [[#2464](https://github.com/sparklemotion/nokogiri/issues/2464)]
+* Remove calls to `vasprintf` in favor of platform-independent `rb_vsprintf`
+* Prefer `ruby_xmalloc` to `malloc` within the C extension. [[#2480](https://github.com/sparklemotion/nokogiri/issues/2480)] (Thanks, [@Garfield96](https://github.com/Garfield96)!)
 
 
 ## 1.13.3 / 2022-02-21

ext/nokogiri/extconf.rb

@@ -982,8 +982,6 @@ def compile
 have_func("xmlSchemaSetValidStructuredErrors") # introduced in libxml 2.6.23
 have_func("xmlSchemaSetParserStructuredErrors") # introduced in libxml 2.6.23
 
-have_func("vasprintf")
-
 other_library_versions_string = OTHER_LIBRARY_VERSIONS.map { |k, v| [k, v].join(":") }.join(",")
 append_cppflags(%[-DNOKOGIRI_OTHER_LIBRARY_VERSIONS="\\\"#{other_library_versions_string}\\\""])
 

ext/nokogiri/nokogiri.c

@@ -52,29 +52,6 @@ void noko_init_test_global_handlers(void);
 static ID id_read, id_write, id_external_encoding;
 
 
-#ifndef HAVE_VASPRINTF
-/*
- * Thank you Geoffroy Couprie for this implementation of vasprintf!
- */
-int
-vasprintf(char **strp, const char *fmt, va_list ap)
-{
-  /* Mingw32/64 have a broken vsnprintf implementation that fails when
-   * using a zero-byte limit in order to retrieve the required size for malloc.
-   * So we use a one byte buffer instead.
-   */
-  char tmp[1];
-  int len = vsnprintf(tmp, 1, fmt, ap) + 1;
-  char *res = (char *)malloc((unsigned int)len);
-  if (res == NULL) {
-    return -1;
-  }
-  *strp = res;
-  return vsnprintf(res, (unsigned int)len, fmt, ap);
-}
-#endif
-
-
 static VALUE
 noko_io_read_check(VALUE val)
 {

ext/nokogiri/nokogiri.h

@@ -162,7 +162,6 @@ typedef struct _nokogiriXsltStylesheetTuple {
   VALUE func_instances;
 } nokogiriXsltStylesheetTuple;
 
-int vasprintf(char **strp, const char *fmt, va_list ap);
 void noko_xml_document_pin_node(xmlNodePtr);
 void noko_xml_document_pin_namespace(xmlNsPtr, xmlDocPtr);
 

ext/nokogiri/xml_sax_parser.c

@@ -200,35 +200,29 @@ warning_func(void *ctx, const char *msg, ...)
 {
   VALUE self = NOKOGIRI_SAX_SELF(ctx);
   VALUE doc = rb_iv_get(self, "@document");
-  char *message;
-  VALUE ruby_message;
+  VALUE rb_message;
 
   va_list args;
   va_start(args, msg);
-  vasprintf(&message, msg, args);
+  rb_message = rb_vsprintf(msg, args);
   va_end(args);
 
-  ruby_message = NOKOGIRI_STR_NEW2(message);
-  free(message);
-  rb_funcall(doc, id_warning, 1, ruby_message);
+  rb_funcall(doc, id_warning, 1, rb_message);
 }
 
 static void
 error_func(void *ctx, const char *msg, ...)
 {
   VALUE self = NOKOGIRI_SAX_SELF(ctx);
   VALUE doc = rb_iv_get(self, "@document");
-  char *message;
-  VALUE ruby_message;
+  VALUE rb_message;
 
   va_list args;
   va_start(args, msg);
-  vasprintf(&message, msg, args);
+  rb_message = rb_vsprintf(msg, args);
   va_end(args);
 
-  ruby_message = NOKOGIRI_STR_NEW2(message);
-  free(message);
-  rb_funcall(doc, id_error, 1, ruby_message);
+  rb_funcall(doc, id_error, 1, rb_message);
 }
 
 static void

ext/nokogiri/xml_xpath_context.c

@@ -293,14 +293,14 @@ 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;
+  VALUE rb_message;
 
   va_list args;
   va_start(args, msg);
-  vasprintf(&message, msg, args);
+  rb_message = rb_vsprintf(msg, args);
   va_end(args);
 
-  rb_raise(rb_eRuntimeError, "%s", message);
+  rb_exc_raise(rb_exc_new3(rb_eRuntimeError, rb_message));
 }
 
 /*

ext/nokogiri/xslt_stylesheet.c

@@ -23,16 +23,14 @@ dealloc(nokogiriXsltStylesheetTuple *wrapper)
 static void
 xslt_generic_error_handler(void *ctx, const char *msg, ...)
 {
-  char *message;
+  VALUE message;
 
   va_list args;
   va_start(args, msg);
-  vasprintf(&message, msg, args);
+  message = rb_vsprintf(msg, args);
   va_end(args);
 
-  rb_str_cat2((VALUE)ctx, message);
-
-  free(message);
+  rb_str_concat((VALUE)ctx, message);
 }
 
 VALUE

suppressions/nokogiri_ruby.supp

@@ -84,30 +84,6 @@
   fun:xmlXPathEval
   fun:evaluate
 }
-{
-  TODO
-  # 44 bytes in 1 blocks are definitely lost in loss record 14,160 of 37,883
-  # *xmlXPathCompOpEval (xpath.c:13197)
-  # *xmlXPathCompOpEval (xpath.c:12947)
-  # *xmlXPathCompOpEvalToBoolean (xpath.c:13589)
-  # *xmlXPathNodeSetFilter (xpath.c:11664)
-  # *xmlXPathNodeCollectAndTest (xpath.c:12492)
-  # *xmlXPathCompOpEval (xpath.c:13105)
-  # *xmlXPathCompOpEval (xpath.c:12947)
-  # *xmlXPathCompOpEval (xpath.c:13353)
-  # *xmlXPathCompOpEval (xpath.c:12947)
-  # *xmlXPathRunEval (xpath.c:13946)
-  # *xmlXPathEval (xpath.c:14463)
-  # *evaluate (xml_xpath_context.c:322)
-  Memcheck:Leak
-  fun:malloc
-  fun:__vasprintf_internal
-  fun:xpath_generic_exception_handler
-  fun:xmlXPathCompOpEval
-  ...
-  fun:xmlXPathEval
-  fun:evaluate
-}
 {
   TODO
   # 96 (16 direct, 80 indirect) bytes in 1 blocks are definitely lost in loss record 24,755 of 37,883

test/helper.rb

@@ -280,7 +280,7 @@ def error(error)
         end
 
         def warning(warning)
-          (@warning ||= []) << warning
+          (@warnings ||= []) << warning
           super
         end
 

test/test_xslt_transforms.rb

@@ -349,7 +349,10 @@ def test_non_html_xslt_transform
         exception = assert_raises(RuntimeError) do
           xslt.transform(doc)
         end
-        assert_match(/decimal/, exception.message)
+        assert_match(
+          /xmlXPathCompOpEval: function decimal not found|java.lang.NoSuchMethodException.*decimal/,
+          exception.message,
+        )
       end
 
       describe "DEFAULT_XSLT parse options" do

test/xml/sax/test_parser.rb

@@ -460,6 +460,20 @@ def call_parse_io_with_encoding(encoding)
 
           assert_nil(doc.data)
         end
+
+        it "handles parser warnings" do
+          skip_unless_libxml2("this is testing error message formatting in the C extension")
+          xml = <<~XML
+            <?xml version="1.0" encoding="UTF-8"?>
+            <doc xmlns="x">
+              this element's ns definition is crafted to raise a warning
+              from libxml2's SAX2.c:xmlSAX2AttributeInternal()
+            </doc>
+          XML
+          parser.parse(xml)
+          refute_empty(parser.document.warnings)
+          assert_match(/URI .* is not absolute/, parser.document.warnings.first)
+        end
       end
     end
   end

test/xml/test_document.rb

@@ -497,19 +497,14 @@ def test_namespace_should_not_exist
           end
         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
+        def test_non_existent_function
+          # TODO: we should not be raising different types on the different engines
+          e_class = Nokogiri.uses_libxml? ? RuntimeError : Nokogiri::XML::XPath::SyntaxError
 
-          if !Nokogiri.uses_libxml? || (Nokogiri.uses_libxml? && Nokogiri::VERSION_INFO["libxml"]["platform"] == "jruby")
-            exception = Nokogiri::XML::XPath::SyntaxError
-          end
-
-          assert_raises(exception) do
+          e = assert_raises(e_class) do
             xml.xpath("//name[foo()]")
           end
+          assert_match(/function.*not found|Could not find function/, e.to_s)
         end
 
         def test_xpath_syntax_error