resolve command: don't select directories

master_changes.md

@@ -111,10 +111,14 @@ users)
  * Move local-cache into archive-field-checks test [#5560 @rjbou]
  * Admin: add `admin add-extrafiles` test cases [#5647 @rjbou]
  * Add download test, to check `OPAMCURL/OPAMFETCH` handling [#5607 @rjbou]
+ * Add `core/opamSystem.ml` specific tests, to test command resolution [#5600 @rjbou]
 
 ### Engine
  * With real path resolved for all opam temp dir, remove `/private` from mac temp dir regexp [#5654 @rjbou]
  * Reimplement `sed-cmd` command regexp, to handle prefixed commands with path not only in subprocess, but anywere in output [#5657 #5607 @rjbou]
+ * Add environment variables path addition [#5606 @rjbou]
+ * Remove duplicated environment variables in environmenet [#5606 @rjbou]
+ * Add `PATH` to replaceable variables [#5606 @rjbou]
 
 ## Github Actions
  * Add coreutils install for cheksum validation tests [#5560 @rjbou]
@@ -144,3 +148,4 @@ users)
 
 ## opam-core
  * `OpamSystem.mk_temp_dir`: resolve real path with `OpamSystem.real_path` before returning it [#5654 @rjbou]
+ * `OpamSystem.resolve_command`: in command resolution path, check that the file is not a directory and that it is a regular file [#5606 @rjbou - fix #5585 #5597]

src/core/opamSystem.ml

@@ -441,11 +441,11 @@ let t_resolve_command =
  else fun f ->
  try
  let open Unix in
- let uid = geteuid () in
+ let {st_uid; st_gid; st_perm; st_kind; _} = stat f in
+ if st_kind <> Unix.S_REG then false else
  let groups = OpamStd.IntSet.of_list (getegid () :: Array.to_list (getgroups ())) in
- let {st_uid; st_gid; st_perm; _} = stat f in
  let mask =
- if uid = st_uid then
+ if geteuid () = st_uid then
  0o100
  else if OpamStd.IntSet.mem st_gid groups then
  0o010
@@ -488,9 +488,13 @@ let t_resolve_command =
  name ^ ".exe"
  else name
  in
- let possibles = OpamStd.List.filter_map (fun path ->
- let candidate = Filename.concat path name in
- if Sys.file_exists candidate then Some candidate else None) path
+ let possibles =
+ OpamStd.List.filter_map (fun path ->
+ let candidate = Filename.concat path name in
+ match Sys.is_directory candidate with
+ | false -> Some candidate
+ | true | exception (Sys_error _) -> None)
+ path
  in
  match List.find check_perms possibles with
  | cmdname -> `Cmd cmdname

tests/reftests/core-system.unix.test

@@ -0,0 +1,32 @@
+N0REP0
+### # This test is unix only until https://github.com/ocaml/opam/pull/5682 resolution
+### ::::::::::::::::::::::::::::::::::
+### :I: System.resolve_commands checks
+### ::::::::::::::::::::::::::::::::::
+### opam switch create resolve --empty
+### :I:a: System one
+### opam exec -- echo system
+system
+### opam exec --no-switch -- echo system
+system
+### :I:b: is a directory
+### mkdir -p bin/echo
+### PATH+=$BASEDIR/bin/
+### opam exec -- echo system
+system
+### opam exec --no-switch -- echo system
+system
+### :I:c: is not executable
+### rm -rf bin/echo
+### <bin/echo>
+echo 'echo'
+### opam exec -- echo system
+system
+### opam exec --no-switch -- echo system
+system
+### :I:d: is executable
+### chmod +x bin/echo
+### opam exec -- echo system
+echo
+### opam exec --no-switch -- echo system
+echo

tests/reftests/dune.inc

@@ -269,6 +269,27 @@
  %{targets}
  (run ./run.exe %{exe:../../src/client/opamMain.exe.exe} %{dep:conflict-solo5.test} %{read-lines:testing-env}))))
 
+(rule
+ (alias reftest-core-system.unix)
+ (enabled_if (= %{os_type} "Unix"))
+ (action
+ (diff core-system.unix.test core-system.unix.out)))
+
+(alias
+ (name reftest)
+ (enabled_if (= %{os_type} "Unix"))
+ (deps (alias reftest-core-system.unix)))
+
+(rule
+ (targets core-system.unix.out)
+ (deps root-N0REP0)
+ (enabled_if (= %{os_type} "Unix"))
+ (package opam)
+ (action
+ (with-stdout-to
+ %{targets}
+ (run ./run.exe %{exe:../../src/client/opamMain.exe.exe} %{dep:core-system.unix.test} %{read-lines:testing-env}))))
+
 (rule
  (alias reftest-cudf-preprocess)
  (action

tests/reftests/run.ml

@@ -196,7 +196,10 @@ let command
  let env =
  Array.of_list @@
  List.map (fun (var, value) -> Printf.sprintf "%s=%s" var value) @@
- (base_env @ vars)
+ (List.filter (fun (v,_) ->
+ List.find_opt (fun (v',_) -> String.equal v v') vars = None)
+ base_env)
+ @ vars
  in
  let input, stdout = Unix.pipe () in
  Unix.set_close_on_exec input;
@@ -306,7 +309,7 @@ type command =
  filter: (Re.t * filt_sort) list;
  output: string option;
  unordered: bool; }
- | Export of (string * string) list
+ | Export of (string * [`eq | `pluseq | `eqplus] * string) list
  | Comment of string
 
 module Parse = struct
@@ -344,7 +347,7 @@ module Parse = struct
  let re_varbind =
  seq [
  group @@ seq [alpha; rep (alt [alnum; set "_-"])];
- char '=';
+ group @@ alt [ str "+="; str "=+"; char '=' ];
  group @@ re_str_atom;
  rep space;
  ]
@@ -383,7 +386,14 @@ module Parse = struct
  else
  let varbinds, pos =
  let gr = exec (compile @@ rep re_varbind) str in
- List.map (fun gr -> Group.get gr 1, get_str (Group.get gr 2))
+ List.map (fun gr ->
+ Group.get gr 1,
+ (match Group.get gr 2 with
+ | "=" -> `eq
+ | "+=" -> `pluseq
+ | "=+" -> `eqplus
+ | _ -> assert false),
+ get_str (Group.get gr 3))
  (all (compile @@ re_varbind) (Group.get gr 0)),
  Group.stop gr 0
  in
@@ -498,8 +508,23 @@ module Parse = struct
  | Some "json-cat" ->
  Json { files = args; filter = rewr; }
  | Some cmd ->
+ let env, plus =
+ List.fold_left (fun (env,plus) (v,op,value) ->
+ match op with
+ | `eq -> (v,value)::env, plus
+ | `pluseq -> env, (v^"+="^value)::plus
+ | `eqplus -> env, (v^"=+"^value)::plus)
+ ([],[]) varbinds
+ in
+ (match plus with
+ | [] -> ()
+ | _ ->
+ OpamConsole.error
+ "variable bindings at the beginning of a command does not \
+ support '+=' or '=+' operators: %s"
+ (OpamStd.Format.pretty_list plus));
  Run {
- env = varbinds;
+ env;
  cmd;
  args;
  filter = rewr;
@@ -671,6 +696,7 @@ let run_test ?(vars=[]) ~opam t =
  "OPAM", opam.as_seen_in_opam;
  "OPAMROOT", opamroot;
  "BASEDIR", dir;
+ "PATH", Sys.getenv "PATH";
  ] @ vars
  in
  if t.repo_hash = no_opam_repo then
@@ -741,12 +767,24 @@ let run_test ?(vars=[]) ~opam t =
  vars
  | Export bindings ->
  List.fold_left
- (fun vars (v, r) ->
- let r =
+ (fun vars -> fun (v, op, r) ->
+ let r' =
  str_replace_path ~escape:`Backslashes
  OpamSystem.forward_to_back (filters_of_var vars) r
  in
- (v, r) :: List.filter (fun (w, _) -> not (String.equal v w)) vars)
+ let value =
+ match op with
+ | `eq -> r'
+ | (`pluseq | `eqplus) as op ->
+ match List.find_opt (fun (v',_) -> String.equal v v') (base_env @ vars) with
+ | Some (_,c) ->
+ let sep = if Sys.win32 then ";" else ":" in
+ (match op with
+ | `pluseq -> r'^sep^c
+ | `eqplus -> c^sep^r')
+ | None -> r'
+ in
+ (v, value) :: List.filter (fun (w, _) -> not (String.equal v w)) vars)
  vars bindings
  | Cat { files; filter } ->
  let files =