libunwind: Evaluating DWARF operation DW_OP_pick is broken

[kamleshbhalui]

Summary

reg is unsigned type and used here for getting array element from the end by negating it.
negation of unsigned can result in large number and array access with that index will result in segmentation
fault.
As a Fix we cast reg to int then negate it.
Fixes this.
https://bugs.llvm.org/show_bug.cgi?id=43872

[ldionne]

This looks good to me, but I must admit I'm surprised this problem has never come up before (this code is very old), and also I don't know what the code is trying to do. Adding Saleem and Nick who have more experience with libunwind just to double-check.

[probinson]

Is it tested? Intuitively I would expect DW_OP_pick to be kind of an unusual operator, unlikely to be seen in the wild.

[kamleshbhalui]

`I do not have a test related to libunwind ,but I do simulated this behavior in c and got segfault.
------
int main(){

	int stack[10]={0};
	int* sp=stack;
	*(sp)=1;
	*(++sp)=2;
	*(++sp)=3;
	unsigned int r=1;
	int d=sp[-r];
	return 0;
}
---------`

[kamleshbhalui]

Yes, Tested All tests passes.

Testing Time: 0.95s

Expected Passes    : 4

[kamleshbhalui]

@ldionne Can you please commit this for me.
I do not have commit access.

[kamleshbhalui]

ping?
can someone commit this for me?

[steven_wu]

The fix LGTM. Do you have a reproducer that can be used as a testcase? We should really add more tests for libunwind.

[kamleshbhalui]

In D69893#1786202, @steven_wu wrote:

The fix LGTM. Do you have a reproducer that can be used as a test case? We should really add more tests for libunwind.

I currently do not have a reproducer test case for this.

[steven_wu]

In D69893#1786222, @kamleshbhalui wrote:

In D69893#1786202, @steven_wu wrote:

The fix LGTM. Do you have a reproducer that can be used as a test case? We should really add more tests for libunwind.

I currently do not have a reproducer test case for this.

Ok. We will need to come back dealing with test coverage in the future. I can commit it for you.

[steven_wu]

Commited in: 9366397f057d18401e680b2cb28a0ee17c59d4a6

Phabriactor might not update this because the patch was created on libunwind repo, not the monorepo.

libunwind/src/DwarfInstructions.hpp

case DW_OP_over:
       if (log)
         fprintf(stderr, "duplicate second in stack\n");
       break;
 
     case DW_OP_pick:
       // pick from
       reg = addressSpace.get8(p);
       p += 1;
-      value = sp[-reg];
+      value = sp[-(int)reg];
       *(++sp) = value;
       if (log)
         fprintf(stderr, "duplicate %d in stack\n", reg);
       break;
 
     case DW_OP_swap:
       // swap top two
       value = sp[0];