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.

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

Mitigate copying/deletion of unrecognised symlinks #5953

Merged
merged 1 commit into from
May 15, 2024
Merged

Mitigate copying/deletion of unrecognised symlinks #5953

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
2 changes: 2 additions & 0 deletions master_changes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ users)
## Exec

## Source
* Fix extraction of tarballs on Windows which contain symlinks both when those symlinks can't be created or if they point to files which don't exist [#5953 @dra27]

## Lint

Expand Down Expand Up @@ -153,3 +154,4 @@ users)

## opam-core
* `OpamStd.String`: add `split_quoted` that preserves quoted separator [#5935 @dra27]
* `OpamSystem.copy_dir` and `OpamSystem.mv` may display a warning on Windows if an invalid symlink (e.g. an LXSS Junction) is found [#5953 @dra27]
42 changes: 22 additions & 20 deletions src/core/opamSystem.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,19 +129,15 @@ let win32_unlink fn =
with _ -> raise e)

let remove_file_t ?(with_log=true) file =
if
try ignore (Unix.lstat file); true with Unix.Unix_error _ -> false
Copy link
Member

@kit-ty-kate kit-ty-kate May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove_file_t on a non-existing file used to simply ignore it. This PR changes this behaviour to error out instead.
Was the old behaviour used anywhere that we know?

Copy link
Member Author

@dra27 dra27 May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so - its only other use is in the cross-device fallback for mv. The only way it could be called with a file which didn't exist is if the file has been removed between the test in the calling function and the test here, and there was already a race for that anyway (between the test I've deleted here and the internal_error handler below).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is also being used in remove_file and remove_dir which are used everywhere in opam's code (e.g. via OpamFilename.remove)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove_file doesn't use remove_file_t (LL202-215) and remove_dir already checks whether its argument exists?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AH! I see. There are two toplevel versions of remove_file in the same file. Only the latter is exported. Sorry for missing that. This is all good then.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, until you’d said, I’d missed that too! I was only sure that the code was ok because I’d deleted the first remove_file as a result of an unused value warning and already searched for other uses of remove_file_t 🙂

then (
try
if with_log || log_for_file_management () then
log "rm %s" file;
if Sys.win32 then
win32_unlink file
else
Unix.unlink file
with Unix.Unix_error _ as e ->
internal_error "Cannot remove %s (%s)." file (Printexc.to_string e)
)
try
if with_log || log_for_file_management () then
log "rm %s" file;
if Sys.win32 then
win32_unlink file
else
Unix.unlink file
with Unix.Unix_error _ as e ->
internal_error "Cannot remove %s (%s)." file (Printexc.to_string e)

let rec remove_dir_t dir =
let files = get_files dir in
Expand All @@ -152,18 +148,20 @@ let rec remove_dir_t dir =
remove_dir_t file
| {Unix.st_kind = Unix.(S_REG | S_LNK | S_CHR | S_BLK | S_FIFO | S_SOCK); _} ->
remove_file_t ~with_log:false file
| exception Unix.(Unix_error (ENOENT, _, _)) when Sys.win32 ->
(* This is usually caused by invalid symlinks extracted by a
Cygwin/MSYS2 tar. *)
remove_file_t ~with_log:false file
) files;
Unix.rmdir dir

let remove_file = remove_file_t ~with_log:true

let remove_dir dir =
log "rmdir %s" dir;
if Sys.file_exists dir then begin
if Sys.is_directory dir then
remove_dir_t dir
else
remove_file dir
remove_file_t ~with_log:true dir
end

let temp_files = Hashtbl.create 1024
Expand Down Expand Up @@ -711,15 +709,15 @@ let rec link_t ?(with_log=true) src dst =
else
copy_file_t src dst

and copy_dir_t ?(with_log=true) src dst =
and copy_dir_t ?(with_log=true) src dst_dir =
if with_log || log_for_file_management () then
log "copydir %s -> %s" src dst;
log "copydir %s -> %s" src dst_dir;
let files = get_files src in
mkdir dst;
mkdir dst_dir;
let with_log = false in
List.iter (fun file ->
let src = Filename.concat src file in
let dst = Filename.concat dst file in
let dst = Filename.concat dst_dir file in
match Unix.lstat src with
| {Unix.st_kind = Unix.S_REG; _} ->
copy_file_t ~with_log src dst
Expand All @@ -736,6 +734,10 @@ and copy_dir_t ?(with_log=true) src dst =
failwith (Printf.sprintf "Copying named pipes (%s) is unsupported" src)
| {Unix.st_kind = Unix.S_SOCK; _} ->
failwith (Printf.sprintf "Copying sockets (%s) is unsupported" src)
| exception Unix.(Unix_error (ENOENT, _, _)) when Sys.win32 ->
(* This is usually caused by invalid symlinks extracted by a
Cygwin/MSYS2 tar. *)
OpamConsole.warning "Warning: cannot copy %s to %s" src dst_dir
) files

let copy_dir = copy_dir_t ~with_log:true
Expand Down
Loading