| Message ID | CABOHX+diWnYZ1Tq_=NP=kremCYOZgtMqzF2g4wnCNJ0bw-yhJg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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.
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
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
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.