Driver: Reject output filenames with the same suffixes as source files [PR80182]

Currently, commands like:
gcc -o file.c -lm
will delete the user's code.

This patch checks the suffix of the output, and errors if the output ends in
any of the suffixes listed in default_compilers.

Unfortunately, I couldn't come up with a better heuristic to diagnose this case
more specifically, so it is now not possible to directly make executables with
said suffixes. I am unsure if any users are depending on this.

	PR driver/80182
	* gcc.cc (process_command): fatal_error if the output has the suffix of
	  a source file.
	(have_c): Change type to bool.
	(have_O): Change type to bool.
	(have_E): Change type to bool.
	(have_S): New global variable.
	(driver_handle_option): Assign have_S

Signed-off-by: Peter Damianov <peter0x44@disroot.org>
---
 gcc/gcc.cc | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

On Sat, May 4, 2024 at 9:36 PM Peter Damianov <peter0x44@disroot.org> wrote:
>
> Currently, commands like:
> gcc -o file.c -lm
> will delete the user's code.

Since there's an error from the linker in the end (missing 'main'), I wonder if
the linker can avoid truncating/opening the output file instead?  A trivial
solution might be to open a temporary file first and only atomically replace
the output file with the temporary file when there were no errors?

> This patch checks the suffix of the output, and errors if the output ends in
> any of the suffixes listed in default_compilers.
>
> Unfortunately, I couldn't come up with a better heuristic to diagnose this case
> more specifically, so it is now not possible to directly make executables with
> said suffixes. I am unsure if any users are depending on this.

A way to provide a workaround would be to require the file not existing.  So
change the heuristic to only trigger if the output file exists (and is
non-empty?).

Richard.

>         PR driver/80182
>         * gcc.cc (process_command): fatal_error if the output has the suffix of
>           a source file.
>         (have_c): Change type to bool.
>         (have_O): Change type to bool.
>         (have_E): Change type to bool.
>         (have_S): New global variable.
>         (driver_handle_option): Assign have_S
>
> Signed-off-by: Peter Damianov <peter0x44@disroot.org>
> ---
>  gcc/gcc.cc | 29 ++++++++++++++++++++++++++---
>  1 file changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/gcc.cc b/gcc/gcc.cc
> index 830a4700a87..53169c16460 100644
> --- a/gcc/gcc.cc
> +++ b/gcc/gcc.cc
> @@ -2127,13 +2127,16 @@ static vec<const_char_p> at_file_argbuf;
>  static bool in_at_file = false;
>
>  /* Were the options -c, -S or -E passed.  */
> -static int have_c = 0;
> +static bool have_c = false;
>
>  /* Was the option -o passed.  */
> -static int have_o = 0;
> +static bool have_o = false;
>
>  /* Was the option -E passed.  */
> -static int have_E = 0;
> +static bool have_E = false;
> +
> +/* Was the option -S passed.  */
> +static bool have_S = false;
>
>  /* Pointer to output file name passed in with -o. */
>  static const char *output_file = 0;
> @@ -4593,6 +4596,10 @@ driver_handle_option (struct gcc_options *opts,
>        have_E = true;
>        break;
>
> +    case OPT_S:
> +      have_S = true;
> +      break;
> +
>      case OPT_x:
>        spec_lang = arg;
>        if (!strcmp (spec_lang, "none"))
> @@ -5058,6 +5065,22 @@ process_command (unsigned int decoded_options_count,
>                        output_file);
>      }
>
> +  /* Reject output file names that have the same suffix as a source
> +     file. This is to catch mistakes like: gcc -o file.c -lm
> +     that could delete the user's code. */
> +  if (have_o && output_file != NULL && !have_E && !have_S)
> +    {
> +      const char* filename = lbasename(output_file);
> +      const char* suffix = strchr(filename, '.');
> +      if (suffix != NULL)
> +       for (int i = 0; i < n_default_compilers; ++i)
> +         if (!strcmp(suffix, default_compilers[i].suffix))
> +           fatal_error (input_location,
> +                        "output file suffix %qs could be a source file",
> +                        suffix);
> +    }
> +
> +
>    if (output_file != NULL && output_file[0] == '\0')
>      fatal_error (input_location, "output filename may not be empty");
>
> --
> 2.39.2
>

On Mon May 6, 2024 at 8:14 AM BST, Richard Biener wrote:
> On Sat, May 4, 2024 at 9:36 PM Peter Damianov <peter0x44@disroot.org> wrote:
> >
> > Currently, commands like:
> > gcc -o file.c -lm
> > will delete the user's code.
>
> Since there's an error from the linker in the end (missing 'main'), I wonder if
> the linker can avoid truncating/opening the output file instead?  A trivial
> solution might be to open a temporary file first and only atomically replace
> the output file with the temporary file when there were no errors?
I think this is a great idea! The only concern I have is that I think
for mingw targets it would be necessary to be careful to append .exe if
the file has no suffix when moving the temporary file to the output
file. Maybe some other targets have similar concerns.
>
> > This patch checks the suffix of the output, and errors if the output ends in
> > any of the suffixes listed in default_compilers.
> >
> > Unfortunately, I couldn't come up with a better heuristic to diagnose this case
> > more specifically, so it is now not possible to directly make executables with
> > said suffixes. I am unsure if any users are depending on this.
>
> A way to provide a workaround would be to require the file not existing.  So
> change the heuristic to only trigger if the output file exists (and is
> non-empty?).
I guess this could work, and has a lower chance of breaking anyone
depending on this behavior, but I think it would still be confusing to
anyone who did rely on this behavior, since then it wouldn't be allowed
to overwrite an executable with the ".c" name. If anyone did rely on
this behavior, their build would succeed once, and then error for every
subsequent invokation, which would be confusing. It seems to me it is
not a meaningful improvement.

With your previous suggestion, this whole heuristic becomes unnecessary
anyway, so I think I will just forego it.
>
> Richard.
>
> >         PR driver/80182
> >         * gcc.cc (process_command): fatal_error if the output has the suffix of
> >           a source file.
> >         (have_c): Change type to bool.
> >         (have_O): Change type to bool.
> >         (have_E): Change type to bool.
> >         (have_S): New global variable.
> >         (driver_handle_option): Assign have_S
> >
> > Signed-off-by: Peter Damianov <peter0x44@disroot.org>
> > ---
> >  gcc/gcc.cc | 29 ++++++++++++++++++++++++++---
> >  1 file changed, 26 insertions(+), 3 deletions(-)
> >
> > diff --git a/gcc/gcc.cc b/gcc/gcc.cc
> > index 830a4700a87..53169c16460 100644
> > --- a/gcc/gcc.cc
> > +++ b/gcc/gcc.cc
> > @@ -2127,13 +2127,16 @@ static vec<const_char_p> at_file_argbuf;
> >  static bool in_at_file = false;
> >
> >  /* Were the options -c, -S or -E passed.  */
> > -static int have_c = 0;
> > +static bool have_c = false;
> >
> >  /* Was the option -o passed.  */
> > -static int have_o = 0;
> > +static bool have_o = false;
> >
> >  /* Was the option -E passed.  */
> > -static int have_E = 0;
> > +static bool have_E = false;
> > +
> > +/* Was the option -S passed.  */
> > +static bool have_S = false;
> >
> >  /* Pointer to output file name passed in with -o. */
> >  static const char *output_file = 0;
> > @@ -4593,6 +4596,10 @@ driver_handle_option (struct gcc_options *opts,
> >        have_E = true;
> >        break;
> >
> > +    case OPT_S:
> > +      have_S = true;
> > +      break;
> > +
> >      case OPT_x:
> >        spec_lang = arg;
> >        if (!strcmp (spec_lang, "none"))
> > @@ -5058,6 +5065,22 @@ process_command (unsigned int decoded_options_count,
> >                        output_file);
> >      }
> >
> > +  /* Reject output file names that have the same suffix as a source
> > +     file. This is to catch mistakes like: gcc -o file.c -lm
> > +     that could delete the user's code. */
> > +  if (have_o && output_file != NULL && !have_E && !have_S)
> > +    {
> > +      const char* filename = lbasename(output_file);
> > +      const char* suffix = strchr(filename, '.');
> > +      if (suffix != NULL)
> > +       for (int i = 0; i < n_default_compilers; ++i)
> > +         if (!strcmp(suffix, default_compilers[i].suffix))
> > +           fatal_error (input_location,
> > +                        "output file suffix %qs could be a source file",
> > +                        suffix);
> > +    }
> > +
> > +
> >    if (output_file != NULL && output_file[0] == '\0')
> >      fatal_error (input_location, "output filename may not be empty");
> >
> > --
> > 2.39.2
> >

Thanks for the feedback,
Peter D.

On Mon, May 6, 2024 at 10:29 AM Peter0x44 <peter0x44@disroot.org> wrote:
>
> On Mon May 6, 2024 at 8:14 AM BST, Richard Biener wrote:
> > On Sat, May 4, 2024 at 9:36 PM Peter Damianov <peter0x44@disroot.org> wrote:
> > >
> > > Currently, commands like:
> > > gcc -o file.c -lm
> > > will delete the user's code.
> >
> > Since there's an error from the linker in the end (missing 'main'), I wonder if
> > the linker can avoid truncating/opening the output file instead?  A trivial
> > solution might be to open a temporary file first and only atomically replace
> > the output file with the temporary file when there were no errors?
> I think this is a great idea! The only concern I have is that I think
> for mingw targets it would be necessary to be careful to append .exe if
> the file has no suffix when moving the temporary file to the output
> file. Maybe some other targets have similar concerns.
> >
> > > This patch checks the suffix of the output, and errors if the output ends in
> > > any of the suffixes listed in default_compilers.
> > >
> > > Unfortunately, I couldn't come up with a better heuristic to diagnose this case
> > > more specifically, so it is now not possible to directly make executables with
> > > said suffixes. I am unsure if any users are depending on this.
> >
> > A way to provide a workaround would be to require the file not existing.  So
> > change the heuristic to only trigger if the output file exists (and is
> > non-empty?).
> I guess this could work, and has a lower chance of breaking anyone
> depending on this behavior, but I think it would still be confusing to
> anyone who did rely on this behavior, since then it wouldn't be allowed
> to overwrite an executable with the ".c" name. If anyone did rely on
> this behavior, their build would succeed once, and then error for every
> subsequent invokation, which would be confusing. It seems to me it is
> not a meaningful improvement.

That's true and the behavior would be confusing.

> With your previous suggestion, this whole heuristic becomes unnecessary
> anyway, so I think I will just forego it.

It of course wouldn't handle the case if there isn't a link error like

gcc -o file -lm -r

but it should still be an improvement.  And yes, I typoed a wrong -o myself
a few times ...

Richard.

> >
> > Richard.
> >
> > >         PR driver/80182
> > >         * gcc.cc (process_command): fatal_error if the output has the suffix of
> > >           a source file.
> > >         (have_c): Change type to bool.
> > >         (have_O): Change type to bool.
> > >         (have_E): Change type to bool.
> > >         (have_S): New global variable.
> > >         (driver_handle_option): Assign have_S
> > >
> > > Signed-off-by: Peter Damianov <peter0x44@disroot.org>
> > > ---
> > >  gcc/gcc.cc | 29 ++++++++++++++++++++++++++---
> > >  1 file changed, 26 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/gcc/gcc.cc b/gcc/gcc.cc
> > > index 830a4700a87..53169c16460 100644
> > > --- a/gcc/gcc.cc
> > > +++ b/gcc/gcc.cc
> > > @@ -2127,13 +2127,16 @@ static vec<const_char_p> at_file_argbuf;
> > >  static bool in_at_file = false;
> > >
> > >  /* Were the options -c, -S or -E passed.  */
> > > -static int have_c = 0;
> > > +static bool have_c = false;
> > >
> > >  /* Was the option -o passed.  */
> > > -static int have_o = 0;
> > > +static bool have_o = false;
> > >
> > >  /* Was the option -E passed.  */
> > > -static int have_E = 0;
> > > +static bool have_E = false;
> > > +
> > > +/* Was the option -S passed.  */
> > > +static bool have_S = false;
> > >
> > >  /* Pointer to output file name passed in with -o. */
> > >  static const char *output_file = 0;
> > > @@ -4593,6 +4596,10 @@ driver_handle_option (struct gcc_options *opts,
> > >        have_E = true;
> > >        break;
> > >
> > > +    case OPT_S:
> > > +      have_S = true;
> > > +      break;
> > > +
> > >      case OPT_x:
> > >        spec_lang = arg;
> > >        if (!strcmp (spec_lang, "none"))
> > > @@ -5058,6 +5065,22 @@ process_command (unsigned int decoded_options_count,
> > >                        output_file);
> > >      }
> > >
> > > +  /* Reject output file names that have the same suffix as a source
> > > +     file. This is to catch mistakes like: gcc -o file.c -lm
> > > +     that could delete the user's code. */
> > > +  if (have_o && output_file != NULL && !have_E && !have_S)
> > > +    {
> > > +      const char* filename = lbasename(output_file);
> > > +      const char* suffix = strchr(filename, '.');
> > > +      if (suffix != NULL)
> > > +       for (int i = 0; i < n_default_compilers; ++i)
> > > +         if (!strcmp(suffix, default_compilers[i].suffix))
> > > +           fatal_error (input_location,
> > > +                        "output file suffix %qs could be a source file",
> > > +                        suffix);
> > > +    }
> > > +
> > > +
> > >    if (output_file != NULL && output_file[0] == '\0')
> > >      fatal_error (input_location, "output filename may not be empty");
> > >
> > > --
> > > 2.39.2
> > >
>
> Thanks for the feedback,
> Peter D.