duk_api_stack: increase memory safety

[pzalewski-thegoodpenguin]

The ptr_diff is taken between the post reallocation new_valstack pointer and pre-allocation thr->valstack pointer variables. Realloc function might change the base pointer, so there might be a difference between thr->valstack and new_valstack, which appears to be the assumption here that this will not be the case ?

Further on this difference is then added to the old base pointers for bottom/top/end thr->valstack variables, with only thr->valstack being repalced with the new base pointer. So we have the difference between two base pointers added to the old base pointer effectively as if the memory was contiguous.

Lets change that and add the pointer differences to the new_valstack base pointer obtained from the realloc function instead.

[svaarala]

The ptr_diff is taken between the post reallocation new_valstack pointer and pre-allocation thr->valstack pointer variables. Realloc function might change the base pointer, so there might be a difference between thr->valstack and new_valstack, which appears to be the assumption here that this will not be the case ?

No, that's not assumed by the code. ptr_diff is the difference between the new and the old base pointer, which may be zero or otherwise. To translate any pointer relative to thr->valstack to the new pointer base, it suffices to add ptr_diff to the old pointer to in effect switch the base.

So for example, suppose:

• Old thr->valstack = A
• New thr->valstack = B
• ptr_diff = B - A
• Old thr->valstack_top = A + 10
• New thr->valstack_top that we want is: B + 10
• To get to B + 10 from A + 10 one needs to add: (B + 10) - (A + 10) = B - A = ptr_diff
• So, new thr->valstack_top can be computed as: old thr->valstack_top + ptr_diff = (A + 10) + (B - A) = B + 10

Same applies to the other pointers.

Perhaps a better name for ptr_diff would be base_ptr_diff which reflects the purpose more accurately?

[pzalewski-thegoodpenguin]

Oh I am a numpty, thank you for spelling it out for me. The commit message is based on my old notes, which are outdate and only the first sentence is relevant. My apologies I might be too old for this..

The problem here is that post realloc the old pointer is invalid as that memory could have been deallocated, yet that adress is used here for arithmetic - mathematically nothing wrong with it, as proven above, semantically in terms of C standard this is undefined. The pointer difference should be taken between two valid pointers.This is in section 6.2.4 of the standard on storage durations of objects. The second issue is that even if the two pointers were valid, the difference is taken between two allocations, which is also against the standard C. I know that a pointer is just an address/integer but...

Some context: I have been using duktape via Zabbix on an ARM Morello, implementation of CHERI architecture, where a pointer is a new datatype at the architecture level and is of size 16 not 8 bytes. The pointer you get after realloc is effectively a new data object with its bounds metadata set in hardware, thus adding an offset to the old pointer object is not longer even possible as you will violate the old bounds and get a segfault. So the C standard is enforced by hardware in this case one could say.

Good news is that after implementing alingment to 16 and fixing the above offset issue duktape had no runtime issues thus far on this hardware (please note this was by no means full coverage just normal usage - but self test did pass). So the above patch would future proof that routine for CHERI based platforms and make the code std C compliant.

Your proposition of renaming the ptr_diff variable is very good, if you would rather keep it the way it is for now. However the issue remains that the original pointer after realloc is not a valid pointer in std C as it points to deallocated memory, so one should try not to operate on it at all as it is possible that its integer address could be undefined, so you can be at the mercy of the compiler effectively which is not a great place to be.

Again sorry for the original commit message, it is wrong and needs to be corrected.

[svaarala]

Thanks for the clarifications, could you comment on this:

However the issue remains that the original pointer after realloc is not a valid pointer in std C as it points to deallocated memory, so one should try not to operate on it at all as it is possible that its integer address could be undefined

So do you mean that it's undefined behavior just to do pointer arithmetic on the old pointer? It's clear that dereferencing it is undefined behavior, but is mere arithmetic also prohibited? I'm not a standards expert but this would break a lot of common practice like using void * for userdata where the value may be an arbitrary integer, etc.

C99 Section 6.2.4 says this:

If an object is referred to outside of its lifetime, the behavior is undefined

But I understand it as referring to the object, i.e. dereferencing the pointer and not just holding/computing with it.

The pointer you get after realloc is effectively a new data object with its bounds metadata set in hardware, thus adding an offset to the old pointer object is not longer even possible as you will violate the old bounds and get a segfault. So the C standard is enforced by hardware in this case one could say.

This sounds quite interesting :-)

[pzalewski-thegoodpenguin]

So do you mean that it's undefined behavior just to do pointer arithmetic on the old pointer? It's clear that dereferencing it is undefined behavior, but is mere arithmetic also prohibited? I'm not a standards expert but this would break a lot of common practice like using void * for userdata where the value may be an arbitrary integer, etc.

So the full body of paragraph 2 in section 6.2.4 is:

The lifetime of an object is the portion of program execution during which storage is
guaranteed to be reserved for it. An object exists, has a constant address,25) and retains
its last-stored value throughout its lifetime.2 If an object is referred to outside of its lifetime, the behavior is undefined. The value of a pointer becomes indeterminate when the object it points to reaches the end of its lifetime.

From this my understanding is that the value of a pointer itself also is invalid if end of lifetime was reached for an object it points to, so the best practice would be not to operate on old pointers at all.

[pzalewski-thegoodpenguin]

Closing as there is no interest in this.