chaos / scrub

disk overwrite utility
GNU General Public License v2.0
70 stars 21 forks source link

scrub crashes when called with "-D somename" flag on symlink to regular file #18

Open shiryaevfed opened 5 years ago

shiryaevfed commented 5 years ago

When I call "scrub -D somename" on symlink, scrub crashes after wiping a file; symlink not renamed. If "-r" flag get added, scrub crashes too, and neither destination file nor symlink removed.

How to reproduce: $ echo "test" > a $ ln -s a b $ scrub -r -D scrubbed b ... scrub: scrub.c:625: scrub_dirent: Assertion `ftype == FILE_REGULAR' failed. Abort (core dumped) $

garlick commented 5 years ago

Thanks! I can reproduce this with the current master.

Not that it probably matters, but which version of scrub is this? (scrub -v)

shiryaevfed commented 5 years ago

scrub version 2.6.1

garlick commented 5 years ago

Just refreshing my memory on how this should work - see if you agree:

Normal behavior should be to scrub the link target (the file). If -D is added, the target should be renamed. If -r is added, the target should be removed. The symlink is left as-is.

If the symlink itself is to be scrubbed, then -L should be specified. In that case, the link target is ignored. If no other options, it's an error. If -D, the symlink is renamed. If -r the symlink is removed.

Does that make sense?

shiryaevfed commented 5 years ago

It's really not an easy question, but I think it's best choice - maybe this behaviour is not the only possible, but it keeps it's logic when operating on symlink chain. Also, I suppose, if user decided to scrub a file, she expects all information in it is totally destroyed forever (including file name, if -D is used), and should be explicitly warned if any data would survive (and it will for symlink, even if -D specified and -L is not - the data will be destroyed, but the symlink name and original filename will be seen in symlink itself, or in the first and last symlink in case of chained symlinks). In this case, if scrub operates with symlink, user should get clear informaion that file is a symlink, and what will be done for current set of flags - what information will be destroyed, and what can retain. As an idea, if symlink is specified, scrub can describe planned actions and ask for confirmation (regardless of -L flag - unless -f is specified; or -i flag can be added to make scrub ask for confirmation instead of adding another meaning for -f). Maybe -q flag is a good idea to suppress such a warning (and scrub's progress details lines) - so user explicitly confirms what she totally understands the idea.

garlick commented 5 years ago

Hmm, good thoughts. Thank you!

shiryaevfed commented 5 years ago

Btw, maybe it'll be good to permit to scrub the name of any file, not just symlink. So, -D -L scrubs name in either case, scrubs content of target if it is a regular file, and then removes target if -r is added. This'll make sense to 'scrub -L -D sometmpname -r /path/to/named_pipe' and even 'scrub -L -D sometmpname -r /path/to/', (permitting to delete empty directory only, to avoid decisions what to do if directory is not empty).

garlick commented 5 years ago

That seems reasonable.

Note this issue may sit here for a while, as I'm currently working on another project that is a higher priority for me.

shiryaevfed commented 5 years ago

Ofcoz! Free software is free, and it's more than enough. Noone can expect 24x7 full support.

pete4abw commented 1 year ago

I'm sorry, I should have tested this first. I am not seeing this bug anymore. What happens is that the link is removed, but not the link target. It seems the solution is to do two directory renames, and two removals.

$ $echo "12345">a
$ ln -s a b
$ scrub -r -D newdir b
scrub: using NNSA NAP-14.1-C patterns
scrub: padding b with 4090 bytes to fill last fs block
scrub: scrubbing b 4096 bytes
scrub: random  |................................................|
scrub: random  |................................................|
scrub: 0x00    |................................................|
scrub: verify  |................................................|
scrub: scrubbing directory entry
scrub: 0x32    |................................................|
scrub: 0x4d    |................................................|
scrub: 0x32    |................................................|
scrub: 0x4d    |................................................|
scrub: 0x32    |................................................|
scrub: 0x4d    |................................................|
scrub: unlinking newdir
$ ls -l a b newdir
ls: cannot access 'b': No such file or directory
ls: cannot access 'newdir': No such file or directory
-rw-r--r-- 1 peter peter 4096 Jul 30 16:13 a

Now, the original symlink is removed, but the target remains not renamed. Now perhaps, after the link is renamed and removed, a second renaming and removal of the link target can be achieved. But I'll leave that for now.

=-=-=-= Original follows

This has been languishing for a while. I think a better way is to NOT permit certain actions on a symlink. For example, a link is not a regular file, so the assertion in the -D code should fail. But in my view, this error should be headed off before it happens. So for example, if the object to be scrubbed is a link and -D is selected the program should abort with a friendly message and suggest using the target instead. I'll take a run at this and let you know.

garlick commented 1 year ago

It appears that 499a491c21b5a18be79334282dfa11fd4f408c4 fixed the particular assertion reported in this issue (tested by reverting and retrying the reproducer). So that answers one question I had.