| Message ID | 20240512133857.1634-1-peter0x44@disroot.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] driver: Output to a temp file; rename upon success [PR80182] | expand |
On Sun, May 12, 2024 at 3:40 PM Peter Damianov <peter0x44@disroot.org> wrote: > > Currently, commands like: > gcc -o file.c -lm > will delete the user's code. > > This patch makes the linker write executables to a temp file, and then renames > the temp file if successful. This fixes the case above, but has limitations. > The source file will still get overwritten if the link "succeeds", such as the > case of: gcc -o file.c -lm -r > > It's not perfect, but it should hopefully stop some people from ruining their > day. Hmm. When suggesting this I was originally hoping for this to be implemented in the linker so that it delays opening (and truncating) of the output file as much as possible. If we want to do something in the compiler driver then I like the filename based heuristics more. v3 seems to only address the case of -o specifying the linker output file but of course gcc -c t.c -o t2.c or gcc -S t.c -o t2.c happily overwrite a source file as well. For these cases heuristically rejecting source file patterns would be better. As we've shown the rename trick when the link was successful doesn't fully solve the issue. And I bet some people will claim it isn't an issue at all ... That is, I do think the linker itself, as a quality of implementation issue, should avoid truncating the output early. In fact the BFD linker seems to unlink the output very early: 24937 stat("t.c", {st_mode=S_IFREG|0644, st_size=13, ...}) = 0 24937 lstat("t.c", {st_mode=S_IFREG|0644, st_size=13, ...}) = 0 24937 unlink("t.c") = 0 24937 openat(AT_FDCWD, "t.c", O_RDWR|O_CREAT|O_TRUNC, 0666) = 3 before even opening other inputs or the default linker script. Richard. > gcc/ChangeLog: > PR driver/80182 > * gcc.cc (output_file_temp): New global variable > (driver_handle_option): Create temp file for executable output > (driver::maybe_run_linker): Rename output_file_temp to output_file if > the linker ran successfully > > Signed-off-by: Peter Damianov <peter0x44@disroot.org> > --- > > v3: don't attempt to create temp files -> rename for -o /dev/null > > gcc/gcc.cc | 53 +++++++++++++++++++++++++++++++++++++---------------- > 1 file changed, 37 insertions(+), 16 deletions(-) > > diff --git a/gcc/gcc.cc b/gcc/gcc.cc > index 830a4700a87..5e38c6e578a 100644 > --- a/gcc/gcc.cc > +++ b/gcc/gcc.cc > @@ -2138,6 +2138,11 @@ static int have_E = 0; > /* Pointer to output file name passed in with -o. */ > static const char *output_file = 0; > > +/* We write the output file to a temp file, and rename it if linking > + is successful. This is to prevent mistakes like: gcc -o file.c -lm from > + deleting the user's code. */ > +static const char *output_file_temp = 0; > + > /* Pointer to input file name passed in with -truncate. > This file should be truncated after linking. */ > static const char *totruncate_file = 0; > @@ -4610,10 +4615,18 @@ driver_handle_option (struct gcc_options *opts, > #if defined(HAVE_TARGET_EXECUTABLE_SUFFIX) || defined(HAVE_TARGET_OBJECT_SUFFIX) > arg = convert_filename (arg, ! have_c, 0); > #endif > - output_file = arg; > + output_file_temp = output_file = arg; > + /* If creating an executable, create a temp file for the output, unless > + -o /dev/null was requested. This will later get renamed, if the linker > + succeeds. */ > + if (!have_c && strcmp (output_file, HOST_BIT_BUCKET) != 0) > + { > + output_file_temp = make_temp_file (""); > + record_temp_file (output_file_temp, false, true); > + } > /* On some systems, ld cannot handle "-o" without a space. So > split the option from its argument. */ > - save_switch ("-o", 1, &arg, validated, true); > + save_switch ("-o", 1, &output_file_temp, validated, true); > return true; > > case OPT_pie: > @@ -9266,22 +9279,30 @@ driver::maybe_run_linker (const char *argv0) const > linker_was_run = (tmp != execution_count); > } > > - /* If options said don't run linker, > - complain about input files to be given to the linker. */ > - > - if (! linker_was_run && !seen_error ()) > - for (i = 0; (int) i < n_infiles; i++) > - if (explicit_link_files[i] > - && !(infiles[i].language && infiles[i].language[0] == '*')) > + if (!seen_error ()) > + { > + if (linker_was_run) > + /* If the linker finished without errors, rename the output from the > + temporary file to the real output name. */ > + rename (output_file_temp, output_file); > + else > { > - warning (0, "%s: linker input file unused because linking not done", > - outfiles[i]); > - if (access (outfiles[i], F_OK) < 0) > - /* This is can be an indication the user specifed an errorneous > - separated option value, (or used the wrong prefix for an > - option). */ > - error ("%s: linker input file not found: %m", outfiles[i]); > + /* If options said don't run linker, > + complain about input files to be given to the linker. */ > + for (i = 0; (int) i < n_infiles; i++) > + if (explicit_link_files[i] > + && !(infiles[i].language && infiles[i].language[0] == '*')) > + { > + warning (0, "%s: linker input file unused because linking not done", > + outfiles[i]); > + if (access (outfiles[i], F_OK) < 0) > + /* This is can be an indication the user specifed an errorneous > + separated option value, (or used the wrong prefix for an > + option). */ > + error ("%s: linker input file not found: %m", outfiles[i]); > + } > } > + } > } > > /* The end of "main". */ > -- > 2.39.2 >
On 2024-05-16 01:29, Richard Biener wrote: > On Sun, May 12, 2024 at 3:40 PM Peter Damianov <peter0x44@disroot.org> > wrote: >> >> Currently, commands like: >> gcc -o file.c -lm >> will delete the user's code. >> >> This patch makes the linker write executables to a temp file, and then >> renames >> the temp file if successful. This fixes the case above, but has >> limitations. >> The source file will still get overwritten if the link "succeeds", >> such as the >> case of: gcc -o file.c -lm -r >> >> It's not perfect, but it should hopefully stop some people from >> ruining their >> day. > > Hmm. When suggesting this I was originally hoping for this to be > implemented > in the linker so that it delays opening (and truncating) of the output > file as much as possible. Ah, okay, I assumed you wanted it in the driver since then it could still fix the problem for older linker versions, but it could be a problem to sort the linker too. > > If we want to do something in the compiler driver then I like the > filename based > heuristics more. v3 seems to only address the case of -o specifying > the linker > output file but of course The filename heuristics feel like too much hacks for my liking, but maybe I don't have a rational reason to feel that way. I had some trouble figuring exactly which suffixes to reject, obviously -S should not reject .s as an output file, but I still don't think I got it all correct. I'm also a little worried, perhaps there is some weird makefiles or configure scripts out there that do depend on this behavior. > > gcc -c t.c -o t2.c > > or > > gcc -S t.c -o t2.c > > happily overwrite a source file as well. For these cases > heuristically rejecting > source file patterns would be better. As we've shown the rename trick > when > the link was successful doesn't fully solve the issue. And I bet some > people > will claim it isn't an issue at all ... I don't think there is any easy or nice way to "fully solve the issue", especially if you want to consider -c, -E, -S, etc. One other idea for -c could be refusing to write out the object file if there is no elf/coff/pe/macho header, but I don't like it, sounds too complex. > > That is, I do think the linker itself, as a quality of implementation > issue, > should avoid truncating the output early. In fact the BFD linker seems > to > unlink the output very early: Agreed. I decided to try some other linkers, lld and mold both don't have this issue. BFD and gold do. I suppose I should open a bug for that, or investigate myself. > > 24937 stat("t.c", {st_mode=S_IFREG|0644, st_size=13, ...}) = 0 > 24937 lstat("t.c", {st_mode=S_IFREG|0644, st_size=13, ...}) = 0 > 24937 unlink("t.c") = 0 > 24937 openat(AT_FDCWD, "t.c", O_RDWR|O_CREAT|O_TRUNC, 0666) = 3 > > before even opening other inputs or the default linker script. > > Richard. > >> gcc/ChangeLog: >> PR driver/80182 >> * gcc.cc (output_file_temp): New global variable >> (driver_handle_option): Create temp file for executable output >> (driver::maybe_run_linker): Rename output_file_temp to >> output_file if >> the linker ran successfully >> >> Signed-off-by: Peter Damianov <peter0x44@disroot.org> >> --- >> >> v3: don't attempt to create temp files -> rename for -o /dev/null >> >> gcc/gcc.cc | 53 +++++++++++++++++++++++++++++++++++++---------------- >> 1 file changed, 37 insertions(+), 16 deletions(-) >> >> diff --git a/gcc/gcc.cc b/gcc/gcc.cc >> index 830a4700a87..5e38c6e578a 100644 >> --- a/gcc/gcc.cc >> +++ b/gcc/gcc.cc >> @@ -2138,6 +2138,11 @@ static int have_E = 0; >> /* Pointer to output file name passed in with -o. */ >> static const char *output_file = 0; >> >> +/* We write the output file to a temp file, and rename it if linking >> + is successful. This is to prevent mistakes like: gcc -o file.c -lm >> from >> + deleting the user's code. */ >> +static const char *output_file_temp = 0; >> + >> /* Pointer to input file name passed in with -truncate. >> This file should be truncated after linking. */ >> static const char *totruncate_file = 0; >> @@ -4610,10 +4615,18 @@ driver_handle_option (struct gcc_options >> *opts, >> #if defined(HAVE_TARGET_EXECUTABLE_SUFFIX) || >> defined(HAVE_TARGET_OBJECT_SUFFIX) >> arg = convert_filename (arg, ! have_c, 0); >> #endif >> - output_file = arg; >> + output_file_temp = output_file = arg; >> + /* If creating an executable, create a temp file for the >> output, unless >> + -o /dev/null was requested. This will later get renamed, if >> the linker >> + succeeds. */ >> + if (!have_c && strcmp (output_file, HOST_BIT_BUCKET) != 0) >> + { >> + output_file_temp = make_temp_file (""); >> + record_temp_file (output_file_temp, false, true); >> + } >> /* On some systems, ld cannot handle "-o" without a space. So >> split the option from its argument. */ >> - save_switch ("-o", 1, &arg, validated, true); >> + save_switch ("-o", 1, &output_file_temp, validated, true); >> return true; >> >> case OPT_pie: >> @@ -9266,22 +9279,30 @@ driver::maybe_run_linker (const char *argv0) >> const >> linker_was_run = (tmp != execution_count); >> } >> >> - /* If options said don't run linker, >> - complain about input files to be given to the linker. */ >> - >> - if (! linker_was_run && !seen_error ()) >> - for (i = 0; (int) i < n_infiles; i++) >> - if (explicit_link_files[i] >> - && !(infiles[i].language && infiles[i].language[0] == '*')) >> + if (!seen_error ()) >> + { >> + if (linker_was_run) >> + /* If the linker finished without errors, rename the output >> from the >> + temporary file to the real output name. */ >> + rename (output_file_temp, output_file); >> + else >> { >> - warning (0, "%s: linker input file unused because linking >> not done", >> - outfiles[i]); >> - if (access (outfiles[i], F_OK) < 0) >> - /* This is can be an indication the user specifed an >> errorneous >> - separated option value, (or used the wrong prefix for >> an >> - option). */ >> - error ("%s: linker input file not found: %m", >> outfiles[i]); >> + /* If options said don't run linker, >> + complain about input files to be given to the linker. */ >> + for (i = 0; (int) i < n_infiles; i++) >> + if (explicit_link_files[i] >> + && !(infiles[i].language && infiles[i].language[0] == >> '*')) >> + { >> + warning (0, "%s: linker input file unused because >> linking not done", >> + outfiles[i]); >> + if (access (outfiles[i], F_OK) < 0) >> + /* This is can be an indication the user specifed an >> errorneous >> + separated option value, (or used the wrong prefix >> for an >> + option). */ >> + error ("%s: linker input file not found: %m", >> outfiles[i]); >> + } >> } >> + } >> } >> >> /* The end of "main". */ >> -- >> 2.39.2 >>
diff --git a/gcc/gcc.cc b/gcc/gcc.cc index 830a4700a87..5e38c6e578a 100644 --- a/gcc/gcc.cc +++ b/gcc/gcc.cc @@ -2138,6 +2138,11 @@ static int have_E = 0; /* Pointer to output file name passed in with -o. */ static const char *output_file = 0; +/* We write the output file to a temp file, and rename it if linking + is successful. This is to prevent mistakes like: gcc -o file.c -lm from + deleting the user's code. */ +static const char *output_file_temp = 0; + /* Pointer to input file name passed in with -truncate. This file should be truncated after linking. */ static const char *totruncate_file = 0; @@ -4610,10 +4615,18 @@ driver_handle_option (struct gcc_options *opts, #if defined(HAVE_TARGET_EXECUTABLE_SUFFIX) || defined(HAVE_TARGET_OBJECT_SUFFIX) arg = convert_filename (arg, ! have_c, 0); #endif - output_file = arg; + output_file_temp = output_file = arg; + /* If creating an executable, create a temp file for the output, unless + -o /dev/null was requested. This will later get renamed, if the linker + succeeds. */ + if (!have_c && strcmp (output_file, HOST_BIT_BUCKET) != 0) + { + output_file_temp = make_temp_file (""); + record_temp_file (output_file_temp, false, true); + } /* On some systems, ld cannot handle "-o" without a space. So split the option from its argument. */ - save_switch ("-o", 1, &arg, validated, true); + save_switch ("-o", 1, &output_file_temp, validated, true); return true; case OPT_pie: @@ -9266,22 +9279,30 @@ driver::maybe_run_linker (const char *argv0) const linker_was_run = (tmp != execution_count); } - /* If options said don't run linker, - complain about input files to be given to the linker. */ - - if (! linker_was_run && !seen_error ()) - for (i = 0; (int) i < n_infiles; i++) - if (explicit_link_files[i] - && !(infiles[i].language && infiles[i].language[0] == '*')) + if (!seen_error ()) + { + if (linker_was_run) + /* If the linker finished without errors, rename the output from the + temporary file to the real output name. */ + rename (output_file_temp, output_file); + else { - warning (0, "%s: linker input file unused because linking not done", - outfiles[i]); - if (access (outfiles[i], F_OK) < 0) - /* This is can be an indication the user specifed an errorneous - separated option value, (or used the wrong prefix for an - option). */ - error ("%s: linker input file not found: %m", outfiles[i]); + /* If options said don't run linker, + complain about input files to be given to the linker. */ + for (i = 0; (int) i < n_infiles; i++) + if (explicit_link_files[i] + && !(infiles[i].language && infiles[i].language[0] == '*')) + { + warning (0, "%s: linker input file unused because linking not done", + outfiles[i]); + if (access (outfiles[i], F_OK) < 0) + /* This is can be an indication the user specifed an errorneous + separated option value, (or used the wrong prefix for an + option). */ + error ("%s: linker input file not found: %m", outfiles[i]); + } } + } } /* The end of "main". */
Currently, commands like: gcc -o file.c -lm will delete the user's code. This patch makes the linker write executables to a temp file, and then renames the temp file if successful. This fixes the case above, but has limitations. The source file will still get overwritten if the link "succeeds", such as the case of: gcc -o file.c -lm -r It's not perfect, but it should hopefully stop some people from ruining their day. gcc/ChangeLog: PR driver/80182 * gcc.cc (output_file_temp): New global variable (driver_handle_option): Create temp file for executable output (driver::maybe_run_linker): Rename output_file_temp to output_file if the linker ran successfully Signed-off-by: Peter Damianov <peter0x44@disroot.org> --- v3: don't attempt to create temp files -> rename for -o /dev/null gcc/gcc.cc | 53 +++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 16 deletions(-)