Page MenuHomeFreeBSD
Differential D50093

cp: Partly restore symlink folllowing.
ClosedPublic
Actions

Authored by des on Apr 30 2025, 7:13 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Aug 21, 12:08 PM
Unknown Object (File)
Thu, Aug 21, 2:30 AM
Unknown Object (File)
Thu, Aug 21, 1:23 AM
Unknown Object (File)
Sun, Aug 17, 10:40 PM
Unknown Object (File)
Tue, Aug 12, 6:11 AM
Unknown Object (File)
Mon, Aug 11, 11:53 PM
Unknown Object (File)
Mon, Aug 11, 12:18 PM
Unknown Object (File)
Mon, Aug 11, 10:30 AM
View All 84 Files
Subscribers
imp
markj

Details

Reviewers
markj
Group Reviewers
Klara
Commits
rG82fc0d09e862: cp: Partly restore symlink folllowing.
Summary
  • As a general rule, follow symbolic links in the destination as long as the target is within the destination hierarchy.
  • As a special case, allow the destination itself to be a symbolic link, and even a dead one (in which case we create the target).
  • The file-to-file case remains unrestricted.

Currently, if a symlink we aren't allowed to follow is encountered,
cp will behave just like it would if the file was not writable. We
should consider whether it would be better to replace the offending
link instead.

Fixes: 0729d1e8fd90
MFC after: never
Relnotes: yes
Sponsored by: Klara, Inc.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 63835
Build 60719: arc lint + arc unit

Event Timeline

des created this revision.Apr 30 2025, 7:13 PM
Herald added a subscriber: imp. · View Herald TranscriptApr 30 2025, 7:13 PM
des requested review of this revision.Apr 30 2025, 7:13 PM
Harbormaster completed remote builds in B63810: Diff 154588.Apr 30 2025, 7:13 PM
markj added a subscriber: markj.May 1 2025, 1:53 PM

The patch doesn't apply to main -- is there a preceding diff?

bin/cp/cp.c
317–320

This line should be wrapped.

des added a comment.May 2 2025, 8:31 AM

Yep I had an older local commit that added test cases and forgot it hadn't been pushed.

des updated this revision to Diff 154683.May 2 2025, 8:34 AM

mising tests

Harbormaster completed remote builds in B63830: Diff 154683.May 2 2025, 8:34 AM
des updated this revision to Diff 154693.May 2 2025, 11:25 AM

nits

Harbormaster completed remote builds in B63833: Diff 154693.May 2 2025, 11:25 AM
des updated this revision to Diff 154695.May 2 2025, 12:47 PM

more tests

Harbormaster completed remote builds in B63834: Diff 154695.May 2 2025, 12:47 PM
des marked an inline comment as done.May 2 2025, 12:48 PM
des updated this revision to Diff 154696.May 2 2025, 1:08 PM

more nits

Harbormaster completed remote builds in B63835: Diff 154696.May 2 2025, 1:08 PM
des updated this revision to Diff 154697.May 2 2025, 2:13 PM

fix acl bug, improve acl / flag tests

Harbormaster completed remote builds in B63836: Diff 154697.May 2 2025, 2:13 PM
markj added inline comments.May 5 2025, 4:25 PM
bin/cp/cp.c
68

*target in main() should be const-qualified as well.

177

It'd be nice to explain why we save it. I guess it's for some acl_set* calls elsewhere, but that's not very obvious.

185
291

What breaks if AT_RESOLVE_BENEATH is specified unconditionally? For a simple file-to-existing-file copy I wouldn't expect any difference.

des marked 4 inline comments as done.May 5 2025, 7:05 PM
des added inline comments.
bin/cp/cp.c
68

That only works because strrchr() does not propagate the qualifier. Except, oops, in C23 it does... so target can't be const (because sep would have to be const and we wouldn't be able to strip the trailing slash) and therefore dot can't be const (because we wouldn't be allowed to assign it to target).

177

It was originally for some acl_set*() calls elsewhere, but I got rid of those, so now it's only used for to.dir in the FILE_TO_FILE case, and also in the FILE_TO_DIR case if we determine that the destination is identical to cwd. I think we can get rid of it now and just use AT_FDCWD in the FILE_TO_FILE case.

291

In the FILE_TO_FILE case, to.dir is identical with cwd. Specifying AT_RESOLVE_BENEATH unconditionally would mean you can only ever copy a file to a destination beneath cwd. Even something as simple as cp foo /tmp would not work.

des marked 3 inline comments as done.May 5 2025, 7:05 PM
des updated this revision to Diff 154893.May 5 2025, 7:07 PM

review feedback

Harbormaster completed remote builds in B63915: Diff 154893.May 5 2025, 7:07 PM
markj accepted this revision as: markj.May 6 2025, 3:11 PM
This revision is now accepted and ready to land.May 6 2025, 3:11 PM
Closed by commit rG82fc0d09e862: cp: Partly restore symlink folllowing. (authored by des). · Explain WhyMay 6 2025, 5:12 PM
This revision was automatically updated to reflect the committed changes.
des added a commit: rG82fc0d09e862: cp: Partly restore symlink folllowing..

Revision Contents
Changeset List

PathSize
bin/
cp/
416 lines
30 lines
tests/
145 lines
153 lines

Diff 154696

View Options

bin/cp/cp.c

Loading...
View Options

bin/cp/extern.h

Loading...
View Options

bin/cp/tests/cp_test.sh

Loading...
View Options

bin/cp/utils.c

Loading...