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.

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

Temporary directory name conflicts when runnig inside containers with shared /tmp #4535

Closed
mc-butler opened this issue May 8, 2024 · 15 comments
Assignees
Labels
area: core Issues not related to a specific subsystem prio: medium Has the potential to affect progress
Milestone

Comments

@mc-butler
Copy link

Important

This issue was migrated from Trac:

Origin https://midnight-commander.org/ticket/4535
Reporter eugenesan (eugenesan@….com)

When running inside Distrobox container temporary directory name conflicts with mc instance running on the host machine which has different permissions:

root@ubuntu:~# mc
Directory /tmp/mc-root is not owned by you
Temporary files will be created in /tmp
Press any key to continue...

Trivial fix I came up with is to add process ID to the directory name.
Also, that might be useful when running multiple instances on the host.

Please find the fix attached to the tocket

Note

Original attachments:

@mc-butler
Copy link
Author

Changed by eugenesan (eugenesan@….com) on May 8, 2024 at 20:45 UTC

Add process ID to the temporary directory name

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on May 9, 2024 at 7:32 UTC (comment 1)

  • Status changed from new to accepted
  • Owner set to andrew_b
  • Milestone changed from Future Releases to 4.8.32
  • Branch state changed from no branch to on review

Branch: 4535_tmpdir_name
[2fda4c6]

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on May 12, 2024 at 16:16 UTC (comment 2)

Hi Andrew, I'm not sure what was the original reasoning behind making the directories per-user, but now that you're going into the direction of making them unique (per process), isn't it much better to use the POSIX mkdtemp API (or rather get_tmp_dir from glib) and remove all our strange black magic altogether? There was already CVE-2004-0231 in the past due to homegrown adventurous handling of temporary directories...

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on May 12, 2024 at 17:38 UTC (comment 2.3)

Replying to zaytsev:

isn't it much better to use the POSIX mkdtemp API

Probably yes.

(or rather get_tmp_dir from glib)

g_get_tmp_dir uses $TMPDIR. mc tries to use $MC_TMPDIR if it's set and $TMPDIR otherwise.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on May 12, 2024 at 17:39 UTC (comment 4)

  • Branch state changed from on review to on rework

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on May 12, 2024 at 18:06 UTC (comment 5)

g_get_tmp_dir uses $TMPDIR. mc tries to use $MC_TMPDIR if it's set and $TMPDIR otherwise.

Well, there seems to be another more flexible API: g_dir_make_tmp - maybe you could use that instead.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on May 12, 2024 at 18:32 UTC (comment 5.6)

Replying to zaytsev:

g_get_tmp_dir uses $TMPDIR. mc tries to use $MC_TMPDIR if it's set and $TMPDIR otherwise.

Well, there seems to be another more flexible API: g_dir_make_tmp - maybe you could use that instead.

It doesn't help us: g_dir_make_tmp calls g_get_tmp_dir.
I think we should keep the $MC_TMPDIR usage, so we have to write a code for temporary directory name. Something like

    g_snprintf (buffer, sizeof (buffer), "%s/mc-XXXXXX", sys_tmp);
    tmpdir = g_mkdtemp (buffer);
    if (tmpdir != NULL)
        g_setenv ("MC_TMPDIR", tmpdir, TRUE);

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on May 13, 2024 at 9:14 UTC (comment 7)

You are right, I agree!

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on May 13, 2024 at 18:51 UTC (comment 8)

comment:1 updated.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on May 13, 2024 at 18:55 UTC (comment 9)

  • Branch state changed from on rework to on review

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on May 14, 2024 at 13:24 UTC (comment 10)

  • Branch state changed from on review to approved
  • Votes set to zyv

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on May 14, 2024 at 18:53 UTC (comment 11)

  • Resolution set to fixed
  • Status changed from accepted to testing
  • Branch state changed from approved to merged
  • Votes changed from zyv to committed-master

Merged to master: [794823b].

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on May 14, 2024 at 18:55 UTC (comment 12)

  • Status changed from testing to closed

A typo in the commit message: s/drirectory/directory/ :-(

@mc-butler
Copy link
Author

Changed by anton_kg (anton.bugs@….com) on Oct 7, 2024 at 7:33 UTC (comment 13)

this commit broke /usr/libexec/mc/mc-wrapper.sh script probably. It expects /tmp/mc-$MC_USER
FYI.

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Oct 7, 2024 at 7:49 UTC (comment 14)

This has been fixed in #4575. FYI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: core Issues not related to a specific subsystem prio: medium Has the potential to affect progress
Development

No branches or pull requests

2 participants