Skip to content

buffer_write indexes by an unchecked head, and the corpus bounds slice writes by four different things #3430

Description

@gHashTag

Follow-up to #3428, which I closed saying I had not measured how widespread that shape was.

The audit, and how much of it my own matchers got wrong

Of 4692 emitted functions, 67 take a slice parameter and 59 index into one. Classifying what bounds the index took three matcher attempts, and each earlier answer was wrong in a way worth recording:

  1. "34 write without bounding by .len()" — true and useless. Most of those bound by an explicit length parameter, which is the only thing that can work in C.
  2. "3 have a length parameter and do not use it as the bound"two of the three were false positives. ternary_shift_right starts its loop at len - 1; my regex looked for while (... len ...) and missed it.
  3. "24 are bounded by a compile-time constant" — inflated. TRIT_NEG, TRIT_POS and TRIT_ZERO are values, not bounds, and matching "a declared constant appears in the body" swept in byte_to_trits, which I had already hand-verified as correctly bounded.

Hand-verified, ten functions read by eye: two were matcher false positives, and eight genuinely bound by something other than the buffer. I did not verify all 59, so the class size is somewhere between those eight and the matchers 24, and I am not going to publish a single number for it.

The four bounds actually in use

bound example safe in C?
the buffers own .len() pipeline_run after #3429 no — C has no length
an explicit length parameter byte_to_trits(byte, trits, len), mac_parallel_multiply(..., count) yes
a compile-time constant hash_insert bounded by TABLE_SIZE, mem_store by MEM_SIZE, set_insert by SET_MAX only if the caller matches it
nothing buffer_write no

cache_kv deserves its own line: it writes cache_k[position * EMBED_DIM + i] with i < EMBED_DIM, so the index grows with position, and nothing bounds position.

The one that is unambiguous, and is fixed here

fn buffer_write(buf_in: []u8, size: usize, head: usize, data: u8) -> bool {
    const new_head = (head + 1) % size;
    if (new_head == 0 && head == size - 1) { return false; }
    buf_in[head] = data;

head is never compared to size. The evidence that this is an oversight rather than a convention is inside the same file: the sibling buffer_read does guard its index. And its guard is tail == size, which catches exactly one-past-the-end and lets every larger index through — so both are corrected to >=.

The bound uses size, not buf_in.len, deliberately. A []T loses its length at the C ABI, so a .len-based guard exists in Rust and Zig and is absent from C. Using the parameter puts the identical check in all four:

bool buffer_write(uint8_t* buf_in, size_t size, size_t head, uint8_t data) {
    if ((head >= size)) { return false; }
pub fn buffer_write(buf_in: &mut [u8], size: usize, head: usize, data: u8) -> bool {
    if (head >= size) { return false; }

Left for a decision

The constant-bounded set — hash_insert, hash_remove, mem_store, mem_store_block, set_insert, cache_kv, and the two gla_* — is a convention with an unstated precondition: "your buffer must be at least CONST long". It is coherent, and nothing enforces it in any backend. Changing it means adding length parameters, which changes the ABI.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions