cp: keep handling the rest of the paths if one does not exists #6208
Conversation
|
Interesting! I think you're on the right track by looking into the value parser, but I think the problematic part in the current code is the |
|
GNU testsuite comparison: |
|
@tertsdiepraam : makes more sense to do it the way you describe it, will make the change ASAP. btw, gnu cp accepts both empty strings and non existant files. both produces the same error and other files are handled. |
d000b88 to
a5d02b3
Compare
|
I've updated to use an OsString instead of a custom struct in the parsing and then filter out the non existant paths in the A quick test here with empty filepath and non existant files. |
a5d02b3 to
5d1c674
Compare
|
GNU testsuite comparison: |
|
Just a few points, maybe you're already aware, but just in case:
|
|
@BenWiederhake : thanks for your feedback, I've updated to coreutils 9.5 and I'm seeing the same behaviour. Seems I'm catching some of the test cases in I'll double check and fix the regressions before removing the draft status. |
8a2a1c3 to
87facf6
Compare
|
GNU testsuite comparison: |
f604765 to
5d85b34
Compare
There was a problem hiding this comment.
Please don't try to predict whether an error is about to happen; this approach is always trouble. Instead, let's handle the error if and when it happens. This has the added advantage that we will automatically have the correct "continue"-behavior for other types of errors.
src/uu/cp/src/cp.rs
Outdated
| Ok((paths, target)) | ||
| // If the path exists and is valid keep it in the argument list. | ||
| // Exception for symlinks as they are a bit more tricky to handle | ||
| // and there's logic further down the execution to handle them. |
There was a problem hiding this comment.
there's logic further down
So this code doesn't actually handle the error as it happens, but rather tries to predict whether an error is about to happen? I don't like it; this type of approach is inevitably fragile: What if we check a file now, but later it appears? When copying several gigabytes on a slow disk, this might be a window of minutes! There are several options that change behavior around symlinks: Either this code results in the wrong error message for those, or wrong behavior, or both. If there are any other problems when copying, I get the impression that GNU cp would continue with the next file, and uutils would still abort; i.e. the same issue that this PR is trying to fix, but in a slightly different flavor.
No. Instead, let's handle the error if and when it happens. This should also have the advantage of reducing the TOCTOU-timespan, and guaranteed robustness: There cannot be false-positive or false-negatives when there is no "prediction" involved.
5d85b34 to
60e7ed8
Compare
|
@BenWiederhake : Thanks for the feedback. |
There was a problem hiding this comment.
Thanks for adding a test!
-
However, the basic approach is still wrong: Don't try to predict whether an error is about to happen, because this will always be inaccurate. Instead, try to execute the action, and then handle the error if and when it happens.
-
Your PR is based on a somewhat old version of uutils, and cannot be easily rebased. I suggest fixing this first, because it might solve some of your troubles, and cannot cause any new work that you wouldn't have otherwise.
-
For extra bonus points, you might even add a test to demonstrate that a certain error type (or scenario that prevents
cpfrom trying to copy at all) has "priority". Note that I just discovered three bugs while doing so, so this might be hard or impossible, but should be done eventually. It's up to you to decide whether this is in-scope or out-of-scope for this PR. If you do, please run all your tests also against GNU 9.5.
src/uu/cp/src/cp.rs
Outdated
| @@ -1200,6 +1201,12 @@ pub fn copy(sources: &[PathBuf], target: &Path, options: &Options) -> CopyResult | |||
| if seen_sources.contains(source) { | |||
| // FIXME: compare sources by the actual file they point to, not their path. (e.g. dir/file == dir/../dir/file in most cases) | |||
| show_warning!("source file {} specified more than once", source.quote()); | |||
| } else if !source.exists() && !source.is_symlink() { | |||
There was a problem hiding this comment.
This still checks a particular condition in advance, instead of handling the error if and when it actually happens.
This type of thinking ("oh I'll just detect this particular scenario beforehand!") can lead to weird inconsistencies that are hard or even impossible to fix. For example, there might be a hypothetical flag where copy attempts are avoided if a particular condition is met (i.e. the filename contains a number that is divisible by 257 or whatever), and suddenly this check would break cp in really weird ways.
In fact, pretty much this silly thing is happening here! Your PR is based on a very old version with a particular bug that has already been fixed. With that bug, cp -i -u is a no-op, and will not attempt to copy anything, so a missing source file is no problem. Because it doesn't even try to copy the source file, there should be no stat, and no error message – but your code introduces an error message that claims something very particular that never happened, misleading the user!
I can't give a more realistic example, because it turns out that most of the paths are buggy; I'll create some issues shortly.
I'm not absolutely sure when we stat the source, but I think it's this chunk in fn copyfile:
let source_metadata = {
let result = if options.dereference(source_in_command_line) {
fs::metadata(source)
} else {
fs::symlink_metadata(source)
};
result.context(context)?
};
If that assumption is true (and please check this assumption, I might be totally wrong!), then this is where we should raise the error. Not before, not after.
|
ping ? :) |
Sorry, got sidetracked (had some vacations, broke a bone...), I will resume working on this next week probably :) |
|
Vacations sound good |
Signed-off-by: Guillaume Ranquet <granquet@baylibre.com>
60e7ed8 to
a1c260e
Compare
|
sorry, i think it has been fixed by #6683 |
|
sorry :( |
|
@sylvestre : no worries, I forgot about it at some point... I should have pinged here to get the reviews going. |
|
@granquet we still have some good first bugs ;) |
|
GNU testsuite comparison: |
Actually, it seems that this works :) |
I have restored the branch, but can't seem to be able to re-open the PR. |
Quick attempt at fixing issue #6177
I'm implementing a custom type for the value parser so that I have the option to not fail early if the path does not exists and I let the operations continue "as if" everything is fine.
Not really happy with the solution as the "bad" path is still in the array of paths and is checked a second time... though I don't have a better idea yet.