Re: [PATCH] Do not copy NULL string with memcpy.
On June 4, 2020 10:22:55 PM GMT+02:00, Alexandre Oliva wrote: >On Jun 3, 2020, Martin Liška wrote: > >> On 6/3/20 5:58 AM, Alexandre Oliva wrote: >>> Please let me know if you'd prefer me to take this PR over. > >> Yes, please take a look. > >Here's what I've regstrapped on x86_64-linux-gnu. It makes both memcpy >calls conditional, and reorders the length computation to match. > >I also ran outputs.exp with a memcpy wrapper in gcc.c to detect any >calls with NULL pointers, and nothing else came up. > >Ok to install? OK. Thanks, Richard. > >[PR95456] avoid memcpy (_, NULL, 0) in gcc.c > >From: Alexandre Oliva > >Some newly-added code in gcc.c might call memcpy with a NULL source >pointer and zero-length inputs. Avoid such calls by rearranging the >code a little. > > >for gcc/ChangeLog > > PR driver/95456 > * gcc.c (do_spec_1): Don't call memcpy (_, NULL, 0). >--- > gcc/gcc.c | 14 +++--- > 1 file changed, 7 insertions(+), 7 deletions(-) > >diff --git a/gcc/gcc.c b/gcc/gcc.c >index e2362175f4..c0eb3c1 100644 >--- a/gcc/gcc.c >+++ b/gcc/gcc.c >@@ -6024,19 +6024,19 @@ do_spec_1 (const char *spec, int inswitch, >const char *soft_matched_part) > } > temp_filename_length > = dumpdir_length + suffix_length + 1; >- if (!outbase_length) >-temp_filename_length += basename_length; >- else >+ if (outbase_length) > temp_filename_length += outbase_length; >+ else >+temp_filename_length += basename_length; > tmp = (char *) alloca (temp_filename_length); > if (dumpdir_length) > memcpy (tmp, dumpdir, dumpdir_length); >- if (!outbase_length) >-memcpy (tmp + dumpdir_length, input_basename, >-basename_length); >- else >+ if (outbase_length) > memcpy (tmp + dumpdir_length, outbase, > outbase_length); >+ else if (basename_length) >+memcpy (tmp + dumpdir_length, input_basename, >+basename_length); > memcpy (tmp + temp_filename_length - suffix_length - 1, > suffix, suffix_length); > if (adjusted_suffix)
Re: [PATCH] Do not copy NULL string with memcpy.
On Jun 3, 2020, Martin Liška wrote: > On 6/3/20 5:58 AM, Alexandre Oliva wrote: >> Please let me know if you'd prefer me to take this PR over. > Yes, please take a look. Here's what I've regstrapped on x86_64-linux-gnu. It makes both memcpy calls conditional, and reorders the length computation to match. I also ran outputs.exp with a memcpy wrapper in gcc.c to detect any calls with NULL pointers, and nothing else came up. Ok to install? [PR95456] avoid memcpy (_, NULL, 0) in gcc.c From: Alexandre Oliva Some newly-added code in gcc.c might call memcpy with a NULL source pointer and zero-length inputs. Avoid such calls by rearranging the code a little. for gcc/ChangeLog PR driver/95456 * gcc.c (do_spec_1): Don't call memcpy (_, NULL, 0). --- gcc/gcc.c | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/gcc/gcc.c b/gcc/gcc.c index e2362175f4..c0eb3c1 100644 --- a/gcc/gcc.c +++ b/gcc/gcc.c @@ -6024,19 +6024,19 @@ do_spec_1 (const char *spec, int inswitch, const char *soft_matched_part) } temp_filename_length = dumpdir_length + suffix_length + 1; - if (!outbase_length) - temp_filename_length += basename_length; - else + if (outbase_length) temp_filename_length += outbase_length; + else + temp_filename_length += basename_length; tmp = (char *) alloca (temp_filename_length); if (dumpdir_length) memcpy (tmp, dumpdir, dumpdir_length); - if (!outbase_length) - memcpy (tmp + dumpdir_length, input_basename, - basename_length); - else + if (outbase_length) memcpy (tmp + dumpdir_length, outbase, outbase_length); + else if (basename_length) + memcpy (tmp + dumpdir_length, input_basename, + basename_length); memcpy (tmp + temp_filename_length - suffix_length - 1, suffix, suffix_length); if (adjusted_suffix) -- Alexandre Oliva, freedom fighterhe/himhttps://FSFLA.org/blogs/lxo/ Free Software Evangelist Stallman was right, but he's left :( GNU Toolchain Engineer Live long and free, and prosper ethically
Re: [PATCH] Do not copy NULL string with memcpy.
On 6/3/20 5:58 AM, Alexandre Oliva wrote: Please let me know if you'd prefer me to take this PR over. Yes, please take a look. Martin
Re: [PATCH] Do not copy NULL string with memcpy.
Hello, Martin,
On Jun 2, 2020, Martin Liška wrote:
> The problem happens when we generate temp file
> for .res file. Tested locally with the problematic
> options.
Thanks for looking into this.
> Ready for master?
Erhm, no, I don't think that's correct.
With local analysis, the length computation just before has only
allocated space for basename_length. If you copy outbase_length, you
might be writing past the end of the allocated area.
I guess basename_length is 0, otherwise you'd see a crash without the
assert you added in the PR.
With some global analysis (and running the testcase), it appears to me
that when input_basename is NULL (e.g., when processing linker specs),
so is outbase, so your proposed change appears to be trading one 0-byte
memcpy from NULL for another.
I'd rather make it:
if (!outbase_length)
{
if (basename_length)
memcpy (tmp + dumpdir_length, input_basename, basename_length);
}
else
memcpy (tmp + dumpdir_length, outbase, outbase_length);
or maybe:
if (outbase_length)
memcpy (tmp + dumpdir_length, outbase, outbase_length);
else if (basename_length)
memcpy (tmp + dumpdir_length, input_basename, basename_length);
Please let me know if you'd prefer me to take this PR over.
--
Alexandre Oliva, freedom fighterhe/himhttps://FSFLA.org/blogs/lxo/
Free Software Evangelist Stallman was right, but he's left :(
GNU Toolchain Engineer Live long and free, and prosper ethically
[PATCH] Do not copy NULL string with memcpy.
The problem happens when we generate temp file for .res file. Tested locally with the problematic options. Ready for master? Thanks, Martin gcc/ChangeLog: PR driver/95456 * gcc.c (do_spec_1): Append to tempfile only when input_basename != NULL. --- gcc/gcc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gcc/gcc.c b/gcc/gcc.c index e2362175f40..6dea22c0669 100644 --- a/gcc/gcc.c +++ b/gcc/gcc.c @@ -6031,7 +6031,7 @@ do_spec_1 (const char *spec, int inswitch, const char *soft_matched_part) tmp = (char *) alloca (temp_filename_length); if (dumpdir_length) memcpy (tmp, dumpdir, dumpdir_length); - if (!outbase_length) + if (!outbase_length && input_basename != NULL) memcpy (tmp + dumpdir_length, input_basename, basename_length); else -- 2.26.2
