[PATCH] D69893: libunwind: Evaluating DWARF operation DW_OP_pick is broken

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

Commited in: 9366397f057d18401e680b2cb28a0ee17c59d4a6 


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


Repository:
  rUNW libunwind

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69893/new/

https://reviews.llvm.org/D69893



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69893: libunwind: Evaluating DWARF operation DW_OP_pick is broken

[Steven Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9366397f057d (authored by steven_wu).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69893/new/

https://reviews.llvm.org/D69893

Files:
  libunwind/src/DwarfInstructions.hpp


Index: libunwind/src/DwarfInstructions.hpp
===
--- libunwind/src/DwarfInstructions.hpp
+++ libunwind/src/DwarfInstructions.hpp
@@ -433,7 +433,7 @@
   // 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);


Index: libunwind/src/DwarfInstructions.hpp
===
--- libunwind/src/DwarfInstructions.hpp
+++ libunwind/src/DwarfInstructions.hpp
@@ -433,7 +433,7 @@
   // 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);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69893: libunwind: Evaluating DWARF operation DW_OP_pick is broken

[Steven Wu via Phabricator via cfe-commits]

steven_wu accepted this revision.
steven_wu added a comment.

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.


Repository:
  rUNW libunwind

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69893/new/

https://reviews.llvm.org/D69893



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69893: libunwind: Evaluating DWARF operation DW_OP_pick is broken

[kamlesh kumar via Phabricator via cfe-commits]

kamleshbhalui added a comment.

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.


Repository:
  rUNW libunwind

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69893/new/

https://reviews.llvm.org/D69893



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69893: libunwind: Evaluating DWARF operation DW_OP_pick is broken

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

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


Repository:
  rUNW libunwind

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69893/new/

https://reviews.llvm.org/D69893



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69893: libunwind: Evaluating DWARF operation DW_OP_pick is broken

[kamlesh kumar via Phabricator via cfe-commits]

kamleshbhalui added a comment.

ping?
can someone commit this for me?


Repository:
  rUNW libunwind

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69893/new/

https://reviews.llvm.org/D69893



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69893: libunwind: Evaluating DWARF operation DW_OP_pick is broken

[kamlesh kumar via Phabricator via cfe-commits]

kamleshbhalui added a comment.

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


Repository:
  rUNW libunwind

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69893/new/

https://reviews.llvm.org/D69893



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69893: libunwind: Evaluating DWARF operation DW_OP_pick is broken

[kamlesh kumar via Phabricator via cfe-commits]

kamleshbhalui added a comment.

Yes, Tested All tests passes.

Testing Time: 0.95s

  Expected Passes: 4


Repository:
  rUNW libunwind

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69893/new/

https://reviews.llvm.org/D69893



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69893: libunwind: Evaluating DWARF operation DW_OP_pick is broken

[kamlesh kumar via Phabricator via cfe-commits]

kamleshbhalui added a comment.

`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;
}
-`


Repository:
  rUNW libunwind

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69893/new/

https://reviews.llvm.org/D69893



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69893: libunwind: Evaluating DWARF operation DW_OP_pick is broken

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

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


Repository:
  rUNW libunwind

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69893/new/

https://reviews.llvm.org/D69893



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69893: libunwind: Evaluating DWARF operation DW_OP_pick is broken

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a reviewer: kledzik.
ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

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.


Repository:
  rUNW libunwind

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69893/new/

https://reviews.llvm.org/D69893



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69893: libunwind: Evaluating DWARF operation DW_OP_pick is broken

[kamlesh kumar via Phabricator via cfe-commits]

kamleshbhalui created this revision.
kamleshbhalui added a reviewer: phosek.
Herald added subscribers: libcxx-commits, ldionne, christof.
kamleshbhalui edited the summary of this revision.

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


Repository:
  rUNW libunwind

https://reviews.llvm.org/D69893

Files:
  libunwind/src/DwarfInstructions.hpp


Index: libunwind/src/DwarfInstructions.hpp
===
--- libunwind/src/DwarfInstructions.hpp
+++ libunwind/src/DwarfInstructions.hpp
@@ -430,7 +430,7 @@
   // 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);


Index: libunwind/src/DwarfInstructions.hpp
===
--- libunwind/src/DwarfInstructions.hpp
+++ libunwind/src/DwarfInstructions.hpp
@@ -430,7 +430,7 @@
   // 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);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits