Skip to content

Fix cp -f, cp -p, chown -RP, chmod -R -P #20

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.

Sign up for GitHub

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

Open
kelp wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix cp -f, cp -p, chown -RP, chmod -R -P #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 68 additions & 5 deletions src/chmod.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,11 +500,11 @@ fn chmodRecursive(allocator: std.mem.Allocator, dir_path: []const u8, mode_spec:
continue;
}
}
// Default: don't follow symlinks (-P, -H during recursion)
const result = applyModeToPath(allocator, full_path, mode_spec, writer, stderr_writer, options);
if (!result) {
had_errors = true;
}
// Default (-P, -H during recursion): skip symlinks entirely.
// GNU chmod does not follow symlinks during recursive traversal
// and cannot change symlink permissions on Linux, so they are
// silently skipped.
continue;
},
else => {
// Handle all other file types (regular files, devices, etc.)
Expand Down Expand Up @@ -2352,3 +2352,66 @@ test "behavioral: chmod -w via runUtility removes write permission" {
const actual_mode = try getFileMode(abs_path);
try testing.expectEqual(@as(u32, 0o444), actual_mode);
}

test "chmod: -R -P should not follow symlinks during traversal" {
// GNU chmod -R does not follow symlinks during recursive traversal.
// When -P is active (default for -R), symlinks encountered inside a
// directory should be skipped — chmod must NOT modify the target file's
// permissions through the symlink.
if (std.c.getuid() == 0) return;

var tmp_dir = testing.tmpDir(.{});
defer tmp_dir.cleanup();

// Create target file OUTSIDE the directory tree being chmod'd
const target_file = try tmp_dir.dir.createFile("outside_target.txt", .{});
target_file.close();

// Create the directory to recurse into
try tmp_dir.dir.makeDir("mydir");
var mydir = try tmp_dir.dir.openDir("mydir", .{});
defer mydir.close();

// Create a regular file inside mydir
const inner_file = try mydir.createFile("regular.txt", .{});
inner_file.close();

// Create a symlink inside mydir pointing to the outside target
mydir.symLink("../outside_target.txt", "link_to_outside", .{}) catch |err| switch (err) {
error.AccessDenied => return, // symlinks not supported
else => return,
};

var mydir_buf: [std.fs.max_path_bytes]u8 = undefined;
const mydir_abs = try tmp_dir.dir.realpath("mydir", &mydir_buf);
var target_buf: [std.fs.max_path_bytes]u8 = undefined;
const target_abs = try tmp_dir.dir.realpath("outside_target.txt", &target_buf);

// Set known modes: target=0o644, regular=0o644
try setFileModeOctal(target_abs, 0o644);

var inner_buf: [std.fs.max_path_bytes]u8 = undefined;
const regular_abs = try tmp_dir.dir.realpath("mydir/regular.txt", &inner_buf);
try setFileModeOctal(regular_abs, 0o644);

var stdout_buf = try std.ArrayList(u8).initCapacity(testing.allocator, 0);
defer stdout_buf.deinit(testing.allocator);
var stderr_buf = try std.ArrayList(u8).initCapacity(testing.allocator, 0);
defer stderr_buf.deinit(testing.allocator);

// chmod -R -P 755 mydir — should NOT follow symlinks during traversal
const files = [_][]const u8{mydir_abs};
try chmodFiles(testing.allocator, "755", &files, stdout_buf.writer(testing.allocator), stderr_buf.writer(testing.allocator), ChmodOptions{
.recursive = true,
.no_traverse_symlinks = true,
});

// regular.txt inside mydir should be 755
const regular_mode = try getFileMode(regular_abs);
try testing.expectEqual(@as(u32, 0o755), regular_mode);

// outside_target.txt should still be 0o644 — the symlink must not
// have been followed to chmod the target
const target_mode = try getFileMode(target_abs);
try testing.expectEqual(@as(u32, 0o644), target_mode);
}
66 changes: 64 additions & 2 deletions src/chown.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,8 +384,15 @@ fn chownRecursive(
stderr_writer: anytype,
root_dev: ?u64,
) !void {
// Check if it's a directory to recurse into
const stat_info = common.file.FileInfo.stat(path) catch |err| {
// Check if it's a directory to recurse into.
// With -P (or default -R with no -H/-L), use lstat so a cmdline
// symlink-to-directory is not followed — we change the symlink itself.
const use_lstat = options.no_traverse_symlinks or
(!options.traverse_all_symlinks and !options.traverse_cmdline_symlinks);
const stat_info = (if (use_lstat)
common.file.FileInfo.lstat(path)
else
common.file.FileInfo.stat(path)) catch |err| {
common.printErrorWithProgram(allocator, stderr_writer, "chown", "cannot stat '{s}': {s}", .{ path, @errorName(err) });
return;
};
Expand Down Expand Up @@ -1271,6 +1278,61 @@ test "runChown production path with valid file and owner spec" {
try testing.expectEqual(@as(u8, 0), exit_code);
}

test "privileged: chown -RP should not follow cmdline symlink to directory" {
// GNU/POSIX: -P means "Do not traverse any symbolic links."
// When a symlink-to-directory is passed on the command line with -RP,
// chown should change the symlink itself, NOT recurse into the target.
var arena = privilege_test.TestArena.init();
defer arena.deinit();
const allocator = arena.allocator();

try privilege_test.requiresPrivilege();

try privilege_test.withFakeroot(allocator, struct {
fn testFn(inner_allocator: std.mem.Allocator) !void {
var tmp_dir = testing.tmpDir(.{});
defer tmp_dir.cleanup();

// Create target directory with a file inside
try tmp_dir.dir.makeDir("target");
var subdir = try tmp_dir.dir.openDir("target", .{});
defer subdir.close();
const file = try subdir.createFile("file.txt", .{});
file.close();

// Create a symlink to the target directory
try tmp_dir.dir.symLink("target", "link", .{});

var path_buf: [fs.max_path_bytes]u8 = undefined;
const tmp_path = try tmp_dir.dir.realpath(".", &path_buf);

const link_path = try std.fmt.allocPrint(inner_allocator, "{s}/link", .{tmp_path});

const current_uid = common.user_group.getCurrentUserId();
const current_gid = common.user_group.getCurrentGroupId();
const owner_spec = try std.fmt.allocPrint(inner_allocator, "{d}:{d}", .{ current_uid, current_gid });

// -R with -P (no_traverse_symlinks): should NOT follow the symlink
const options = ChownOptions{
.recursive = true,
.no_traverse_symlinks = true,
.verbose = true,
};

var stdout_buffer = try std.ArrayList(u8).initCapacity(inner_allocator, 0);
defer stdout_buffer.deinit(inner_allocator);
var stderr_buffer = try std.ArrayList(u8).initCapacity(inner_allocator, 0);
defer stderr_buffer.deinit(inner_allocator);

try chownFile(inner_allocator, link_path, owner_spec, options, stdout_buffer.writer(inner_allocator), stderr_buffer.writer(inner_allocator));

// Verbose output should only mention "link", not "target/file.txt"
const output = stdout_buffer.items;
try testing.expect(std.mem.indexOf(u8, output, "file.txt") == null);
}
}.testFn);
}

test "chown help text includes new flags" {
var stdout_buffer = try std.ArrayList(u8).initCapacity(testing.allocator, 0);
defer stdout_buffer.deinit(testing.allocator);
Expand Down
150 changes: 145 additions & 5 deletions src/cp.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,18 +483,35 @@ fn copyRegularFile(allocator: Allocator, stderr_writer: anytype, source_path: []
return false;
};

// Handle force overwrite if needed
// Handle force overwrite if needed.
// GNU spec: "if an existing destination file cannot be opened, remove it
// and try again." Only unlink when the file cannot be opened for writing;
// this preserves hard links to writable destinations.
var dest_unlinked = false;
if (fileExists(dest_path) and options.force) {
handleForceOverwrite(dest_path) catch |err| {
common.printErrorWithProgram(allocator, stderr_writer, "cp", "cannot remove '{s}': {s}", .{ dest_path, getStandardErrorName(err) });
return false;
};
if (std.fs.cwd().openFile(dest_path, .{ .mode = .write_only })) |f| {
// Destination is writable — no need to unlink
f.close();
} else |_| {
// Cannot open for writing — unlink and retry
handleForceOverwrite(dest_path) catch |e| {
common.printErrorWithProgram(allocator, stderr_writer, "cp", "cannot remove '{s}': {s}", .{ dest_path, getStandardErrorName(e) });
return false;
};
dest_unlinked = true;
}
}

if (options.preserve) {
copyFileWithAttributes(allocator, stderr_writer, source_path, dest_path, source_stat) catch {
return false;
};
} else if (!dest_unlinked and fileExists(dest_path)) {
// Destination exists and was not unlinked: overwrite in place to
// preserve the inode (and thus hard links).
copyInPlace(allocator, stderr_writer, source_path, dest_path) catch {
return false;
};
} else {
std.fs.cwd().copyFile(source_path, std.fs.cwd(), dest_path, .{}) catch |err| {
common.printErrorWithProgram(allocator, stderr_writer, "cp", "cannot copy '{s}' to '{s}': {s}", .{ source_path, dest_path, getStandardErrorName(err) });
Expand All @@ -505,6 +522,33 @@ fn copyRegularFile(allocator: Allocator, stderr_writer: anytype, source_path: []
return true;
}

/// Copy file contents in place, preserving the destination inode.
/// Opens both files, truncates the destination, and copies data.
fn copyInPlace(allocator: Allocator, stderr_writer: anytype, source_path: []const u8, dest_path: []const u8) !void {
const source_file = std.fs.cwd().openFile(source_path, .{}) catch |err| {
common.printErrorWithProgram(allocator, stderr_writer, "cp", "cannot open '{s}': {s}", .{ source_path, getStandardErrorName(err) });
return error.SourceNotReadable;
};
defer source_file.close();

const dest_file = std.fs.cwd().openFile(dest_path, .{ .mode = .write_only }) catch |err| {
common.printErrorWithProgram(allocator, stderr_writer, "cp", "cannot open '{s}' for writing: {s}", .{ dest_path, getStandardErrorName(err) });
return error.DestinationNotWritable;
};
defer dest_file.close();

// Truncate to zero before writing new content
dest_file.setEndPos(0) catch |err| {
common.printErrorWithProgram(allocator, stderr_writer, "cp", "cannot truncate '{s}': {s}", .{ dest_path, getStandardErrorName(err) });
return error.DestinationNotWritable;
};

common.file_ops.copyFileContents(source_file, dest_file) catch |err| {
common.printErrorWithProgram(allocator, stderr_writer, "cp", "error copying '{s}' to '{s}': {s}", .{ source_path, dest_path, @errorName(err) });
return error.SourceNotReadable;
};
}

/// Copy a symbolic link
fn copySymlink(allocator: Allocator, stderr_writer: anytype, source_path: []const u8, dest_path: []const u8, options: RuntimeOptions) bool {
// Read the symlink target
Expand Down Expand Up @@ -676,6 +720,23 @@ fn copyFileWithAttributes(allocator: Allocator, stderr_writer: anytype, source_p
dest_file.updateTimes(source_stat.atime, source_stat.mtime) catch |err| {
common.printWarningWithProgram(allocator, stderr_writer, "cp", "cannot preserve timestamps for '{s}': {s}", .{ dest_path, getStandardErrorName(err) });
};

// Preserve ownership (uid/gid) — GNU cp -p is --preserve=mode,ownership,timestamps.
// Use fstat on the source fd to get uid/gid, then fchown on the dest fd.
// Silently ignore EPERM (non-root cannot chown to other users).
const src_info = common.file.FileInfo.statFile(source_file) catch {
return; // Cannot stat source; ownership preservation skipped
};
const fchown_result = std.c.fchown(dest_file.handle, src_info.uid, src_info.gid);
if (fchown_result != 0) {
const errno = std.c._errno().*;
switch (errno) {
@intFromEnum(std.c.E.PERM) => {}, // Non-root; silently ignore
else => {
common.printWarningWithProgram(allocator, stderr_writer, "cp", "cannot preserve ownership for '{s}'", .{dest_path});
},
}
}
}

/// Get file type atomically to avoid race conditions
Expand Down Expand Up @@ -2124,3 +2185,82 @@ test "cp: -f reports error when force-remove of destination fails" {
const stderr_output = stderr_buffer.items;
try testing.expect(std.mem.indexOf(u8, stderr_output, "cannot remove") != null);
}

test "cp: -f should not unlink writable destination (preserves hard links)" {
// GNU spec: "if an existing destination file cannot be opened, remove it
// and try again." A writable destination CAN be opened, so -f must NOT
// unlink it. Unlinking a writable file severs hard links unnecessarily.
var test_dir = TestDir.init(testing.allocator);
defer test_dir.deinit();

try test_dir.createFile("source.txt", "new content", null);
try test_dir.createFile("dest.txt", "old content", null);

const base_path = try test_dir.getBasePath();
defer testing.allocator.free(base_path);
const dest_path = try std.fmt.allocPrint(testing.allocator, "{s}/dest.txt", .{base_path});
defer testing.allocator.free(dest_path);
const hardlink_path = try std.fmt.allocPrint(testing.allocator, "{s}/hardlink.txt", .{base_path});
defer testing.allocator.free(hardlink_path);

// Create a hard link to dest.txt
const dest_path_z = try std.posix.toPosixPath(dest_path);
const hardlink_path_z = try std.posix.toPosixPath(hardlink_path);
const link_result = std.c.link(&dest_path_z, &hardlink_path_z);
if (link_result != 0) {
// Hard links not supported on this filesystem; skip
return;
}

// Verify they share the same inode before the copy
try testing.expect(common.file_ops.isSameFile(dest_path, hardlink_path));

const source_path = try test_dir.getPath("source.txt");
defer testing.allocator.free(source_path);

var stderr_buffer = try std.ArrayList(u8).initCapacity(testing.allocator, 0);
defer stderr_buffer.deinit(testing.allocator);

const args = [_][]const u8{ "-f", source_path, dest_path };
const exit_code = try runUtility(testing.allocator, &args, common.null_writer, stderr_buffer.writer(testing.allocator));
try testing.expectEqual(@as(u8, 0), exit_code);

// dest.txt should have the new content
try test_dir.expectFileContent("dest.txt", "new content");

// The hard link must still point to the same inode as dest.txt.
// If -f wrongly unlinked dest.txt first, dest.txt gets a NEW inode
// and the hard link is severed (still points to the old inode).
try testing.expect(common.file_ops.isSameFile(dest_path, hardlink_path));
}

test "cp: -p should preserve group ownership via chown" {
// GNU spec: -p is --preserve=mode,ownership,timestamps.
// Ownership means uid and gid must be copied via chown.
// This test verifies that the source file's gid is preserved
// on the destination (at minimum, the attempt is made).
var test_dir = TestDir.init(testing.allocator);
defer test_dir.deinit();

try test_dir.createFile("source.txt", "preserve me", null);

const source_path = try test_dir.getPath("source.txt");
defer testing.allocator.free(source_path);
const base_path = try test_dir.getBasePath();
defer testing.allocator.free(base_path);
const dest_path = try std.fmt.allocPrint(testing.allocator, "{s}/dest.txt", .{base_path});
defer testing.allocator.free(dest_path);

var stderr_buffer = try std.ArrayList(u8).initCapacity(testing.allocator, 0);
defer stderr_buffer.deinit(testing.allocator);

const args = [_][]const u8{ "-p", source_path, dest_path };
const exit_code = try runUtility(testing.allocator, &args, common.null_writer, stderr_buffer.writer(testing.allocator));
try testing.expectEqual(@as(u8, 0), exit_code);

// Verify the destination has the same gid as the source.
// As a non-root user, chown to the same gid should succeed.
const src_info = try common.file.FileInfo.stat(source_path);
const dst_info = try common.file.FileInfo.stat(dest_path);
try testing.expectEqual(src_info.gid, dst_info.gid);
}
Loading