Merge pull request #3083 from Shopify/reject-unreachable-start-pos

Reject a byte position the lexer cannot start on

Author: soutaro

ext/rbs_extension/main.c

@@ -146,44 +146,66 @@ static VALUE parse_type_try(VALUE a) {
     return rbs_struct_to_ruby_value(ctx, type);
 }
 
-static void validate_position_range(int start_pos, int end_pos) {
+/**
+ * `end_pos` may point past the end of the buffer: clamping with a large
+ * number instead of measuring the buffer is ordinary, and the lexer stops at
+ * the end on its own.
+ * */
+static void validate_position_range(VALUE string, int start_pos, int end_pos) {
     if (start_pos < 0 || end_pos < 0) {
         rb_raise(rb_eArgError, "negative position range: %d...%d", start_pos, end_pos);
     }
     if (start_pos > end_pos) {
         rb_raise(rb_eArgError, "invalid position range: %d...%d", start_pos, end_pos);
     }
+
+    long size = RSTRING_LEN(string);
+    if ((long) start_pos > size) {
+        rb_raise(rb_eArgError, "position range starts past the end of the buffer: %d...%d, buffer is %ld bytes", start_pos, end_pos, size);
+    }
 }
 
 static rbs_lexer_t *alloc_lexer_from_buffer(rbs_allocator_t *allocator, VALUE string, rb_encoding *encoding, int start_pos, int end_pos) {
-    validate_position_range(start_pos, end_pos);
+    validate_position_range(string, start_pos, end_pos);
 
     const char *encoding_name = rb_enc_name(encoding);
 
-    return rbs_lexer_new(
+    rbs_lexer_t *lexer = rbs_lexer_new(
         allocator,
         rbs_string_from_ruby_string(string),
         rbs_encoding_find((const uint8_t *) encoding_name, (const uint8_t *) (encoding_name + strlen(encoding_name))),
         start_pos,
         end_pos
     );
+
+    if (lexer == NULL) {
+        rb_raise(rb_eArgError, "position range starts inside a character: %d...%d", start_pos, end_pos);
+    }
+
+    return lexer;
 }
 
 static rbs_parser_t *alloc_parser_from_buffer(VALUE buffer, int start_pos, int end_pos) {
-    validate_position_range(start_pos, end_pos);
-
     VALUE string = rb_funcall(buffer, rb_intern("content"), 0);
     StringValue(string);
 
+    validate_position_range(string, start_pos, end_pos);
+
     rb_encoding *encoding = rb_enc_get(string);
     const char *encoding_name = rb_enc_name(encoding);
 
-    return rbs_parser_new(
+    rbs_parser_t *parser = rbs_parser_new(
         rbs_string_from_ruby_string(string),
         rbs_encoding_find((const uint8_t *) encoding_name, (const uint8_t *) (encoding_name + strlen(encoding_name))),
         start_pos,
         end_pos
     );
+
+    if (parser == NULL) {
+        rb_raise(rb_eArgError, "position range starts inside a character: %d...%d", start_pos, end_pos);
+    }
+
+    return parser;
 }
 
 static VALUE rbsparser_parse_type(VALUE self, VALUE buffer, VALUE start_pos, VALUE end_pos, VALUE variables, VALUE require_eof, VALUE void_allowed, VALUE self_allowed, VALUE classish_allowed) {

include/rbs/parser.h

@@ -89,17 +89,24 @@ RBS_NODISCARD bool rbs_parser_insert_typevar(rbs_parser_t *parser, rbs_constant_
  * VALUE string = rb_funcall(buffer, rb_intern("content"), 0);
  * rbs_lexer_new(string, 0, 31)    // New rbs_lexer_t with buffer content
  * ```
+ *
+ * Returns `NULL` when `start_pos` is not the first byte of a character in
+ * `string` -- inside one, or past the end of the buffer. The lexer reaches
+ * `start_pos` a character at a time, so nowhere else is a position it can
+ * start from.
  * */
-rbs_lexer_t *rbs_lexer_new(rbs_allocator_t *, rbs_string_t string, const rbs_encoding_t *encoding, int start_pos, int end_pos);
+RBS_NODISCARD rbs_lexer_t *rbs_lexer_new(rbs_allocator_t *, rbs_string_t string, const rbs_encoding_t *encoding, int start_pos, int end_pos);
 
 /**
  * Allocate new rbs_parser_t object.
  *
  * ```
  * rbs_parser_new(buffer, string, encoding, 0, 1);
  * ```
+ *
+ * Returns `NULL` for a `start_pos` that `rbs_lexer_new` rejects.
  * */
-rbs_parser_t *rbs_parser_new(rbs_string_t string, const rbs_encoding_t *encoding, int start_pos, int end_pos);
+RBS_NODISCARD rbs_parser_t *rbs_parser_new(rbs_string_t string, const rbs_encoding_t *encoding, int start_pos, int end_pos);
 void rbs_parser_free(rbs_parser_t *parser);
 
 /**

src/parser.c

@@ -3539,12 +3539,18 @@ rbs_lexer_t *rbs_lexer_new(rbs_allocator_t *allocator, rbs_string_t string, cons
         lexer->current_character_bytes = 1;
     }
 
-    if (start_pos > 0) {
-        while (lexer->current.byte_pos < start_pos) {
-            rbs_skip(lexer);
-        }
+    // `rbs_skip` moves a whole character at a time, and moves nothing at all
+    // once the input is spent, so this walk can only ever stand on the first
+    // byte of a character.
+    while (lexer->current.byte_pos < start_pos && lexer->current_code_point != '\0') {
+        rbs_skip(lexer);
     }
 
+    // Stopping anywhere else means `start_pos` is a position the lexer cannot
+    // start from: over it, and the walk stepped across a character that
+    // straddles it; short of it, and the input ran out first.
+    if (lexer->current.byte_pos != start_pos) return NULL;
+
     lexer->start = lexer->current;
 
     return lexer;
@@ -3554,6 +3560,11 @@ rbs_parser_t *rbs_parser_new(rbs_string_t string, const rbs_encoding_t *encoding
     rbs_allocator_t *allocator = rbs_allocator_init();
 
     rbs_lexer_t *lexer = rbs_lexer_new(allocator, string, encoding, start_pos, end_pos);
+    if (lexer == NULL) {
+        rbs_allocator_free(allocator);
+        return NULL;
+    }
+
     rbs_parser_t *parser = rbs_allocator_alloc(allocator, rbs_parser_t);
 
     *parser = (rbs_parser_t) {

test/rbs/type_parsing_test.rb

@@ -1019,14 +1019,58 @@ def test_parse__range_works
   end
 
   def test_parse__byte_range_incorrect
-    # We want a better error handling ergonomics, but currently simply raises a syntax error.
+    # `"🐕🐈"` is 10 bytes: `"` then two four-byte characters then `"`. Bytes 2
+    # to 4 are inside the first of them, and the lexer can only ever stop on a
+    # character boundary, so there is no honest answer to give.
 
     input = '"🐕🐈"'
 
+    (2..4).each do |start|
+      exn = assert_raises ArgumentError do
+        Parser.parse_type(input, byte_range: start...)
+      end
+
+      assert_equal "position range starts inside a character: #{start}...10", exn.message
+    end
+
+    # Byte 5 opens `🐈`, so lexing starts exactly where it was asked to. This
+    # is the error the three positions above used to be silently rounded up
+    # to.
     exn = assert_raises RBS::ParsingError do
-      Parser.parse_type(input, byte_range: 2...)
+      Parser.parse_type(input, byte_range: 5...)
     end
 
     assert_equal "a.rbs:1:2...1:3: Syntax error: unexpected token for simple type, token=`🐈` (ErrorToken)", exn.message
   end
+
+  def test_parse__byte_range_starting_past_the_end
+    # Reaching `start_pos` means stepping to it, and there is nothing left to
+    # step over here, so the lexer used to walk forever.
+    exn = assert_raises ArgumentError do
+      Parser.parse_type("Integer", byte_range: 20...30)
+    end
+
+    assert_equal "position range starts past the end of the buffer: 20...30, buffer is 7 bytes", exn.message
+
+    # The end of the buffer itself is a boundary like any other, and an
+    # `end_pos` past it is how a caller clamps without measuring.
+    assert_nil Parser.parse_type("Integer", byte_range: 7...7)
+    assert_equal "Integer", Parser.parse_type("Integer", byte_range: 0...9999).to_s
+  end
+
+  def test_parse__byte_range_incorrect_in_euc_jp
+    # In EUC-JP the second byte of a character sits in the same range as a
+    # first byte, so decoding at the offset would happily report a character.
+    # Only the walk from the start of the buffer knows byte 1 is inside one.
+    euc = (+"\xC6\xFCFoo").force_encoding(Encoding::EUC_JP) # 日Foo
+
+    exn = assert_raises ArgumentError do
+      Parser.parse_type(euc, byte_range: 1...)
+    end
+
+    assert_equal "position range starts inside a character: 1...5", exn.message
+
+    assert_equal RBS::TypeName.parse("Foo"),
+                 Parser.parse_type(euc, byte_range: 2...).name
+  end
 end