InterpolatedSymbolNode start location is overwritten on subsequent interpolations in `%I` symbol arrays

Start in parse_interpolated_symbol_array in src/prism.c and compare its PM_TOKEN_EMBVAR and PM_TOKEN_EMBEXPR_BEGIN cases with the linked pull request #4336, which appears to contain related work. Reproduce with %I[#{1}#{2}] and inspect the interpolated symbol node's location bounds; done means the node ends within the source buffer and the regression is covered.

Fuzzing find on bdbd482dc.

In parse_interpolated_symbol_array (%I[...]), the PM_TOKEN_EMBVAR (#@a) and PM_TOKEN_EMBEXPR_BEGIN (#{...}) cases track a local boolean start_location_set = false that is only set to true when converting an existing PM_SYMBOL_NODE (created from a leading PM_TOKEN_STRING_CONTENT) into a PM_INTERPOLATED_SYMBOL_NODE.

When an element in a %I[...] symbol array begins with an interpolation (#{...} or #@a, so current is created directly as a PM_INTERPOLATED_SYMBOL_NODE) and contains a second interpolation (such as %I[#{1}#{2}]), start_location_set remains false on the second interpolation. As a result, PM_NODE_START_SET_NODE(current, part) overwrites current->location.start with the start offset of the second interpolation without adjusting current->location.length, shifting current->location.start + current->location.length past the actual end of the node (and past the end of the source buffer).

Reproduction

C AST Bounds Check (pm_parse + pm_visit_node)

Even on valid Ruby "%I[#{1}#{2}]" (12 bytes, interactive simulation in Prism Loupe):

#include "prism.h"
#include <inttypes.h>
#include <stdio.h>
#include <string.h>

static pm_visit_result_t
check_bounds(const pm_node_t *node, void *data) {
    size_t source_len = *(const size_t *) data;
    uint64_t end = (uint64_t) node->location.start + (uint64_t) node->location.length;

    if (end > source_len) {
        printf(
            "%-30s start=%" PRIu32 " length=%" PRIu32 " end=%" PRIu64 " > source_len=%zu\n",
            pm_node_type(node->type),
            node->location.start,
            node->location.length,
            end,
            source_len
        );
    }

    return PM_VISIT_DESCEND;
}

int main(void) {
    const char *src = "%I[#{1}#{2}]";
    size_t len = strlen(src);

    pm_arena_t *arena = pm_arena_new();
    pm_parser_t *parser = pm_parser_new(arena, (const uint8_t *) src, len, NULL);
    pm_node_t *root = pm_parse(parser);

    pm_visit_node(root, check_bounds, &len);

    pm_parser_free(parser);
    pm_arena_free(arena);
    return 0;
}

Building and running against libprism.a:

cc -Iinclude check.c build/libprism.a -o check && ./check

Output on the 12-byte input (0..12):

PM_INTERPOLATED_SYMBOL_NODE    start=7 length=8 end=15 > source_len=12

Root Cause

In parse_interpolated_symbol_array (src/prism.c), pm_interpolated_symbol_node_append already sets node->base.location = part->location when node->parts.size == 0 and updates PM_NODE_LENGTH_SET_NODE(node, part) when node->parts.size > 0:

static void
pm_interpolated_symbol_node_append(pm_arena_t *arena, pm_interpolated_symbol_node_t *node, pm_node_t *part) {
    if (node->parts.size == 0) {
        node->base.location = part->location;
    } else {
        PM_NODE_LENGTH_SET_NODE(node, part);
    }

    pm_interpolated_node_append(arena, UP(node), &node->parts, part);
}

However, both case PM_TOKEN_EMBVAR: and case PM_TOKEN_EMBEXPR_BEGIN: in parse_interpolated_symbol_array additionally maintain bool start_location_set = false;:

case PM_TOKEN_EMBEXPR_BEGIN: {
    bool start_location_set = false;
    if (current == NULL) {
        current = UP(pm_interpolated_symbol_node_create(parser, NULL, NULL, NULL));
    } else if (PM_NODE_TYPE_P(current, PM_SYMBOL_NODE)) {
        pm_interpolated_symbol_node_t *interpolated = pm_interpolated_symbol_node_create(parser, NULL, NULL, NULL);

        current = UP(pm_symbol_node_to_string_node(parser, (pm_symbol_node_t *) current));
        pm_interpolated_symbol_node_append(parser->arena, interpolated, current);
        PM_NODE_START_SET_NODE(interpolated, current);
        start_location_set = true;
        current = UP(interpolated);
    } else if (PM_NODE_TYPE_P(current, PM_INTERPOLATED_SYMBOL_NODE)) {
        // start_location_set is NOT set to true here!
    }

    pm_node_t *part = parse_string_part(parser, (uint16_t) (depth + 1));
    pm_interpolated_symbol_node_append(parser->arena, (pm_interpolated_symbol_node_t *) current, part);
    if (!start_location_set) {
        PM_NODE_START_SET_NODE(current, part);
    }
    break;
}

When an element starts with an interpolation (current == NULL on the first part) and has a second interpolation (PM_NODE_TYPE_P(current, PM_INTERPOLATED_SYMBOL_NODE) on the second part):

1. pm_interpolated_symbol_node_append updates current->location.length = PM_NODE_END(part2) - PM_NODE_START(current).
2. Because start_location_set is still false, if (!start_location_set) PM_NODE_START_SET_NODE(current, part2) then overwrites current->location.start with PM_NODE_START(part2) while leaving current->location.length equal to PM_NODE_END(part2) - PM_NODE_START(part1).
3. Consequently, PM_NODE_END(current) (start + length) becomes PM_NODE_START(part2) + (PM_NODE_END(part2) - PM_NODE_START(part1)), extending past the end of part2 and potentially past parser->end.

Note

Looks like this is already known https://github.com/ruby/prism/pull/4336/changes/c7673f57940d57388ce43cad07ef65a23862c78a?