Re: [PATCH] Fix invalid memory access in gcc.c (driver/72765)

[Martin Liška]

On 08/15/2016 01:02 PM, Jakub Jelinek wrote:
> On Fri, Aug 12, 2016 at 02:22:56PM +0200, Martin Liška wrote:
>> Simple patch corrects assumption about string length, however the hunk in
>> save_string is kind of discussable as one can have a string with '\0' chars
>> which is length enough? 
>>
>> Thoughts?
>>
>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>>
>> Ready to be installed?
>> Martin
> 
>> >From c7a7e1be3c113ee0f610d627426b8f241357b86e Mon Sep 17 00:00:00 2001
>> From: marxin 
>> Date: Tue, 9 Aug 2016 13:04:57 +0200
>> Subject: [PATCH] Fix invalid memory access in gcc.c (driver/72765)
>>
>> gcc/ChangeLog:
>>
>> 2016-08-09  Martin Liska  
>>
>>  PR driver/72765
>>  * gcc.c (do_spec_1): Call save_string with the right size.
>>  (save_string): Do an assert about string we copy.
>> ---
>>  gcc/gcc.c | 6 --
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/gcc/gcc.c b/gcc/gcc.c
>> index 7460f6a..a5c4a19 100644
>> --- a/gcc/gcc.c
>> +++ b/gcc/gcc.c
>> @@ -5420,8 +5420,9 @@ do_spec_1 (const char *spec, int inswitch, const char 
>> *soft_matched_part)
>>  if (files_differ)
>>  #endif
>>{
>> -temp_filename = save_string (temp_filename,
>> - temp_filename_length + 
>> 1);
>> +temp_filename
>> +  = save_string (temp_filename,
>> + temp_filename_length - 1);
>>  obstack_grow (, temp_filename,
>>  temp_filename_length);
>>  arg_going = 1;
> 
> This is ok for trunk/6.2 (if you commit RSN).

Ok, I'll commit the first hunk to both GCC-5 and GCC-6 branches.

> 
>> @@ -8362,6 +8363,7 @@ save_string (const char *s, int len)
>>  {
>>char *result = XNEWVEC (char, len + 1);
>>  
>> +  gcc_assert (strlen (s) >= (unsigned int)len);
>>memcpy (result, s, len);
>>result[len] = 0;
>>return result;
> 
> I'd leave this one out (at least from 6.x); if anything, use just
> gcc_checking_assert (since otherwise it doesn't make much sense to pass len
> if you are going to recompute it anyway) and put a space between the cast and 
> len.

And will commit checking assert for trunk.

Martin

> 
>   Jakub
> 

Re: [PATCH] Fix invalid memory access in gcc.c (driver/72765)

[Jakub Jelinek]

On Fri, Aug 12, 2016 at 02:22:56PM +0200, Martin Liška wrote:
> Simple patch corrects assumption about string length, however the hunk in
> save_string is kind of discussable as one can have a string with '\0' chars
> which is length enough? 
> 
> Thoughts?
> 
> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
> 
> Ready to be installed?
> Martin

> >From c7a7e1be3c113ee0f610d627426b8f241357b86e Mon Sep 17 00:00:00 2001
> From: marxin 
> Date: Tue, 9 Aug 2016 13:04:57 +0200
> Subject: [PATCH] Fix invalid memory access in gcc.c (driver/72765)
> 
> gcc/ChangeLog:
> 
> 2016-08-09  Martin Liska  
> 
>   PR driver/72765
>   * gcc.c (do_spec_1): Call save_string with the right size.
>   (save_string): Do an assert about string we copy.
> ---
>  gcc/gcc.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/gcc/gcc.c b/gcc/gcc.c
> index 7460f6a..a5c4a19 100644
> --- a/gcc/gcc.c
> +++ b/gcc/gcc.c
> @@ -5420,8 +5420,9 @@ do_spec_1 (const char *spec, int inswitch, const char 
> *soft_matched_part)
>   if (files_differ)
>  #endif
> {
> - temp_filename = save_string (temp_filename,
> -  temp_filename_length + 
> 1);
> + temp_filename
> +   = save_string (temp_filename,
> +  temp_filename_length - 1);
>   obstack_grow (, temp_filename,
>   temp_filename_length);
>   arg_going = 1;

This is ok for trunk/6.2 (if you commit RSN).

> @@ -8362,6 +8363,7 @@ save_string (const char *s, int len)
>  {
>char *result = XNEWVEC (char, len + 1);
>  
> +  gcc_assert (strlen (s) >= (unsigned int)len);
>memcpy (result, s, len);
>result[len] = 0;
>return result;

I'd leave this one out (at least from 6.x); if anything, use just
gcc_checking_assert (since otherwise it doesn't make much sense to pass len
if you are going to recompute it anyway) and put a space between the cast and 
len.

Jakub