Skip to content

Commit

Permalink
fix: address memory leak when xpath evaluation raises an error
Browse files Browse the repository at this point in the history
Related to #1610 and #1882.
  • Loading branch information
flavorjones committed Oct 12, 2020
1 parent 6816f52 commit 3bae5cc
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 41 deletions.
32 changes: 25 additions & 7 deletions ext/nokogiri/xml_syntax_error.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
2 changes: 2 additions & 0 deletions ext/nokogiri/xml_syntax_error.h
Original file line number Diff line number Diff line change
Expand Up @@ -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

25 changes: 4 additions & 21 deletions ext/nokogiri/xml_xpath_context.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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);

Expand All @@ -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);
Expand Down
23 changes: 10 additions & 13 deletions test/xml/test_document.rb
Original file line number Diff line number Diff line change
Expand Up @@ -536,25 +536,22 @@ 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
if Nokogiri.uses_libxml?
assert_match(/Unregistered function/, e.message)
end
end

def test_xpath_syntax_error
assert_raises(Nokogiri::XML::XPath::SyntaxError) do
e = assert_raises(Nokogiri::XML::XPath::SyntaxError) do
@xml.xpath('\\')
end
if Nokogiri.uses_libxml?
assert_match(/Invalid expression/, e.message)
end
end

def test_ancestors
Expand Down

0 comments on commit 3bae5cc

Please sign in to comment.