Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
shiryaevfed opened this issue Jul 5, 2019 · 10 comments

Comments

@shiryaevfed
Copy link

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
Copy link
Member

garlick commented Jul 5, 2019

Thanks! I can reproduce this with the current master.

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

@shiryaevfed
Copy link
Author

scrub version 2.6.1

@garlick
Copy link
Member

garlick commented Jul 5, 2019

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
Copy link
Author

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
Copy link
Member

garlick commented Jul 8, 2019

Hmm, good thoughts. Thank you!

@shiryaevfed
Copy link
Author

shiryaevfed commented Jul 9, 2019

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
Copy link
Member

garlick commented Jul 10, 2019

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
Copy link
Author

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

@pete4abw
Copy link
Contributor

pete4abw commented Jul 30, 2023

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
Copy link
Member

garlick commented Jul 31, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants