Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up error handling in the C extension. #2096

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 8 additions & 12 deletions ext/nokogiri/extconf.rb
Original file line number Diff line number Diff line change
Expand Up @@ -421,11 +421,7 @@ def using_system_libraries?
add_cflags(ENV["CFLAGS"])

if windows?
$CFLAGS << " -DXP_WIN -DXP_WIN32 -DUSE_INCLUDED_VASPRINTF"
end

if solaris? || aix?
$CFLAGS << " -DUSE_INCLUDED_VASPRINTF"
$CFLAGS << " -DXP_WIN -DXP_WIN32"
end

if darwin?
Expand Down Expand Up @@ -675,13 +671,13 @@ def configure
asplode("lib#{lib}")
end

have_func('xmlHasFeature') or abort "xmlHasFeature() is missing."
have_func('xmlFirstElementChild')
have_func('xmlRelaxNGSetParserStructuredErrors')
have_func('xmlRelaxNGSetParserStructuredErrors')
have_func('xmlRelaxNGSetValidStructuredErrors')
have_func('xmlSchemaSetValidStructuredErrors')
have_func('xmlSchemaSetParserStructuredErrors')
have_func('vasprintf')
have_func('xmlHasFeature') or abort "xmlHasFeature() is missing." # introduced in libxml 2.6.21
have_func('xmlFirstElementChild') # introduced in libxml 2.7.3
have_func('xmlRelaxNGSetParserStructuredErrors') # introduced in libxml 2.6.24
have_func('xmlRelaxNGSetValidStructuredErrors') # introduced in libxml 2.6.21
have_func('xmlSchemaSetValidStructuredErrors') # introduced in libxml 2.6.23
have_func('xmlSchemaSetParserStructuredErrors') # introduced in libxml 2.6.23

create_makefile('nokogiri/nokogiri')

Expand Down
14 changes: 2 additions & 12 deletions ext/nokogiri/nokogiri.c
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,11 @@ VALUE mNokogiriXslt ;
VALUE mNokogiriXmlSax ;
VALUE mNokogiriHtmlSax ;

#ifdef USE_INCLUDED_VASPRINTF
#ifndef HAVE_VASPRINTF
/*
* I srsly hate windows. it doesn't have vasprintf.
* Thank you Geoffroy Couprie for this implementation of vasprintf!
*/
int vasprintf (char **strp, const char *fmt, va_list ap)
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.
Expand All @@ -28,16 +27,7 @@ int vasprintf (char **strp, const char *fmt, va_list ap)
}
#endif

void vasprintf_free (void *p)
{
free(p);
}

#ifdef HAVE_RUBY_UTIL_H
#include "ruby/util.h"
#else
#include "util.h"
#endif

void nokogiri_root_node(xmlNodePtr node)
{
Expand Down
38 changes: 17 additions & 21 deletions ext/nokogiri/nokogiri.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,31 +2,24 @@
#define NOKOGIRI_NATIVE

#if _MSC_VER
#ifndef WIN32_LEAN_AND_MEAN
#define WIN32_LEAN_AND_MEAN
#endif /* WIN32_LEAN_AND_MEAN */
#ifndef WIN32
#define WIN32
#endif /* WIN32 */
#include <winsock2.h>
#include <ws2tcpip.h>
#include <windows.h>
# ifndef WIN32_LEAN_AND_MEAN
# define WIN32_LEAN_AND_MEAN
# endif /* WIN32_LEAN_AND_MEAN */

# ifndef WIN32
# define WIN32
# endif /* WIN32 */

# include <winsock2.h>
# include <ws2tcpip.h>
# include <windows.h>
#endif

#include <stdlib.h>
#include <string.h>
#include <assert.h>
#include <stdarg.h>

#ifdef USE_INCLUDED_VASPRINTF
int vasprintf (char **strp, const char *fmt, va_list ap);
#else

#define _GNU_SOURCE
# include <stdio.h>
#undef _GNU_SOURCE

#endif
#include <stdio.h>

#include <libxml/parser.h>
#include <libxml/entities.h>
Expand All @@ -43,6 +36,9 @@ int vasprintf (char **strp, const char *fmt, va_list ap);
#include <libxslt/extensions.h>
#include <libxslt/xsltconfig.h>
#include <libxml/c14n.h>

#include <xml_libxml2_hacks.h>

#include <ruby.h>
#include <ruby/st.h>
#include <ruby/encoding.h>
Expand All @@ -64,8 +60,6 @@ int vasprintf (char **strp, const char *fmt, va_list ap);
#define RBSTR_OR_QNIL(_str) \
(_str ? NOKOGIRI_STR_NEW2(_str) : Qnil)

#include <xml_libxml2_hacks.h>

#include <xml_io.h>
#include <xml_document.h>
#include <html_entity_lookup.h>
Expand Down Expand Up @@ -106,6 +100,8 @@ extern VALUE mNokogiriHtml ;
extern VALUE mNokogiriHtmlSax ;
extern VALUE mNokogiriXslt ;

int vasprintf(char **strp, const char *fmt, va_list ap);

void nokogiri_root_node(xmlNodePtr);
void nokogiri_root_nsdef(xmlNsPtr, xmlDocPtr);

Expand Down
2 changes: 1 addition & 1 deletion ext/nokogiri/xml_node.c
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ static VALUE reparent_node_with(VALUE pivot_obj, VALUE reparentee_obj, pivot_rep
* issue #391, where new node's prefix may become the string "default"
* see libxml2 tree.c xmlNewReconciliedNs which implements this behavior.
*/
xmlFree(reparentee->ns->prefix);
xmlFree((xmlChar*)reparentee->ns->prefix);
reparentee->ns->prefix = NULL;
}
}
Expand Down
7 changes: 2 additions & 5 deletions ext/nokogiri/xml_sax_parser.c
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
#include <xml_sax_parser.h>

int vasprintf (char **strp, const char *fmt, va_list ap);
void vasprintf_free (void *p);

static ID id_start_document, id_end_document, id_start_element, id_end_element;
static ID id_start_element_namespace, id_end_element_namespace;
static ID id_comment, id_characters, id_xmldecl, id_error, id_warning;
Expand Down Expand Up @@ -206,7 +203,7 @@ static void warning_func(void * ctx, const char *msg, ...)
va_end(args);

ruby_message = NOKOGIRI_STR_NEW2(message);
vasprintf_free(message);
free(message);
rb_funcall(doc, id_warning, 1, ruby_message);
}

Expand All @@ -223,7 +220,7 @@ static void error_func(void * ctx, const char *msg, ...)
va_end(args);

ruby_message = NOKOGIRI_STR_NEW2(message);
vasprintf_free(message);
free(message);
rb_funcall(doc, id_error, 1, ruby_message);
}

Expand Down
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

27 changes: 4 additions & 23 deletions ext/nokogiri/xml_xpath_context.c
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
#include <xml_xpath_context.h>

int vasprintf (char **strp, const char *fmt, va_list ap);

static void deallocate(xmlXPathContextPtr ctx)
{
NOKOGIRI_DEBUG_START(ctx);
Expand Down Expand Up @@ -168,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 @@ -194,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 @@ -208,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
5 changes: 1 addition & 4 deletions ext/nokogiri/xslt_stylesheet.c
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,6 @@

VALUE xslt;

int vasprintf (char **strp, const char *fmt, va_list ap);
void vasprintf_free (void *p);

static void mark(nokogiriXsltStylesheetTuple *wrapper)
{
rb_gc_mark(wrapper->func_instances);
Expand Down Expand Up @@ -37,7 +34,7 @@ static void xslt_generic_error_handler(void * ctx, const char *msg, ...)

rb_str_cat2((VALUE)ctx, message);

vasprintf_free(message);
free(message);
}

VALUE Nokogiri_wrap_xslt_stylesheet(xsltStylesheetPtr ss)
Expand Down
21 changes: 7 additions & 14 deletions test/xml/test_document.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down