Potenitally incorrect frame border check in pop_values

[Harrm]

Hi!
I'm implementing the stack limiter logic for KAGOME, an alternative Polkadot Host implementation, and I'm confused by this line:
https://github.com/paritytech/wasm-instrument/blob/39fa497aeec3f35c0d9419ff804aa3beae22c2d5/src/stack_limiter/max_height.rs#L106C2-L106C2

Shouldn't the check be

if self.height - value_count < top_frame.start_height

to make sure that neither of the popped values exceed the frame border?

[athei]

self.height == top_frame.start_height means that there is no value on the stack. So it doesn't matter how many (as long as > 0) you pop it will always be too much.

The check you are expecting is further down in the function:

self.height = self.height.checked_sub(value_count).ok_or("stack underflow")?;

[Harrm]

Thanks for the quick reply.
The way I understand it, if the stack height is, say, 42, top_frame.start_height is 40, and pop_values is called with the count of 5, then
self.height == top_frame.start_height is false, so this check passes
self.height = self.height.checked_sub(value_count) passes, self.height becomes 37,
but the stack height decreases lower than the frame start_height. Isn't that an error?

[vanhauser-thc]

I think @athei is correct.
The misunderstanding is the expectation that the stack frame height after popping all values is expected to be positive.

Below in the code we see in the frame initialization:

start_height: 0

Hence every function frame starts at 0 height and the checked_sub is enough.

[Harrm]

But the checked_sub subtracts from the global stack height, not from the start_height of the frame. Besides, I think the start_height of a function frame is not 0 in general, but the global height of the stack at the moment of creating this stack frame:
https://github.com/paritytech/wasm-instrument/blob/39fa497aeec3f35c0d9419ff804aa3beae22c2d5/src/stack_limiter/max_height.rs#L189C7-L189C7
Please correct me if I'm wrong.

UPD: Ah, I see, function frames indeed start with height 0. However, the comment in check states that the current frame border should not be crossed by popping values, not only a function frame.