GNU nano - Bugs: bug #58642, potential data loss when...
You are not allowed to post comments on this tracker with your current authentication level.
bug #58642: potential data loss when overwriting existing files
Submitter: | Michalis Kokologiannakis <michalis> | ||
Submitted: | Mon 22 Jun 2020 04:36:41 PM UTC | ||
Severity: | 3 - Normal | Status: | Fixed |
Assigned to: | bens | Open/Closed: | Closed |
Jump to the original submission
Thu 30 Jul 2020 07:17:15 AM UTC, comment #28: |
Benno Schulenberg <bens> |
Sun 26 Jul 2020 02:31:18 PM UTC, comment #27: For completeness, here is a test case showing how data loss can occur under ext4 (assuming the old backup procedure with no fsync()s).
|
Michalis Kokologiannakis <michalis> |
Thu 09 Jul 2020 03:03:23 PM UTC, comment #26: Thank you for your help.
|
Michalis Kokologiannakis <michalis> |
Thu 09 Jul 2020 02:38:52 PM UTC, comment #25: V3 of your patch has been pushed to master, commit a84cdaaa, with a small message tweak. Thank you for all the work!
|
Benno Schulenberg <bens> |
Sat 04 Jul 2020 01:48:38 PM UTC, comment #24: Thank you. Looks good. The only improvement I would suggest is to add a "~" before the "XXXXXX", in both cases. This makes the name more readable, and the "~" kind of indicates that it is backup file. With that improvement, please send the patch to the devel list (with git send-email, or otherwise as an attachment). |
Benno Schulenberg <bens> |
Sat 04 Jul 2020 10:47:56 AM UTC, comment #23: Here's what I have now; this version should not overwrite existing backups when making temporaries.
|
Michalis Kokologiannakis <michalis> |
Fri 03 Jul 2020 05:30:12 PM UTC, comment #22: Thanks for thinking this through. Yes, when the user did not request any backups, nano should not overwrite any existing files -- as you said earlier: the user might want to retain certain tilded backups in their home directory (or anywhere else).
|
Benno Schulenberg <bens> |
Fri 03 Jul 2020 04:06:39 PM UTC, comment #21:
|
Michalis Kokologiannakis <michalis> |
Thu 02 Jul 2020 12:28:16 PM UTC, comment #20: Hello Michalis,
|
Benno Schulenberg <bens> |
Thu 02 Jul 2020 10:02:59 AM UTC, comment #19: Hello, Benno,
|
Michalis Kokologiannakis <michalis> |
Tue 30 Jun 2020 02:03:01 PM UTC, comment #18: Sure. I will take another look at the patch as well. |
Michalis Kokologiannakis <michalis> |
Tue 30 Jun 2020 01:52:45 PM UTC, comment #17: About the parentheses: yes, added locally. Thanks. I'm surprised I missed those. :)
|
Benno Schulenberg <bens> |
Tue 30 Jun 2020 10:52:54 AM UTC, comment #16: Seems good to me! For what it is worth, I do prefer the latter approach regarding write failures to a specified backupdir.
|
Michalis Kokologiannakis <michalis> |
Tue 30 Jun 2020 09:28:35 AM UTC, comment #15: Concerning a write failure to a specified backupdir, I have reconsidered. Such a write might fail if it is on a different device and that device is full, or if it is a remote device and the network is down. In those cases nano should still ensure that no data loss can occur and should try and write the backup to the user's home directory. So I've changed the attached patch accordingly.
|
Benno Schulenberg <bens> |
Mon 29 Jun 2020 06:11:42 PM UTC, comment #14: Attached is an edited version of your patch, to reduce the number of unnecessary changes (things can be optimized and comments can be adjusted later), and the two fixes folded in. I've also added a check to prevent a second attempt when '--backupdir' was used: I think that a write failure to a specified backup dir is a user error that nano does not need to pardon. What do you think? |
Benno Schulenberg <bens> |
Mon 29 Jun 2020 03:05:42 PM UTC, comment #13: The fixup stops nano from crashing vocally, but now the "free(): invalid pointer" appears in both cases: with and without --ignore. I think my Ubuntu does extra checking on free(), possibly detecting a double free?
|
Benno Schulenberg <bens> |
Mon 29 Jun 2020 01:05:21 PM UTC, comment #12: Thanks for spotting this. I had tried that exact test case before sending it out but it did not crash for me (although it does crash if I use --ignore).
|
Michalis Kokologiannakis <michalis> |
Mon 29 Jun 2020 12:41:23 PM UTC, comment #11: Thanks for working on this. The latest patch looks okay, but... when I tried it without using any options, nano crashes when the target directory is unwritable. For example:
Type some more text, then type ^S. Nano crashes.
|
Benno Schulenberg <bens> |
Mon 29 Jun 2020 11:47:12 AM UTC, comment #10: Sorry, I just noticed that I uploaded a version with minor errors. Try this one instead. |
Michalis Kokologiannakis <michalis> |
Mon 29 Jun 2020 11:18:18 AM UTC, comment #9: Ok, so here is a summary of a revised version:
|
Michalis Kokologiannakis <michalis> |
Thu 25 Jun 2020 05:28:50 PM UTC, comment #8: Oh, forgot to say: yes, I noticed most of the code was the same. But I saw that in the Slowpath, when opening the original file fails, you set ret = 2, which results in a retry, but... with an 'original' that is NULL. That will not work. It will be much easier for me to see such cases of changed behavior if you don't move any code around. |
Benno Schulenberg <bens> |
Thu 25 Jun 2020 05:17:12 PM UTC, comment #7: Using the specified backupdir when the user has not specified -B/--backup is a problem, because the backupdir might be on a different device, it might be on a USB stick or something, and the user may want to write to it only under very specific circumstances. So I don't want nano to write to it when not explicitly requested to do so.
|
Benno Schulenberg <bens> |
Thu 25 Jun 2020 08:59:18 AM UTC, comment #6: Thanks for the feedback.
|
Michalis Kokologiannakis <michalis> |
Thu 25 Jun 2020 07:44:04 AM UTC, comment #5: Thanks for the patch. But... too many changes in one patch -- I cannot follow any more what things you are changing. It's better to not change any code that does not need to be changed to get the behavior we want. It doesn't matter if the code becomes a mess: cleanups and refactorings can be done afterward. In the patch I want to see only the necessary changes.
|
Benno Schulenberg <bens> |
Wed 24 Jun 2020 04:51:35 PM UTC, comment #4: Ah, I see, thanks for the example.
|
Michalis Kokologiannakis <michalis> |
Tue 23 Jun 2020 05:44:15 PM UTC, comment #3: There is another (not uncommon) scenario: editing a temp file in a temp directory, owned by root, that has the sticky bit set. Renaming the original file to make a backup will not work there, but making a copy of the file will work. For example:
The above will work (will make a backup of foo in the glued/ directory) with current nano, but will fail to make a backup when using your patched version.
|
Benno Schulenberg <bens> |
Tue 23 Jun 2020 09:57:47 AM UTC, comment #2: Good point, I did not think of that scenario.
|
Michalis Kokologiannakis <michalis> |
Mon 22 Jun 2020 05:56:14 PM UTC, comment #1: Hello Michalis,
|
Benno Schulenberg <bens> |
Mon 22 Jun 2020 04:36:41 PM UTC, original submission:
Hello,
|
Michalis Kokologiannakis <michalis> |
Depends on the following items: None found
Items that depend on this one: None found
There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.
Follow 16 latest changes.
Date | Changed by | Updated Field | Previous Value | => | Replaced by |
---|---|---|---|---|---|
2020-07-30 | bens | Open/Closed | Open | Closed | |
2020-07-26 | michalis | Attached File | - | Added editor-interactive.sh, #49549 | |
2020-07-09 | bens | Status | In Progress | Fixed | |
2020-07-04 | michalis | Attached File | - | Added v2-0001-files-improve-the-backup-procedure-to-ensure-no-d.patch, #49422 | |
2020-07-03 | michalis | Attached File | - | Added v2-0001-files-improve-the-backup-procedure-to-ensure-no-d.patch, #49420 | |
2020-07-02 | michalis | Attached File | - | Added v2-0001-files-improve-the-backup-procedure-to-ensure-no-d.patch, #49407 | |
2020-06-30 | bens | Attached File | - | Added 0001-files-improve-the-backup-procedure-to-ensure-no-data.patch, #49396 | |
Assigned to | None | bens | |||
2020-06-29 | bens | Attached File | - | Added 0001-files-improve-the-backup-procedure-to-ensure-no-data.patch, #49394 | |
2020-06-29 | michalis | Attached File | - | Added 0001-fixup-files-improve-backup-procedure-to-ensure-no-da.patch, #49385 | |
2020-06-29 | bens | Status | None | In Progress | |
Summary | Potential data loss under ext4 when overwriting files | potential data loss when overwriting existing files | |||
2020-06-29 | michalis | Attached File | - | Added 0001-files-improve-backup-procedure-to-ensure-no-data-is-.patch, #49384 | |
2020-06-29 | michalis | Attached File | - | Added 0001-files-improve-backup-procedure-to-ensure-no-data-is-.patch, #49383 | |
2020-06-24 | michalis | Attached File | - | Added 0001-files-improve-backup-procedure-to-ensure-no-data-is-.patch, #49369 | |
2020-06-22 | michalis | Attached File | - | Added 0001-files-ensure-no-data-loss-under-ext4-when-saving.patch, #49347 |
Powered by Savane 3.13-d3ae.
Corresponding source code
(Thanks for the test case.)
The fix for this issue has been released in nano-5.0.