mc-wrapper needs rewriting to fit changes in #4535

[dmartina]

As in #4535 the temporary directory seems to have switched from using a user name based name to the process ID, and now mc-wrapper is broken. It keeps using `whoami` in the temporary directory name: the directory doesn't exist, the proposed output file unreachable, the returned path is lost and the shell goes back to the initial directory.

As a workaround /tmp/mc-userxxx may be recreated after every boot/reboot and everything behaves as expected. It's worth fixing the script, sniff...

David

[zaytsev]

Can you test if this works? In particular, I have no clue about csh.

diff --git a/contrib/mc-wrapper.csh.in b/contrib/mc-wrapper.csh.in
index 1eca8e7b7..c6ba07c30 100644
--- a/contrib/mc-wrapper.csh.in
+++ b/contrib/mc-wrapper.csh.in
@@ -1,9 +1,7 @@
-set MC_USER=`whoami`
-
 if ($?TMPDIR) then
-       setenv MC_PWD_FILE $TMPDIR/mc-$MC_USER/mc.pwd.$$
+       setenv MC_PWD_FILE `echo 'mkstemp(template)' | m4 -D template="$TMPDIR/mc.pwd.XXXXXX"`
 else
-       setenv MC_PWD_FILE /tmp/mc-$MC_USER/mc.pwd.$$
+       setenv MC_PWD_FILE `echo 'mkstemp(template)' | m4 -D template="/tmp/mc.pwd.XXXXXX"`
 endif
 
 @bindir@/mc -P "$MC_PWD_FILE" $*
@@ -18,4 +16,3 @@ endif
 
 rm -f "$MC_PWD_FILE"
 unsetenv MC_PWD_FILE
-unsetenv MC_USER
diff --git a/contrib/mc-wrapper.sh.in b/contrib/mc-wrapper.sh.in
index 3905be480..72fddcc85 100644
--- a/contrib/mc-wrapper.sh.in
+++ b/contrib/mc-wrapper.sh.in
@@ -1,5 +1,9 @@
-MC_USER=`whoami`
-MC_PWD_FILE="${TMPDIR-/tmp}/mc-$MC_USER/mc.pwd.$$"
+MC_PWD_FILE=`
+  echo 'mkstemp(template)' |
+    m4 -D template="${TMPDIR:-/tmp}/mc.pwd.XXXXXX"
+` || exit
+trap 'rm -f -- "$MC_PWD_FILE"' INT TERM HUP EXIT
+
 @bindir@/mc -P "$MC_PWD_FILE" "$@"
 
 if test -r "$MC_PWD_FILE"; then
@@ -10,6 +14,4 @@ if test -r "$MC_PWD_FILE"; then
        unset MC_PWD
 fi
 
-rm -f "$MC_PWD_FILE"
 unset MC_PWD_FILE
-unset MC_USER

[andrew_b]

No m4 please. We don't want yet another run-time dependency.

mc-wrapper should use MC_TMPDIR variable if it's defined in system. If not, define it itself for mc. mc-wrapper and mc_tmpdir() should be in consistence.

I'm working on it.

[zaytsev]

No m4 please. We don't want yet another run-time dependency.

Just to explain my approach:

Unfortunately, I couldn't find any POSIX shell command to create a temporary file with a unique name. There is a POSIX call mkstemp though. The standard specifies, that a compliant POSIX system should have m4. Hence the hack of using the following to create a temporary file:

echo 'mkstemp(template)' | m4 -D template="/tmp/mc.pwd.XXXXXX"

I can see that we have mktemp though. I'm not sure how portable it is, but apparently using it doesn't make things worse:

diff --git a/contrib/mc-wrapper.csh.in b/contrib/mc-wrapper.csh.in
index 1eca8e7b7..5fedb7aa6 100644
--- a/contrib/mc-wrapper.csh.in
+++ b/contrib/mc-wrapper.csh.in
@@ -1,9 +1,7 @@
-set MC_USER=`whoami`
-
 if ($?TMPDIR) then
-       setenv MC_PWD_FILE $TMPDIR/mc-$MC_USER/mc.pwd.$$
+       setenv MC_PWD_FILE `mktemp $MC_TMPDIR/mc.pwd.XXXXXX`
 else
-       setenv MC_PWD_FILE /tmp/mc-$MC_USER/mc.pwd.$$
+       setenv MC_PWD_FILE `mktemp /tmp/mc.pwd.XXXXXX`
 endif
 
 @bindir@/mc -P "$MC_PWD_FILE" $*
@@ -18,4 +16,3 @@ endif
 
 rm -f "$MC_PWD_FILE"
 unsetenv MC_PWD_FILE
-unsetenv MC_USER
diff --git a/contrib/mc-wrapper.sh.in b/contrib/mc-wrapper.sh.in
index 3905be480..bbf24bf8c 100644
--- a/contrib/mc-wrapper.sh.in
+++ b/contrib/mc-wrapper.sh.in
@@ -1,5 +1,6 @@
-MC_USER=`whoami`
-MC_PWD_FILE="${TMPDIR-/tmp}/mc-$MC_USER/mc.pwd.$$"
+MC_PWD_FILE=`mktemp ${MC_TMPDIR:-/tmp}/mc.pwd.XXXXXX` || exit 1
+trap 'rm -f -- "$MC_PWD_FILE"' INT TERM HUP EXIT
+
 @bindir@/mc -P "$MC_PWD_FILE" "$@"
 
 if test -r "$MC_PWD_FILE"; then
@@ -10,6 +11,4 @@ if test -r "$MC_PWD_FILE"; then
        unset MC_PWD
 fi
 
-rm -f "$MC_PWD_FILE"
 unset MC_PWD_FILE
-unset MC_USER

I'm working on it.

Are you working on something like the patch above? Or something more? I'm confused.

[andrew_b]

Branch: 4575_mc-wrapper
changeset:1e299eefe7a12a3c707997f918056d1a22c2e49c

[ossi]

uh, no, you can't construct predictable paths in a shared temp dir - this is the classic vector for symlink attacks.

i think the way to go is to have configure find the mktemp equivalent command (may be tempfile on some systems).

i for one have this, but it suffers from obvious portability issues:

> type mc
mc is a function
mc ()
{
    local tf=$XDG_RUNTIME_DIR/mc-wd-$$;
    /usr/local/bin/mc -P $tf "$@" && test -r $tf && cd "$(< $tf)";
    rm -f $tf
}

[andrew_b]

Replying to ossi:

predictable paths

Where?

[ossi]

$$ is largely predictable.

[dmartina]

Replying to ossi:

i think the way to go is to have configure find the mktemp equivalent command (may be tempfile on some systems).

Me, I just changed

MC_PWD_FILE="$(mktemp -ut mc.pwd.XXXXXXXXXX.$$)"

It's a recent Ubuntu 24.04 where tempfile seems to be deprecated, and instead of it mktemp is recommended. The -t option looks handy to deal with $TMPDIR. I can't figure why it didn't work without the -u that is publicised as unsafe... Surely replacing the bash $() with tildes is a must to provide a more portable script.

Simple to me. But I'm afraid getting to a pretty universal solution is to be an endless job.

Is a temporary file really needed?

[andrew_b]

Replying to ossi:

$$ is largely predictable.

But you still use it.

[andrew_b]

Replying to dmartina:

Is a temporary file really needed?

Yes if you want to stay in the mc's working directory after exit from.

[ossi]

Replying to andrew_b:

Replying to ossi:

$$ is largely predictable.

But you still use it.

because $XDG_RUNTIME_DIR isn't shared.

[dmartina]

Replying to andrew_b:

Branch: 4575_mc-wrapper
changeset:1e299eefe7a12a3c707997f918056d1a22c2e49c

If MC is run with '-P filepath' from outside any mc-wrapper shell no one deletes the temporary directory. Surely something could be tricked to abuse it...

if (!pwd_file_written) 
  (void) my_rmdir (tmpdir); 

[dmartina]

I have been reading once and again andrew's patch. I feel like there are no benefits in changing mc main code code. There are environment variables with mixed roles that produce confusing cases.

Let me reorder some ideas. We deal with:

• A system directory to contain temporary files o directories TMPDIR/sys_tmp. Should exist before running mc and is shared with other programs or even different instances of mc.
• An mc alternative to overwrite the previous system defined directory MC_TMPDIR/sys_tmp. If it is to be shared, it also "should exist before running mc and is shared with other programs or even different instances of mc".
• A subdirectory created for each running instance of mc. With a unique name as created by g_mdktemp() with a proper template. I think that the easiest way is to create it and delete it by mc as it is now done. It's difficult to accomplish the mentioned compromise with mc-wrapper about it been created/removed from inside/outside.
• Last, we need this file MC_PWD_FILE for the last directory path name communicated from mc to the parent shell. I would store it into the MC_TMPDIR or TMPDIR, but I find it difficult to keep it in the instance volatile temp directory. So, at this point, we need a procedure similar to the one used by gdk_mdktemp() ¿the mktemp command? to name or even create this file with another unique and preferable non predictable name. There is no need to match both unique names; actually, it would be hard and not worth.

Concluding. I don't find any benefit in changing the main code as the compromise between the main binary and the script will always be weak. We just want to fix the script in the most basic/portable dialect we can figure out. It's also true that the number of popular shells has significantly reduced since the early days of mc.

[zaytsev]

Replying to ossi:

uh, no, you can't construct predictable paths in a shared temp dir - this is the classic vector for symlink attacks.

So could you please explain to me how one could possibly mount such an attack on mc, and more importantly, how to protect from it in a meaningful way? This is an honest question. People keep complaining about it, but I still don't really understand the attack vector and mitigation:

​https://bugs.launchpad.net/ubuntu/+source/mc/+bug/129133

I have asked Andrew to use g_mkdtemp API in #4535, which he did, but what I still don't really get is, what's the use, really if you could ls /tmp/mc-* and just look at everything you can find? One can drop the prefix, but then again, as long as you can read the directory in the first place, how does unpredictability give you any protection, especially against enumeration, which is totally practical?

i think the way to go is to have configure find the mktemp equivalent command (may be tempfile on some systems).

Autoconf people suggest to use mktemp -d and fallback to $RANDOM if it's not available. We could also just implement own mktemp in mc binary, but this really feels like an overkill to me.

​https://www.gnu.org/software/autoconf/manual/autoconf-2.63/html_node/Limitations-of-Usual-Tools.html

mktemp

Shell scripts can use temporary files safely with mktemp, but it does not exist on all systems. A portable way to create a safe temporary file name is to create a temporary directory with mode 700 and use a file inside this directory. Both methods prevent attackers from gaining control, though mktemp is far less likely to fail gratuitously under attack.

Here is sample code to create a new temporary directory safely:

# Create a temporary directory $tmp in $TMPDIR (default /tmp).
# Use mktemp if possible; otherwise fall back on mkdir,
# with $RANDOM to make collisions less likely.
: ${TMPDIR=/tmp}
{
  tmp=`
    (umask 077 && mktemp -d "$TMPDIR/fooXXXXXX") 2>/dev/null
  ` &&
  test -n "$tmp" && test -d "$tmp"
} || {
  tmp=$TMPDIR/foo$$-$RANDOM
  (umask 077 && mkdir "$tmp")
} || exit $?

[zaytsev]

Replying to andrew_b:

Branch: 4575_mc-wrapper
changeset:1e299eefe7a12a3c707997f918056d1a22c2e49c

To me this looks extremely complicated and dangerous/insecure (because of using the PIDs for uniqueness - which I have seen to wrap around quite often), and as you commented the stuff has to be maintained in sync between the wrapper and mc code, which makes it more fragile.

Could you please explain what problem are you trying to solve with this approach? I really want to understand your point.

I think that the patch I suggested in comment:4 (whithout m4) is very simple and secure. I can improve it to use mktemp -d for some more portability (don't know if we really need a fallback to $RANDOM). It creates an extra file inside $MC_TMPDIR, so it respects it.

The only difference is that it's not in the mc's internal instance sub-directory, but it's inside of the parent directory and is going to be removed when the script is finished anyways, so what's the deal?

[dmartina]

Replying to zaytsev:

I think that the patch I suggested in comment:4 (whithout m4) is very simple and secure. I can improve it to use mktemp -d for some more portability (don't know if we really need a fallback to $RANDOM). It creates an extra file inside $MC_TMPDIR, so it respects it.

Me too, I like the approach in comment:4. I also found a clue about the program not completing the final directory change if the PWD file already exists: the fopen flag O_EXCL... I suppose it also failed to you.

07f3e97a67ea5714d83c749c6694f7058b24c10b Fix mc-wrapper PWD file name for #4575
diff --git a/contrib/mc-wrapper.sh.in b/contrib/mc-wrapper.sh.in
index 3905be4..300bdb1 100644
--- a/contrib/mc-wrapper.sh.in
+++ b/contrib/mc-wrapper.sh.in
@@ -1,15 +1,19 @@
-MC_USER=`whoami`
-MC_PWD_FILE="${TMPDIR-/tmp}/mc-$MC_USER/mc.pwd.$$"
-@bindir@/mc -P "$MC_PWD_FILE" "$@"
+MC_PWD_DIR="${MC_TMPDIR-${TMPDIR-/tmp}}"
+MC_PWD_FILE=`mktemp -p "$MC_PWD_DIR" mc.pwd-XXXXX`
 
-if test -r "$MC_PWD_FILE"; then
-	MC_PWD="`cat "$MC_PWD_FILE"`"
-	if test -n "$MC_PWD" && test "$MC_PWD" != "$PWD" && test -d "$MC_PWD"; then
-		cd "$MC_PWD"
+if [ $? -eq 0 ] ; then
+	@bindir@/mc -P "$MC_PWD_FILE" "$@"
+	
+	if test -r "$MC_PWD_FILE"; then
+		MC_PWD="`cat "$MC_PWD_FILE"`"
+		if [ -n "$MC_PWD" ] && [ "$MC_PWD" != "$PWD" ] && [ -d "$MC_PWD" ] ; then
+			cd "$MC_PWD"
+		fi
+		unset MC_PWD
 	fi
-	unset MC_PWD
+	
+	rm -f "$MC_PWD_FILE"
 fi
 
-rm -f "$MC_PWD_FILE"
 unset MC_PWD_FILE
-unset MC_USER
+unset MC_PWD_DIR
diff --git a/src/main.c b/src/main.c
index 803cec1..ebf1d91 100644
--- a/src/main.c
+++ b/src/main.c
@@ -510,5 +510,5 @@
         int last_wd_fd;
 
-        last_wd_fd = open (mc_args__last_wd_file, O_WRONLY | O_CREAT | O_TRUNC | O_EXCL,
+        last_wd_fd = open (mc_args__last_wd_file, O_WRONLY | O_CREAT | O_TRUNC,
                            S_IRUSR | S_IWUSR);
         if (last_wd_fd != -1)

The exit for mktemp errors also causes unlikely effects. On SSH sessions it closes the connection.

[ossi]

the fundamental idea behind a symlink attack is that someone with low privileges creates a symlink to a high-value target, which is then inadvertently accessed by a user with high privileges. by symlinking mc's pwd output file one could cause mc run by root to overwrite arbitrary files, which can have effects from DoS (e.g., kill /etc/passwd) to root shell access (e.g., if making some file invalid removes access controls from something).

i for one don't see much use in creating a temp dir for the pwd file, for the reasons @dmartina named. just running mktemp (or an equivalent) with default parameters will do.

[zaytsev]

i for one don't see much use in creating a temp dir for the pwd file, for the reasons @dmartina named. just running mktemp (or an equivalent) with default parameters will do.

Neither do I, it's just the autoconf people claim, that on many systems mktemp supports -d, but doesn't support creating files, so creating a directory and a file inside it is portable.

[ossi]

think mc --mktemp as a fallback if configure detects no usable system command.

[zaytsev]

Replying to ossi:

think mc --mktemp as a fallback if configure detects no usable system command.

... like I suggested above - but then, I'd just do always it. Why complicate things?

[ossi]

oh, indeed you did.
i wouldn't want to make that the default, as it seems a bit heavy (a lot heavier than an actual mktemp execuable, at least), and thus an unreasonable penalty for reasonable systems. my first thought was actually to just have a separate mc_mktemp executable in libexec, which seems like a more reasonable option for doing it always.

fwiw, the idea to re-implement mktemp without -d as a shell function is a non-starter, because the limiting factor is access to O_EXCL from the shell. this is where the mkdir-based approach comes in, as that's always exclusive. but the clutter it creates is ugly.

anyway, i wonder whether we're even talking about a real problem at all - do we need to care for systems that don't come with mktemp (by which i mean that gnu coreutils cannot be deployed to them)? imo, it would be just fine to not have the wrapper function available there.

so i'm actually more inclined to think in the direction whether mktemp use shouldn't be already the fallback, while my $XDG_RUNTIME_DIR thing would be the default.

[andrew_b]

Ticket #4577 has been marked as a duplicate of this ticket.

[andrew_b]

Replying to zaytsev:

Could you please explain what problem are you trying to solve with this approach? I really want to understand your point.

I'm trying to provide the same behaviour regardless of how mc was invoked: from wrapper or not.

Modifying of mc-wrapper only is pointless: mc-wrapper creates the temp directory and mc makes another one that. We should get only one place where the temp directory is created.

1. mc-wrapper is used.
   1. Check if MC_TEMPDIR is already defined.
   2. If yes, use it as is. If directory exists, use it. If no, create it.
   3. If no, define it using TMPDIR with appended (pseudo)random part. Create directory.
   4. pwd file should be created in the MC_TEMPDIR (with unique name or not. It's discussable).
   5. Remove created directory.
2. mc-wrapper is unused. mc itself makes all below steps.
   1. Check if MC_TEMPDIR is already defined.
   2. If yes, use it as is. If directory exists, use it. If no, create it.
   3. If no, define it using TMPDIR with appended (pseudo)random part. Create directory.
   4. pwd file (a value of -P option) is used as is.
   5. Remove created directory.

[dmartina]

Replying to andrew_b:

I'm trying to provide the same behaviour regardless of how mc was invoked: from wrapper or not.

Modifying of mc-wrapper only is pointless: mc-wrapper creates the temp directory and mc makes another one that. We should get only one place where the temp directory is created.

Thanks Andrew, much better this way! It was tough to understand the whole changeset.

May we check some key points about it? It's an exercise to identify potential security risks.

1. Each running instance of mc is to have it's own temporary directory identified in MC_TEMPDIR. mc binary should reasonably check on start if it's being tricked with a suspiciously populated directory.
2. Whoever creates the directory is to set the environment variable and is responsible for deleting the directory. if mc-wrapper creates the directory it should finally delete it; if mc has to create it itself it should delete it too. Unreliable situations (such as non existent or non writable MC_TEMPDIR, MC_TEMPDIR without -p option present...) should led to mc aborting with an error message.
3. What may be the risks of an undeleted directory? It won't be difficult to cheat mc binary from a fake wrapper in order to access such content or place them in an undesirable place.

When I first read the changeset I found the meaning of MC_TEMPDIR confusing as could be seen above: MC_TEMPDIR was sometimes the container (as could be /tmp) but also could be the actual directory (/tmp/mc-XGFX23). Both systmp and tmpdir variables get tied to it in interface.c ¿? I don't like such reuse...

If we don't feel confident enough with these questions, we should fall back to separate the PWD file and the temporary directory. Me, I'm still not sure if it's worth changing the core program if it not were just for the benefit of learning from these debates.

[zaytsev]

Ticket #4578 has been marked as a duplicate of this ticket.

[zaytsev]

I'm trying to provide the same behaviour regardless of how mc was invoked: from wrapper or not.

But why would you want to do this? It seems to me extremely complicated: the wrappers become much more complex, and you have the same logic duplicated in mc code, and the wrappers. Is there any technical reason why would you want do to this?

My understanding of MC_TMPDIR is that it was introduced so that people can move big stuff mc vfs creates elsewhere without redefining TMPDIR. mc should do best-effort cleanup, but there is no problem creating multiple temporary files and directories there.

So what I want to do to solve the problem is to leave mc logic exactly like it was before, but in addition to that create a pwd file inside MC_TMPDIR with an unique name. This file will be cleaned up by the wrapper. If the wrapper is not used, everything will be like before.

I think this is easy and elegant, and solves the problem in a secure and reliable way.

Now that I'm looking at our code, we already use mktemp in tons of places and nobody complained. So I would just keep it as simple as possible. I can prepare a clean branch and test it. Is it ok with you? I didn't test what I wrote, it wasn't clean and was just supposed to demonstrate the idea.

[andrew_b]

Replying to zaytsev:

So what I want to do to solve the problem is to leave mc logic exactly like it was before, but in addition to that create a pwd file inside MC_TMPDIR with an unique name.

What MC_TMPDIR are you talking about? wrapper creates an MC_TMPDIR, mc creates another MC_TMPDIR. Those MC_TMPDIRs are different. Two MC_TMPDIRs are too much.

[zaytsev]

Ironically, I've just managed to debug and fix the long-standing test failures on macOS, which turned out to be caused by an incorrect path concatenation that has been present in the mc code pretty much forever.

FYI, macOS defines TMPDIR similarly to the following, and note the trailing slash "for safety" against badly written scripts:

/var/folders/3h/rv553qxs063dnqy2wlq1sswc0000gn/T/

I need more time to work on the actual problem with wrappers, make comments, and present another solution. I think it's a mess, and we don't want to make more of a mess.

[zaytsev]

So I've taken a closer look at MC_TMPDIR, and I think I understand Andrew better. What he is trying to accomplish is to streamline the meaning and handling of MC_TMPDIR, all while minimizing the amount of directories created in the process.

The current situation (changed after #4535, after our fake g_mkdtemp was discarded) is as follows:

1. Wrapper is not used
   1. mc uses MC_TMPDIR or TMPDIR as base directory (fallback to /tmp)
   2. mc creates a new subdirectory AND defines it as the new MC_TMPDIR for its process.
      1. used to create temporary files for mc
      2. used by VFS & menu scripts etc. to create more subdirectories
      3. when mc is started inside mc (so MC_TMPDIR is set), recursion is applied.
2. Wrapper is used
   1. wrapper always puts pwd file in TMPDIR or /tmp (MC_TMPDIR is ignored), and uses old scheme with $USER
   2. this file used to mostly end up in the directory of mc_tmpdir, because the creation schemes were roughly the same.
      1. is broken at least since 2012 if directory got a uid instead of name
      2. is broken since 2021 if MC_TMPDIR is set (changeset:faa195ae59fc4f2b105a6eea9caf7616cf159822)
      3. is completely broken since #4535

I think the only "easy" solution, without making everything very complex and duplicating the directory creation logic in mc and wrappers as has been suggested, is to keep the current logic in mc. So my proposal is:

2. Wrapper is used
   1. wrapper creates (and removes) a random file in MC_TMPDIR or TMPDIR or /tmp
   2. mc does any further handling of its own temporary directory as in (1).

The only "drawback" I see here is that we get a temporary file not in mc's own temporary directory, but right next to it. However, I think it's absolutely worth it, because it's as simple as possible, and there's no need to couple logic that's bound to break (see above - it's always broken to some degree, it seems).

I have prepared a new branch. Is this OK for everyone? Good to be tested.

Branch: 4575_mc-wrapper-2
Initial changeset:e2d96fa802abebf888dcc2cc938cfd06abca8eb0

[ossi]

yep, that's my suggestion as well.

the patch is ok, sans some nits:

• to make the code less repetitive, i'd assign a variable for the chosen temp dir in the if-cascade, and have one central mktemp call. in fact, it would be ok to have MC_TMPDIR be that variable, as it reproduces what mc will do anyway.
• the second point relates to a pre-existing issue: don't quote the rhs of plain assignments in sh scripts. the extra layer of quotes just makes things less legible, and may in fact impair portability (though shells which would get broken by it aren't relevant anymore).

also, the unsetting of the variables at the end of the scripts is entirely pointless, as they go out of scope anyway.

[dmartina]

Replying to zaytsev:

I have prepared a new branch. Is this OK for everyone? Good to be tested.

Branch: 4575_mc-wrapper-2
Initial changeset:e2d96fa802abebf888dcc2cc938cfd06abca8eb0

Failed. The temporary empty file is created in /tmp by mc-wrapper.sh with size 0, but it seems it's never written and the final cd has no effect. Seems to be the problem I mentioned before, with the open O_EXCL flag (file is to be created or truncated, but not both).

O_EXCL Ensure that this call creates the file: if this flag is
              specified in conjunction with O_CREAT, and pathname
              already exists, then open() fails with the error EEXIST.

Fixed and tested OK when patched with:

diff --git a/src/main.c b/src/main.c
index 803cec1..ebf1d91 100644
--- a/src/main.c
+++ b/src/main.c
@@ -510,5 +510,5 @@
         int last_wd_fd;
 
-        last_wd_fd = open (mc_args__last_wd_file, O_WRONLY | O_CREAT | O_TRUNC | O_EXCL,
+        last_wd_fd = open (mc_args__last_wd_file, O_WRONLY | O_CREAT | O_TRUNC,
                            S_IRUSR | S_IWUSR);
         if (last_wd_fd != -1)

[andrew_b]

mc-wrapper shouldn't create mc.pwd. mc makes it itself.

[zaytsev]

also, the unsetting of the variables at the end of the scripts is entirely pointless, as they go out of scope anyway.

Not with the current system how it's supposed to be used:

zaytsev@Yurys-MBP ~ % alias twr='. /Users/zaytsev/src/test-wrapper.sh'
zaytsev@Yurys-MBP ~ % twr
123
zaytsev@Yurys-MBP ~ % echo $TEST_VARIABLE
123
zaytsev@Yurys-MBP ~ % cat ~/src/test-wrapper.sh 

TEST_VARIABLE=123
echo $TEST_VARIABLE

[zaytsev]

mc-wrapper shouldn't create mc.pwd. mc makes it itself.

Oh, thank you, Andrew! This would explain why it fails because of O_EXCL :-(

However, it's not possible for mc to manage the file, if the filename is to be unpredictable and race-free. So we only have two options:

1. wrapper chooses name, but mc creates file (can result in races or need predictable name, both bad)
2. wrapper chooses name and manages file (create / delete), so mc needs to only know the name

I think Option 2 is the way to go. Would you agree to remove O_CREAT and O_EXCL in mc?

[ossi]

dmartina wrote:

Seems to be the problem I mentioned before, with the open O_EXCL flag

oh, indeed, that must go.
i suppose the presence of that is in fact what prompted me to go with XDG_RUNTIME_DIR.
option 2 it is.

zaytsev wrote:

Not with the current system how it's supposed to be used:

that's a rather unconventional way to do things.
but i also didn't want the overhead of a wrapping shell instance, so i have a shell function (which is probably inspired by an ancient suse or debian mc package).

[andrew_b]

Replying to zaytsev:

2. wrapper chooses name and manages file (create / delete), so mc needs to only know the name

I think Option 2 is the way to go. Would you agree to remove O_CREAT and O_EXCL in mc?

What about run mc without wrapper but with -P /path/to/mc.pwd.file. Who creates that file?

[dmartina]

Replying to andrew_b:

Replying to zaytsev:

2. wrapper chooses name and manages file (create / delete), so mc needs to only know the name

I think Option 2 is the way to go. Would you agree to remove O_CREAT and O_EXCL in mc?

What about run mc without wrapper but with -P /path/to/mc.pwd.file. Who creates that file?

This -P option is just for the wrapper, and the wrapper is just there to change directory back in the shell. If run with -P /path/to/mc.pwd.file and the file doesn't exist or is not writeable, it doesn't make sense to go on. An error message would be shown just like it would happen if the MC_TMPDIR provided is not writeable.

In case of any suspicious option, stop and error message.

[zaytsev]

What about run mc without wrapper but with -P /path/to/mc.pwd.file. Who creates that file?

My suggestion was to consider this a user responsibility to create (and delete) this file - just like the wrapper. This is why I suggested to remove O_CREAT. But you are right that this will for example break ossi's wrapper - and probably there isn't much value in forcing him to touch ... & ....

I don't have a very strong opinion on this part. Are you fine with removing just O_EXCL?

[andrew_b]

Replying to zaytsev:

Are you fine with removing just O_EXCL?

Yes.

[zaytsev]

I have pushed an update to the branch removing O_EXCL. I would prefer to keep wrappers as is to be similar and make minimal changes.

[andrew_b]

A have added a fixup commit. Unfortunately, *INDENT-ON* was missed in this file. Fixed in the 4572_cleanup branch: [d10440f49f80ca1adc18cd8fbd0096c74279a772].

[zaytsev]

Merged to master as changeset:7a3a763f0ea07a825ca2af4642e31f9e358a9fd0 . Thank you for your help, Andrew!