Do not copy NULL string with memcpy.

Message ID 8f595966-8193-5023-1d82-5ad0f1ffdaf0@suse.cz
State New
Headers show
Series
  • Do not copy NULL string with memcpy.
Related show

Commit Message

Martin Liška June 2, 2020, 9:04 a.m.
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(-)

-- 
2.26.2

Comments

Alexandre Oliva June 3, 2020, 3:58 a.m. | #1
Hello, Martin,

On Jun  2, 2020, Martin Liška <mliska@suse.cz> 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 fighter    he/him    https://FSFLA.org/blogs/lxo/
Free Software Evangelist              Stallman was right, but he's left :(
GNU Toolchain Engineer           Live long and free, and prosper ethically
Martin Liška June 3, 2020, 5:49 a.m. | #2
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
Alexandre Oliva June 4, 2020, 8:22 p.m. | #3
On Jun  3, 2020, Martin Liška <mliska@suse.cz> 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 <oliva@adacore.com>


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 fighter    he/him    https://FSFLA.org/blogs/lxo/
Free Software Evangelist              Stallman was right, but he's left :(
GNU Toolchain Engineer           Live long and free, and prosper ethically
H.J. Lu via Gcc-patches June 5, 2020, 6:02 a.m. | #4
On June 4, 2020 10:22:55 PM GMT+02:00, Alexandre Oliva <oliva@adacore.com> wrote:
>On Jun  3, 2020, Martin Liška <mliska@suse.cz> 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 <oliva@adacore.com>

>

>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)

Patch

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