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

Jump to bottom

(PUP-9997) Avoid Dir.chdir #9387

Merged
merged 4 commits into from
Jul 1, 2024
Merged

(PUP-9997) Avoid Dir.chdir #9387

merged 4 commits into from
Jul 1, 2024

Commits on Jun 28, 2024

  1. (PUP-9997) Don't Dir.chdir when installing modules 

    When the Dir.chdir block ends we may not have permission to switch our cwd back
to where we started. So keep the cwd as is and pass the dest dir to the
tar/find/chown commands. Since we're passing arbitrary destination directories,
escape it.
    @joshcooper
    joshcooper committed Jun 28, 2024
    Configuration menu
    Copy the full SHA
    0df08e9 View commit details
    Browse the repository at this point in the history

  2. (PUP-9997) Pass cwd to execute method 

    Dir.chdir is problematic because it affects all threads in the current process
and if puppet is started with a current working directory it doesn't have
traverse/execute permission to, then it won't be able to restore the cwd at the
end of the Dir.chdir block.

Puppet supports three execution implementations: posix, windows and stub. The
first two already support the `cwd` option. Puppetserver injects its stub
implementation and it recently added support for `cwd`, see SERVER-3051.
    @joshcooper
    joshcooper committed Jun 28, 2024
    Configuration menu
    Copy the full SHA
    ee46e78 View commit details
    Browse the repository at this point in the history

  3. (PUP-9997) Remove dead code 

    The Puppet::Util::Reference.pdf method has been broken since 2012 due to
commit f0c9995, because it was calling
Puppet::Util.replace_file with 1 argumentm, but it takes 2, resulting
in:

    $ puppet doc --mode pdf --reference type
    creating pdf
    Error: Could not run: wrong number of arguments (given 1, expected 2)

So delete the code and remove `pdf` as a valid reference doc mode.
    @joshcooper
    joshcooper committed Jun 28, 2024
    Configuration menu
    Copy the full SHA
    cd0f3ce View commit details
    Browse the repository at this point in the history

  4. (PUP-9997) Remove call to Dir.chdir when managing symlinks 

    Symlinks can refer to their target using a relative or absolute path. If a
relative path is used, then it's assumed to be relative to the symlink's parent
directory. For example, /tmp/link refers to /tmp/target using a relative path:

    $ ls -l /tmp/link
    lrwxrwxrwx 1 josh 6 Jun 27 23:26 /tmp/link -> target

In commit 02f91fc, symlinks were merged into the file (aka pfile) type. The
`ensure` property was modified to validate whether the target existed or was a
directory[1] In order for relative symlinks to be validated, it was necessary
to call `Dir.chdir`.

Later the call to `Dir.chdir` was moved to `target.rb`[2], and then the
`FileTest.exists?(target)` check was deleted[3].

This commit removes the call to `Dir.chdir`. This means we no longer change our
cwd prior to dropping privileges and creating the symlink. This should be ok
because none of the code within the Dir.chdir block is sensitive to cwd. Also
when we reach this block of code, we're committed to creating the symlink given
an absolute `@resource[:path]`, so we don't need to be concerned about TOCTOU
race conditions.

[1] https://github.com/puppetlabs/puppet/blob/02f91fcd55aefe63a11a29c5608c0738867b8130/lib/puppet/type/pfile/ensure.rb#L76-L86
[2] https://github.com/puppetlabs/puppet/blob/2cd67ad843409a49747748a1278e5ef788dd97bd/lib/puppet/type/pfile/target.rb#L41-L45
[3] puppetlabs@662fcaf#diff-03f0464c0f42c4b979f6150ec2f7e8043ec964fe4d401cd0cd57a82f43a856ba
    @joshcooper
    joshcooper committed Jun 28, 2024
    Configuration menu
    Copy the full SHA
    3c1cac6 View commit details
    Browse the repository at this point in the history