[PATCH 2/7] [D] libiberty: Fail if reached end of symbol string

[PATCH 2/7] [D] libiberty: Fail if reached end of symbol string

[Iain Buclaw]

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

If a symbol that has so far been valid abruptly ends then we will want
to fail the process rather than silently succeed.

---
libiberty/ChangeLog

2015-05-13 Iain Buclaw  <ibuclaw@gdcproject.org>

    * d-demangle.c (dlang_call_convention): Return NULL if have reached the
    end of the symbol, but expected more.
    (dlang_attributes): Likewise.
    (dlang_function_type): Likewise.
    (dlang_type): Likewise.
    (dlang_identifier): Likewise.
    (dlang_value): Likewise.

[-- Attachment #2: 0002-D-demangle-Fail-early-on-bad-symbols.patch --]
[-- Type: text/x-diff, Size: 1845 bytes --]

From e9806a182f8296d92a99d842edab6a4c29124eb1 Mon Sep 17 00:00:00 2001
From: Iain Buclaw <ibuclaw@gdcproject.org>
Date: Wed, 13 May 2015 08:50:55 +0200
Subject: [PATCH 2/7] D demangle: Fail early on bad symbols

---
 libiberty/d-demangle.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/libiberty/d-demangle.c b/libiberty/d-demangle.c
index fa01767..91c6d55 100644
--- a/libiberty/d-demangle.c
+++ b/libiberty/d-demangle.c
@@ -185,7 +185,7 @@ static const char *
 dlang_call_convention (string *decl, const char *mangled)
 {
   if (mangled == NULL || *mangled == '\0')
-    return mangled;
+    return NULL;
 
   switch (*mangled)
     {
@@ -221,7 +221,7 @@ static const char *
 dlang_attributes (string *decl, const char *mangled)
 {
   if (mangled == NULL || *mangled == '\0')
-    return mangled;
+    return NULL;
 
   while (*mangled == 'N')
     {
@@ -280,7 +280,7 @@ dlang_function_type (string *decl, const char *mangled)
   size_t szattr, szargs, sztype;
 
   if (mangled == NULL || *mangled == '\0')
-    return mangled;
+    return NULL;
 
   /* The order of the mangled string is:
 	CallConvention FuncAttrs Arguments ArgClose Type
@@ -380,7 +380,7 @@ static const char *
 dlang_type (string *decl, const char *mangled)
 {
   if (mangled == NULL || *mangled == '\0')
-    return mangled;
+    return NULL;
 
   switch (*mangled)
     {
@@ -600,7 +600,7 @@ static const char *
 dlang_identifier (string *decl, const char *mangled)
 {
   if (mangled == NULL || *mangled == '\0')
-    return mangled;
+    return NULL;
 
   if (ISDIGIT (*mangled))
     {
@@ -1061,7 +1061,7 @@ static const char *
 dlang_value (string *decl, const char *mangled, const char *name, char type)
 {
   if (mangled == NULL || *mangled == '\0')
-    return mangled;
+    return NULL;
 
   switch (*mangled)
     {
-- 
2.1.0

Re: [PATCH 2/7] [D] libiberty: Fail if reached end of symbol string

[Jeff Law]

On 05/13/2015 02:51 AM, Iain Buclaw wrote:
> If a symbol that has so far been valid abruptly ends then we will want
> to fail the process rather than silently succeed.
>
> ---
> libiberty/ChangeLog
>
> 2015-05-13 Iain Buclaw  <ibuclaw@gdcproject.org>
>
>      * d-demangle.c (dlang_call_convention): Return NULL if have reached the
>      end of the symbol, but expected more.
>      (dlang_attributes): Likewise.
>      (dlang_function_type): Likewise.
>      (dlang_type): Likewise.
>      (dlang_identifier): Likewise.
>      (dlang_value): Likewise.
I spot checked various callers of these functions that not return NULL 
and they looked reasonable. Though I was a bit concerned about the 
callers of dlang_type, dlang_value and dlang_identifier.

In those cases we'll often still do the string_append, string_setlength 
and other calls in the caller of dlang_{value,type,identifier}.  I'm 
assuming that's safe (the error still appears to be bubbling up properly).

If you can confirm that we're OK in those cases, then this is OK for the 
trunk.

jeff

Re: [PATCH 2/7] [D] libiberty: Fail if reached end of symbol string

[Iain Buclaw]

On 14 May 2015 at 14:58, Jeff Law <law@redhat.com> wrote:
> On 05/13/2015 02:51 AM, Iain Buclaw wrote:
>>
>> If a symbol that has so far been valid abruptly ends then we will want
>> to fail the process rather than silently succeed.
>>
>> ---
>> libiberty/ChangeLog
>>
>> 2015-05-13 Iain Buclaw  <ibuclaw@gdcproject.org>
>>
>>      * d-demangle.c (dlang_call_convention): Return NULL if have reached
>> the
>>      end of the symbol, but expected more.
>>      (dlang_attributes): Likewise.
>>      (dlang_function_type): Likewise.
>>      (dlang_type): Likewise.
>>      (dlang_identifier): Likewise.
>>      (dlang_value): Likewise.
>
> I spot checked various callers of these functions that not return NULL and
> they looked reasonable. Though I was a bit concerned about the callers of
> dlang_type, dlang_value and dlang_identifier.
>
> In those cases we'll often still do the string_append, string_setlength and
> other calls in the caller of dlang_{value,type,identifier}.  I'm assuming
> that's safe (the error still appears to be bubbling up properly).
>

The string routines should be safe for that (appending a string with a
zero length does nothing, for instance).

> If you can confirm that we're OK in those cases, then this is OK for the
> trunk.
>

I can start fuzzing mangle strings to test that failures actually fail
gracefully.  There's already a fuzzer that exists for C++, I think the
only change that would be required for D is exchanging "_Z" for "_D"
and calling cplus_demangle with DMGL_DLANG.

Regards
Iain

Re: [PATCH 2/7] [D] libiberty: Fail if reached end of symbol string

[Jeff Law]

On 05/14/2015 07:36 AM, Iain Buclaw wrote:
> On 14 May 2015 at 14:58, Jeff Law <law@redhat.com> wrote:
>> On 05/13/2015 02:51 AM, Iain Buclaw wrote:
>>>
>>> If a symbol that has so far been valid abruptly ends then we will want
>>> to fail the process rather than silently succeed.
>>>
>>> ---
>>> libiberty/ChangeLog
>>>
>>> 2015-05-13 Iain Buclaw  <ibuclaw@gdcproject.org>
>>>
>>>       * d-demangle.c (dlang_call_convention): Return NULL if have reached
>>> the
>>>       end of the symbol, but expected more.
>>>       (dlang_attributes): Likewise.
>>>       (dlang_function_type): Likewise.
>>>       (dlang_type): Likewise.
>>>       (dlang_identifier): Likewise.
>>>       (dlang_value): Likewise.
>>
>> I spot checked various callers of these functions that not return NULL and
>> they looked reasonable. Though I was a bit concerned about the callers of
>> dlang_type, dlang_value and dlang_identifier.
>>
>> In those cases we'll often still do the string_append, string_setlength and
>> other calls in the caller of dlang_{value,type,identifier}.  I'm assuming
>> that's safe (the error still appears to be bubbling up properly).
>>
>
> The string routines should be safe for that (appending a string with a
> zero length does nothing, for instance).
>
>> If you can confirm that we're OK in those cases, then this is OK for the
>> trunk.
>>
>
> I can start fuzzing mangle strings to test that failures actually fail
> gracefully.  There's already a fuzzer that exists for C++, I think the
> only change that would be required for D is exchanging "_Z" for "_D"
> and calling cplus_demangle with DMGL_DLANG.
Your call whether to fuzz before or after committing the changes.

jeff

Re: [PATCH 2/7] [D] libiberty: Fail if reached end of symbol string

[Iain Buclaw]

On 14 May 2015 at 16:51, Jeff Law <law@redhat.com> wrote:
> On 05/14/2015 07:36 AM, Iain Buclaw wrote:
>>
>> On 14 May 2015 at 14:58, Jeff Law <law@redhat.com> wrote:
>>>
>>> On 05/13/2015 02:51 AM, Iain Buclaw wrote:
>>>>
>>>>
>>>> If a symbol that has so far been valid abruptly ends then we will want
>>>> to fail the process rather than silently succeed.
>>>>
>>>> ---
>>>> libiberty/ChangeLog
>>>>
>>>> 2015-05-13 Iain Buclaw  <ibuclaw@gdcproject.org>
>>>>
>>>>       * d-demangle.c (dlang_call_convention): Return NULL if have
>>>> reached
>>>> the
>>>>       end of the symbol, but expected more.
>>>>       (dlang_attributes): Likewise.
>>>>       (dlang_function_type): Likewise.
>>>>       (dlang_type): Likewise.
>>>>       (dlang_identifier): Likewise.
>>>>       (dlang_value): Likewise.
>>>
>>>
>>> I spot checked various callers of these functions that not return NULL
>>> and
>>> they looked reasonable. Though I was a bit concerned about the callers of
>>> dlang_type, dlang_value and dlang_identifier.
>>>
>>> In those cases we'll often still do the string_append, string_setlength
>>> and
>>> other calls in the caller of dlang_{value,type,identifier}.  I'm assuming
>>> that's safe (the error still appears to be bubbling up properly).
>>>
>>
>> The string routines should be safe for that (appending a string with a
>> zero length does nothing, for instance).
>>
>>> If you can confirm that we're OK in those cases, then this is OK for the
>>> trunk.
>>>
>>
>> I can start fuzzing mangle strings to test that failures actually fail
>> gracefully.  There's already a fuzzer that exists for C++, I think the
>> only change that would be required for D is exchanging "_Z" for "_D"
>> and calling cplus_demangle with DMGL_DLANG.
>
> Your call whether to fuzz before or after committing the changes.
>
> jeff

Iant commited in the changes first time around.  I don't have write
after approval access in GCC just yet, probably should go about
getting that sorted out if this is to become a regular thing.  I dealt
with copyright assignments back in September.

Iain.

Re: [PATCH 2/7] [D] libiberty: Fail if reached end of symbol string

[Jeff Law]

On 05/14/2015 01:15 PM, Iain Buclaw wrote:

> Iant commited in the changes first time around.  I don't have write
> after approval access in GCC just yet, probably should go about
> getting that sorted out if this is to become a regular thing.  I dealt
> with copyright assignments back in September.
Yea, let's get write access sorted out.  I don't mind occasionally 
committing changes for random contributors, but you're around here 
enough that you should just have access.

See authenticated access on this page:

https://gcc.gnu.org/svnwrite.html

List me as your sponsor :-)

jeff