Reject a byte position the lexer cannot start on

`rbs_lexer_new` reaches `start_pos` by stepping one character at a time
from the beginning of the buffer, so only the first byte of a character
is a position it can stand on.

Inside a character the step goes over `start_pos` and lexing quietly
begins at the next one. Nothing downstream can tell: the walk keeps
`line`, `column` and `char_pos` consistent, so the result points at a
real position that simply is not the one asked for. The shift is caught
by accident when what it lands on cannot open a token, but an ASCII
letter right after the multibyte character parses cleanly in the wrong
place -- and #3082 gives the non-ASCII characters tokens of
their own, taking even the accident away.

Past the end of the buffer there is nothing left to step over, so the
walk never finishes: `rbs_skip` stops moving at EOF while the loop
waits for a position it will never reach. `parse_type("Integer",
byte_range: 20...30)` hangs on master.

`rbs_lexer_new` returns `NULL` for a `start_pos` it cannot reach, and
the extension turns that into an `ArgumentError` alongside the reversed
and negative ranges, since all are the caller's mistake rather than
anything about the source text. Starting past the end is plain from the
buffer's size, so the extension rejects it before parsing; a `NULL`
that comes back after that check can only mean a start inside a
character.

`end_pos` keeps taking any value: clamping with a large number instead
of measuring the buffer is ordinary, and the lexer stops at the end on
its own.

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