Validate individual offset values in BULK_OFFSETS bounds checks

[ChrisHegarty]

While working on bulk sparse scoring (#144557), I noticed that checkBulkOffsets and checkBBQBulkOffsets validated segment sizes but not individual offset values. An out-of-range or negative offset would silently read memory beyond the data segment, risking a crash or silently wrong results.

The solution is to replace the sequential size check with per-offset validation that checks each offset points to a valid vector within the data segment. The O(count) loop should be negligible relative to the O(count * dims) native call, but we've made the checks conditional on asserts to avoid any potential negative cost of this, and asserts should be good enough given our testing.

Note: INT4 skips size=2 (packedLen=1) because checkBulkOffsets computes rowBytes = packedLen * 4 / 8 which truncates to 0 via integer division, making the bounds check trivially pass. This is a pre-existing issue with how INT4 passes packed byte length (not element count) as the length parameter to the generic check formula. We can address this separately, if needed.

[elasticsearchmachine]

Pinging @elastic/es-search-relevance (Team:Search Relevance)

[benwtrent]

This would have been a very sneaky bug to track down!

[ldematte]

Note: INT4 skips size=2 (packedLen=1) because checkBulkOffsets computes rowBytes = packedLen * 4 / 8 which truncates to 0 via integer division, making the bounds check trivially pass. This is a pre-existing issue with how INT4 passes packed byte length (not element count) as the length parameter to the generic check formula. We can address this separately, if needed.

I think @thecoop already noticed and fixed that?

[ldematte]

I like the change, but I wonder if we should make that a "assert-like" check, running only with assertions enabled (e.g. in tests)

[ChrisHegarty]

I think @thecoop already noticed and fixed that?

It's still broken in main, but I did not try to fix it here. Just test and avoid it for noe.

I like the change, but I wonder if we should make that a "assert-like" check, running only with assertions enabled (e.g. in tests)

I dunno. Maybe. I am worried about dereferencing the wrong memory, so checks are super important. The existing checks are always present, so I just followed the same pattern - tho this is expanding somewhat. I don't think that it will have any real noticeable affect on performance. However we could check afterwards and move these to asserts is needed?

[ldematte]

I was already not 100% sure about the existing checks to be honest :)
I'm not sure how "light" they are, even compared with bulk operations. Remember, even the cost of calling a function is visible, with the small-ish bulk sizes we have and the level of optimization we get with SIMD, we are talking about nanoseconds. But you are probably right and I'm worrying for nothing.

I am worried about dereferencing the wrong memory, so checks are super important

Well, that ship has sailed the day we decided to go with native code I think :D It's true that in this case the check is meaningful though.

[thecoop]

Do we have info on what performance effect this has? It's changing the check from O(1) to some kind of O(n), so it's going to have some effect.

[thecoop]

Could we have a more in-depth check with assertions, and a O(1) top-level sanity check for production? All the code paths should be covered during tests, so there should be no need to run the full checks in production...right?

[ldematte]

Could we have a more in-depth check with assertions, and a O(1) top-level sanity check for production? All the code paths should be covered during tests, so there should be no need to run the full checks in production...right?

That could be a good middle ground; my suggestion was along the same lines, but a bit more radical -- assertions should be enough as tests should cover us, and we should have validation somewhere so that it's not possible to generate invalid data (e.g. ordinals that are negative or > the num of vectors).

[ChrisHegarty]

The O(count) per-offset bounds checks in checkBulkOffsets and checkBBQBulkOffsets are moved into separate validateBulkOffsets / validateBBQBulkOffsets methods, called via assert so they have zero cost in production. The validate methods also add alignment checks on the offsets/result segments and non-negative/positive guards on count, length, and pitch.

[ldematte]

LGTM!
But what about Int4? Is that fixed already, or do we want to address it here, or in a separate PR?

[ChrisHegarty]

LGTM! But what about Int4? Is that fixed already, or do we want to address it here, or in a separate PR?

I already added an int4 unit test in this PR, so it should be covered and verified. Is there something else that I'm missing?

[ldematte]

I already added an int4 unit test in this PR, so it should be covered and verified. Is there something else that I'm missing?

I don't know.. probably not, I was referring to the note in the PR description. If that's solved, probably you just need to update the description.

[ChrisHegarty]

I already added an int4 unit test in this PR, so it should be covered and verified. Is there something else that I'm missing?

I don't know.. probably not, I was referring to the note in the PR description. If that's solved, probably you just need to update the description.

Oh yeah. That's a separate issue. If we want to do it at all.