C: Cleanup parser and bindings (#1212)

This pull request hardens the C parser core and its language bindings
(Ruby, WASM, Node.js) against memory safety issues.

The Ruby C extension functions now use `rb_ensure` to guarantee that C
memory is freed even when a Ruby exception unwinds the stack

The Ruby node and error conversion functions were also calling
`rb_define_module` and `rb_define_class_under` on every conversion to
look up the target class. The class references are now cached in static
globals, initialized once during `Init_herb`.

Author: marcoroth

config.yml

@@ -297,7 +297,7 @@ errors:
 
     - name: NestedERBTagError
       message:
-        template: "ERB tag `%s` at (%u:%u) was terminated by nested `<%%` tag at (%u:%u). Nesting `<%%` tags is not supported."
+        template: "ERB tag `%s` at (%u:%u) was terminated by nested `<%%` tag at (%zu:%zu). Nesting `<%%` tags is not supported."
         arguments:
           - opening_tag->value
           - opening_tag->location.start.line

examples/unclosed_tags.invalid.html.erb

@@ -0,0 +1,5 @@
+<div>
+  <span>
+    <p>Text
+  </span>
+</div>

ext/herb/extension.c

@@ -14,28 +14,77 @@ VALUE cResult;
 VALUE cLexResult;
 VALUE cParseResult;
 
-static VALUE Herb_lex(VALUE self, VALUE source) {
-  char* string = (char*) check_string(source);
+typedef struct {
+  AST_DOCUMENT_NODE_T* root;
+  VALUE source;
+} parse_args_T;
+
+typedef struct {
+  hb_array_T* tokens;
+  VALUE source;
+} lex_args_T;
+
+typedef struct {
+  char* buffer_value;
+} buffer_args_T;
+
+static VALUE parse_convert_body(VALUE arg) {
+  parse_args_T* args = (parse_args_T*) arg;
+
+  return create_parse_result(args->root, args->source);
+}
+
+static VALUE parse_cleanup(VALUE arg) {
+  parse_args_T* args = (parse_args_T*) arg;
+
+  if (args->root != NULL) { ast_node_free((AST_NODE_T*) args->root); }
+
+  return Qnil;
+}
+
+static VALUE lex_convert_body(VALUE arg) {
+  lex_args_T* args = (lex_args_T*) arg;
+
+  return create_lex_result(args->tokens, args->source);
+}
+
+static VALUE lex_cleanup(VALUE arg) {
+  lex_args_T* args = (lex_args_T*) arg;
+
+  if (args->tokens != NULL) { herb_free_tokens(&args->tokens); }
 
-  hb_array_T* tokens = herb_lex(string);
+  return Qnil;
+}
+
+static VALUE buffer_to_string_body(VALUE arg) {
+  buffer_args_T* args = (buffer_args_T*) arg;
+
+  return rb_utf8_str_new_cstr(args->buffer_value);
+}
+
+static VALUE buffer_cleanup(VALUE arg) {
+  buffer_args_T* args = (buffer_args_T*) arg;
+
+  if (args->buffer_value != NULL) { free(args->buffer_value); }
+
+  return Qnil;
+}
 
-  VALUE result = create_lex_result(tokens, source);
+static VALUE Herb_lex(VALUE self, VALUE source) {
+  char* string = (char*) check_string(source);
 
-  herb_free_tokens(&tokens);
+  lex_args_T args = { .tokens = herb_lex(string), .source = source };
 
-  return result;
+  return rb_ensure(lex_convert_body, (VALUE) &args, lex_cleanup, (VALUE) &args);
 }
 
 static VALUE Herb_lex_file(VALUE self, VALUE path) {
   char* file_path = (char*) check_string(path);
-  hb_array_T* tokens = herb_lex_file(file_path);
 
   VALUE source_value = read_file_to_ruby_string(file_path);
-  VALUE result = create_lex_result(tokens, source_value);
+  lex_args_T args = { .tokens = herb_lex_file(file_path), .source = source_value };
 
-  herb_free_tokens(&tokens);
-
-  return result;
+  return rb_ensure(lex_convert_body, (VALUE) &args, lex_cleanup, (VALUE) &args);
 }
 
 static VALUE Herb_parse(int argc, VALUE* argv, VALUE self) {
@@ -60,13 +109,9 @@ static VALUE Herb_parse(int argc, VALUE* argv, VALUE self) {
     if (!NIL_P(strict)) { parser_options.strict = RTEST(strict); }
   }
 
-  AST_DOCUMENT_NODE_T* root = herb_parse(string, &parser_options);
-
-  VALUE result = create_parse_result(root, source);
+  parse_args_T args = { .root = herb_parse(string, &parser_options), .source = source };
 
-  ast_node_free((AST_NODE_T*) root);
-
-  return result;
+  return rb_ensure(parse_convert_body, (VALUE) &args, parse_cleanup, (VALUE) &args);
 }
 
 static VALUE Herb_parse_file(int argc, VALUE* argv, VALUE self) {
@@ -94,13 +139,9 @@ static VALUE Herb_parse_file(int argc, VALUE* argv, VALUE self) {
     if (!NIL_P(strict)) { parser_options.strict = RTEST(strict); }
   }
 
-  AST_DOCUMENT_NODE_T* root = herb_parse(string, &parser_options);
-
-  VALUE result = create_parse_result(root, source_value);
+  parse_args_T args = { .root = herb_parse(string, &parser_options), .source = source_value };
 
-  ast_node_free((AST_NODE_T*) root);
-
-  return result;
+  return rb_ensure(parse_convert_body, (VALUE) &args, parse_cleanup, (VALUE) &args);
 }
 
 static VALUE Herb_extract_ruby(int argc, VALUE* argv, VALUE self) {
@@ -132,10 +173,9 @@ static VALUE Herb_extract_ruby(int argc, VALUE* argv, VALUE self) {
 
   herb_extract_ruby_to_buffer_with_options(string, &output, &extract_options);
 
-  VALUE result = rb_utf8_str_new_cstr(output.value);
-  free(output.value);
+  buffer_args_T args = { .buffer_value = output.value };
 
-  return result;
+  return rb_ensure(buffer_to_string_body, (VALUE) &args, buffer_cleanup, (VALUE) &args);
 }
 
 static VALUE Herb_extract_html(VALUE self, VALUE source) {
@@ -146,10 +186,9 @@ static VALUE Herb_extract_html(VALUE self, VALUE source) {
 
   herb_extract_html_to_buffer(string, &output);
 
-  VALUE result = rb_utf8_str_new_cstr(output.value);
-  free(output.value);
+  buffer_args_T args = { .buffer_value = output.value };
 
-  return result;
+  return rb_ensure(buffer_to_string_body, (VALUE) &args, buffer_cleanup, (VALUE) &args);
 }
 
 static VALUE Herb_version(VALUE self) {
@@ -171,6 +210,9 @@ __attribute__((__visibility__("default"))) void Init_herb(void) {
   cLexResult = rb_define_class_under(mHerb, "LexResult", cResult);
   cParseResult = rb_define_class_under(mHerb, "ParseResult", cResult);
 
+  rb_init_node_classes();
+  rb_init_error_classes();
+
   rb_define_singleton_method(mHerb, "parse", Herb_parse, -1);
   rb_define_singleton_method(mHerb, "lex", Herb_lex, 1);
   rb_define_singleton_method(mHerb, "parse_file", Herb_parse_file, -1);

src/analyze.c

@@ -1537,6 +1537,9 @@ void herb_analyze_parse_tree(AST_DOCUMENT_NODE_T* document, const char* source,
   herb_visit_node((AST_NODE_T*) document, analyze_erb_content, NULL);
 
   analyze_ruby_context_T* context = malloc(sizeof(analyze_ruby_context_T));
+
+  if (!context) { return; }
+
   context->document = document;
   context->parent = NULL;
   context->ruby_context_stack = hb_array_init(8);
@@ -1546,6 +1549,13 @@ void herb_analyze_parse_tree(AST_DOCUMENT_NODE_T* document, const char* source,
   herb_transform_conditional_open_tags(document);
 
   invalid_erb_context_T* invalid_context = malloc(sizeof(invalid_erb_context_T));
+
+  if (!invalid_context) {
+    hb_array_free(&context->ruby_context_stack);
+    free(context);
+    return;
+  }
+
   invalid_context->loop_depth = 0;
   invalid_context->rescue_depth = 0;
 

src/analyze_conditional_elements.c

@@ -285,6 +285,9 @@ static void rewrite_conditional_elements(hb_array_T* nodes, hb_array_T* document
 
     if (contains_single_open_tag(statements, &open_tag)) {
       conditional_open_tag_T* entry = malloc(sizeof(conditional_open_tag_T));
+
+      if (!entry) { continue; }
+
       entry->open_index = node_index;
       entry->open_conditional = node;
       entry->open_tag = open_tag;
@@ -416,17 +419,23 @@ static void rewrite_conditional_elements(hb_array_T* nodes, hb_array_T* document
 
     for (size_t body_index = matched_open->open_index + 1; body_index < node_index; body_index++) {
       size_t* consumed_index = malloc(sizeof(size_t));
-      *consumed_index = body_index;
 
-      hb_array_append(consumed_indices, consumed_index);
+      if (consumed_index) {
+        *consumed_index = body_index;
+        hb_array_append(consumed_indices, consumed_index);
+      }
+
       hb_array_set(nodes, body_index, NULL);
     }
 
     hb_array_set(nodes, matched_open->open_index, conditional_element);
 
     size_t* close_index = malloc(sizeof(size_t));
-    *close_index = node_index;
-    hb_array_append(consumed_indices, close_index);
+
+    if (close_index) {
+      *close_index = node_index;
+      hb_array_append(consumed_indices, close_index);
+    }
     hb_array_set(nodes, node_index, NULL);
 
     if (matched_open->condition) { free((void*) matched_open->condition); }

src/analyze_conditional_open_tags.c

@@ -424,8 +424,11 @@ static void rewrite_conditional_open_tags(hb_array_T* nodes, hb_array_T* documen
 
     for (size_t j = i + 1; j <= close_index; j++) {
       size_t* index = malloc(sizeof(size_t));
-      *index = j;
-      hb_array_append(consumed_indices, index);
+
+      if (index) {
+        *index = j;
+        hb_array_append(consumed_indices, index);
+      }
     }
   }
 

src/analyzed_ruby.c

@@ -7,6 +7,8 @@
 analyzed_ruby_T* init_analyzed_ruby(hb_string_T source) {
   analyzed_ruby_T* analyzed = malloc(sizeof(analyzed_ruby_T));
 
+  if (!analyzed) { return NULL; }
+
   pm_parser_init(&analyzed->parser, (const uint8_t*) source.data, source.length, NULL);
 
   analyzed->root = pm_parse(&analyzed->parser);

src/ast_node.c

@@ -30,6 +30,8 @@ void ast_node_init(AST_NODE_T* node, const ast_node_type_T type, position_T star
 AST_LITERAL_NODE_T* ast_literal_node_init_from_token(const token_T* token) {
   AST_LITERAL_NODE_T* literal = malloc(sizeof(AST_LITERAL_NODE_T));
 
+  if (!literal) { return NULL; }
+
   ast_node_init(&literal->base, AST_LITERAL_NODE, token->location.start, token->location.end, NULL);
 
   literal->content = herb_strdup(token->value);

src/include/util/hb_array.h

@@ -1,6 +1,7 @@
 #ifndef HERB_ARRAY_H
 #define HERB_ARRAY_H
 
+#include <stdbool.h>
 #include <stdlib.h>
 
 typedef struct HB_ARRAY_STRUCT {
@@ -15,15 +16,15 @@ void* hb_array_get(const hb_array_T* array, size_t index);
 void* hb_array_first(hb_array_T* array);
 void* hb_array_last(hb_array_T* array);
 
-void hb_array_append(hb_array_T* array, void* item);
+bool hb_array_append(hb_array_T* array, void* item);
 void hb_array_set(const hb_array_T* array, size_t index, void* item);
 void hb_array_free(hb_array_T** array);
 void hb_array_remove(hb_array_T* array, size_t index);
 
 size_t hb_array_index_of(hb_array_T* array, void* item);
 void hb_array_remove_item(hb_array_T* array, void* item);
 
-void hb_array_push(hb_array_T* array, void* item);
+bool hb_array_push(hb_array_T* array, void* item);
 void* hb_array_pop(hb_array_T* array);
 
 size_t hb_array_capacity(const hb_array_T* array);

src/include/util/hb_narray.h

@@ -12,15 +12,15 @@ typedef struct HB_NARRAY_STRUCT {
   size_t capacity;
 } hb_narray_T;
 
-void hb_narray_init(hb_narray_T* array, size_t item_size, size_t initial_capacity);
+bool hb_narray_init(hb_narray_T* array, size_t item_size, size_t initial_capacity);
 #define hb_narray_pointer_init(array, initial_capacity) (hb_narray_init(array, sizeof(void*), initial_capacity))
 
 void* hb_narray_get(const hb_narray_T* array, size_t index);
 void* hb_narray_first(hb_narray_T* array);
 void* hb_narray_last(hb_narray_T* array);
 size_t hb_narray_size(const hb_narray_T* array);
 
-void hb_narray_append(hb_narray_T* array, void* item);
+bool hb_narray_append(hb_narray_T* array, void* item);
 void hb_narray_remove(hb_narray_T* array, size_t index);
 void hb_narray_deinit(hb_narray_T* array);