dwarf_next_cfi returns -1

dwarf_next_cfi returns -1

[Sasha Da Rocha Pinheiro]

Hi,
We had a case where dwarf_next_cfi returns -1 but the offset does not update, as we should expect by the comment:

 330    On errors, returns -1.  Some format errors will permit safely
 331    skipping to the next CFI entry though the current one is unusable.
 332    In that case, *NEXT_OFF will be updated before a -1 return.
Is there a correct way to deal with it, or just check if the NEXT_OFF had changed?

 Sasha

Re: dwarf_next_cfi returns -1

[Mark Wielaard]

On Mon, 2018-06-04 at 16:16 +0000, Sasha Da Rocha Pinheiro wrote:
> We had a case where dwarf_next_cfi returns -1 but the offset does not
> update, as we should expect by the comment:
> 
>  330    On errors, returns -1.  Some format errors will permit safely
>  331    skipping to the next CFI entry though the current one is
> unusable.
>  332    In that case, *NEXT_OFF will be updated before a -1 return.
> Is there a correct way to deal with it, or just check if the NEXT_OFF
> had changed?

A CFI entry starts with the length of that entry, so dwarf_next_cfi can
often setup the next offset correctly. But if there is anything else
"wrong" with the CFI entry (maybe it has a version or augmentation
string not recognized) then it cannot reliably return the CFI entry. It
is indeed a slightly inconvenient interface, you'll have to check
whether the return value, if it is zero, all is fine, if it is 1 you
reached the end, if it is -1 an error occurred. In that last case, if
you really want to try you can try. *next_off having changed after the
call means it might work (or not).

If you have an example of a "wrong" (or not recognized) CFI that would
be helpful.

Thanks,

Mark

Re: dwarf_next_cfi returns -1

[Sasha Da Rocha Pinheiro]

This is a binary that infinite loops with dwarf_next_cfi -1 because the offset is not updated.
https://rice.box.com/s/yzul9oavplq1qdx12ozjpgssawea36xy

A fix was done by saving the previous *next_off and comparing with the current, after getting -1 in the return value.



Sasha
  
From: Mark Wielaard <mark@klomp.org>
Sent: Tuesday, June 5, 2018 6:27:17 AM
To: Sasha Da Rocha Pinheiro; elfutils-devel@sourceware.org
Subject: Re: dwarf_next_cfi returns -1
  

On Mon, 2018-06-04 at 16:16 +0000, Sasha Da Rocha Pinheiro wrote:
> We had a case where dwarf_next_cfi returns -1 but the offset does not
> update, as we should expect by the comment:
> 
>  330    On errors, returns -1.  Some format errors will permit safely
>  331    skipping to the next CFI entry though the current one is
> unusable.
>  332    In that case, *NEXT_OFF will be updated before a -1 return.
> Is there a correct way to deal with it, or just check if the NEXT_OFF
> had changed?

A CFI entry starts with the length of that entry, so dwarf_next_cfi can
often setup the next offset correctly. But if there is anything else
"wrong" with the CFI entry (maybe it has a version or augmentation
string not recognized) then it cannot reliably return the CFI entry. It
is indeed a slightly inconvenient interface, you'll have to check
whether the return value, if it is zero, all is fine, if it is 1 you
reached the end, if it is -1 an error occurred. In that last case, if
you really want to try you can try. *next_off having changed after the
call means it might work (or not).

If you have an example of a "wrong" (or not recognized) CFI that would
be helpful.

Thanks,

Mark
    

Re: dwarf_next_cfi returns -1

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 851 bytes --]

On Wed, Jun 27, 2018 at 11:01:25PM +0000, Sasha Da Rocha Pinheiro wrote:
> This is a binary that infinite loops with dwarf_next_cfi -1 because the offset is not updated.
> https://rice.box.com/s/yzul9oavplq1qdx12ozjpgssawea36xy
> 
> A fix was done by saving the previous *next_off and comparing with the current, after getting -1 in the return value.

That is probably the best way to handle that.

Looking at the file I see it has (multiple) zero terminators, that
dwarf_next_cfi seems to not handle. Strangely these aren't described
in the Dwarf spec, but they are mentioned in the LSB exception frames
spec: https://refspecs.linuxfoundation.org/LSB_5.0.0/LSB-Core-generic/LSB-Core-generic/ehframechpt.html#EHFRAME

Totally untested patch attached. If you could test it that would be
wonderful. I'll write a proper testcase tomorrow.

Thanks,

Mark

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 919 bytes --]

diff --git a/libdw/dwarf_next_cfi.c b/libdw/dwarf_next_cfi.c
index 53fc3697..fa28d99b 100644
--- a/libdw/dwarf_next_cfi.c
+++ b/libdw/dwarf_next_cfi.c
@@ -54,6 +54,7 @@ dwarf_next_cfi (const unsigned char e_ident[],
 	 we don't know yet whether this is a 64-bit object or not.  */
       || unlikely (off + 4 >= data->d_size))
     {
+    done:
       *next_off = (Dwarf_Off) -1l;
       return 1;
     }
@@ -79,6 +80,13 @@ dwarf_next_cfi (const unsigned char e_ident[],
 	}
       length = read_8ubyte_unaligned_inc (&dw, bytes);
     }
+
+  /* Not explicitly in the DWARF spec, but mentioned in the LSB exception
+     frames (.eh_frame) spec. If Length contains the value 0, then this
+     CIE shall be considered a terminator and processing shall end.  */
+  if (length == 0)
+    goto done;
+
   if (unlikely ((uint64_t) (limit - bytes) < length)
       || unlikely (length < offset_size + 1))
     goto invalid;