Page MenuHomePhabricator

libunwind: Evaluating DWARF operation DW_OP_pick is broken
ClosedPublic

Authored by kamleshbhalui on Nov 6 2019, 6:59 AM.

Details

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

Diff Detail

Event Timeline

kamleshbhalui created this revision.Nov 6 2019, 6:59 AM
kamleshbhalui edited the summary of this revision. (Show Details)Nov 6 2019, 7:00 AM
kamleshbhalui added a project: Restricted Project.
ldionne accepted this revision.Nov 6 2019, 7:44 AM
ldionne added a reviewer: kledzik.

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.

This revision is now accepted and ready to land.Nov 6 2019, 7:44 AM

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 added a comment.EditedNov 6 2019, 3:17 PM
`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;
}
---------`

Yes, Tested All tests passes.

Testing Time: 0.95s

Expected Passes    : 4

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

kamleshbhalui added a comment.EditedDec 13 2019, 11:27 PM

ping?
can someone commit this for me?

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

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 accepted this revision.Dec 18 2019, 12:19 PM

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.

Commited in: 9366397f057d18401e680b2cb28a0ee17c59d4a6

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

This revision was automatically updated to reflect the committed changes.