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

.target.tmpdir - honor the target root (bsc#1199840) #159

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

Conversation

lslezak
Copy link
Member

@lslezak lslezak commented Jul 25, 2022

Problem

The .target.tmpdir agent creates the temporary directory in the current system root.

That makes troubles when the SCR is chrooted. In that case the other agents (.target.string or target.bash) use the chroot and the temporary directory is not reachable from there as it is outside the chroot.

This causes problems when running YaST in a container.

Solution

The solution required to move the code from the constructor and create the temporary directory at the first access. The reason is that the root() call returns the / path in the constructor, the correct target root is set later.

Notes

  • Now the code checks whether the /tmp directory exists in the target root, this might be useful in testing when you use an empty chroot.
  • The lazy creating has a side effect that the temporary directory is not created when not needed.

Testing

Tested manually with this small testing Ruby script:

#! /usr/bin/env ruby

require "tmpdir"
require "yast"

def print_scr_tmpdir
  puts "SCR root: #{Yast::WFM.scr_root}"
  scr_tmpdir = Yast::SCR.Read(Yast::path(".target.tmpdir"))
  puts ".target.tmpdir: #{scr_tmpdir}"
  dir = File.join(Yast::WFM.scr_root, scr_tmpdir)
  puts "#{dir} exists: #{File.exist?(dir)}"
  puts ".target.bash_output(\"ls -l #{scr_tmpdir}\"): #{Yast::SCR.Execute(Yast::path(".target.bash_output"), "ls -l #{scr_tmpdir}").inspect}"
  puts
end

# print ".target.tmpdir" for standard root (/)
print_scr_tmpdir

# print ".target.tmpdir" for a chroot
handle = Yast::WFM.SCROpen("chroot=#{ENV["YAST_SCR_TARGET"]}:scr", false)
Yast::WFM.SCRSetDefault(handle)

print_scr_tmpdir

This code checks the behavior in both cases, running in standard root and running in a chroot.

Results

With the original code it failed when running in a container with this error:

# ./tmpdir_test.rb 
SCR root: /
.target.tmpdir: /tmp/YaST2-09483-2vNEUh
/tmp/YaST2-09483-2vNEUh exists: true
.target.bash_output("ls -l /tmp/YaST2-09483-2vNEUh"): {"exit"=>0, "stderr"=>"", "stdout"=>"total 0\n-rw-r--r-- 1 root root 0 Jul 22 11:35 stderr\n-rw-r--r-- 1 root root 0 Jul 22 11:35 stdout\n"}

SCR root: /mnt
.target.tmpdir: /tmp/YaST2-09483-cyjDYe
/mnt/tmp/YaST2-09483-cyjDYe exists: false
.target.bash_output("ls -l /tmp/YaST2-09483-cyjDYe"): {"exit"=>2, "stderr"=>"ls: cannot access '/tmp/YaST2-09483-cyjDYe': No such file or directory\n", "stdout"=>""}

After fixing .target.tmpdir it works as expected:

./tmpdir_test.rb 
SCR root: /
.target.tmpdir: /tmp/YaST2-10185-UnHBqm
/tmp/YaST2-10185-UnHBqm exists: true
.target.bash_output("ls -l /tmp/YaST2-10185-UnHBqm"): {"exit"=>0, "stderr"=>"", "stdout"=>"total 0\n-rw-r--r-- 1 root root 0 Jul 22 11:35 stderr\n-rw-r--r-- 1 root root 0 Jul 22 11:35 stdout\n"}

SCR root: /mnt
.target.tmpdir: /tmp/YaST2-10185-ERPiTk
/mnt/tmp/YaST2-10185-ERPiTk exists: true
.target.bash_output("ls -l /tmp/YaST2-10185-ERPiTk"): {"exit"=>0, "stderr"=>"", "stdout"=>"total 0\n-rw-r--r-- 1 root root 0 Jul 22 13:35 stderr\n-rw-r--r-- 1 root root 0 Jul 22 13:35 stdout\n"}

GitHub Action

The GitHub Action is failing because of missing glibc-locale package in the image. Fixed with yast/ci-ruby-container#20.

- Create the temporary directory in the target root,
  not in the system root. Allow using it from other
  .target paths (like .bash or .string)
- The temporary directory is now created only when needed.
- 4.5.3
@@ -332,7 +359,8 @@ SystemAgent::Read (const YCPPath& path, const YCPValue& arg, const YCPValue&)
* @example Read (.target.tmpdir) -> "/some/temp/dir"
*/

return YCPString (tempdir);
// skip the chroot prefix, return the location relative to the current root
return YCPString (tempdir().substr(strlen(root())));
Copy link
Member

Choose a reason for hiding this comment

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

I think this is wrong :)

It is fine when tempdir is /mnt/tmp and root is /mnt
It is fine when tempdir is /tmp and root is empty
But what if root is /?

AFAIK SCRAgent::root is not clear about "" vs "/"

Yet your test shows it works... ... ah, because tempdir() will return two leading slashes. I think it deserves a comment in the code.

Copy link
Member

Choose a reason for hiding this comment

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

for safety I would also change code to something like (pseudocode) YCPString ((root() == "/" || root() == "") ? tempdir() : tempdir().substr(strlen(root())))

* Returns the temporary directory path, it creates the directory at the first
* call. The directory is automatically deleted in the destructor.
*
* @return string The returned path includes the current root (chroot) prefix.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @return string The returned path includes the current root (chroot) prefix.
* @return string The returned path includes the current root (chroot) prefix.
For example "//tmp"

Copy link
Member

@jreidinger jreidinger left a comment

Choose a reason for hiding this comment

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

I agree with mvidner to be on safe side with having always absolute root

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants