Skip to content

Suppress FileNotFoundError when reading status #1298

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

Merged
blackboxsw merged 3 commits into from
Apr 8, 2022
Merged

Suppress FileNotFoundError when reading status #1298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6668619
Suppress FileNotFoundError when reading status
sparkiegeek Feb 24, 2022
f6beb15
Avoid creation of race in the first place
sparkiegeek Mar 22, 2022
f9761f6
review comments
TheRealFalcon Apr 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion cloudinit/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1887,7 +1887,12 @@ def is_link(path):
def sym_link(source, link, force=False):
LOG.debug("Creating symbolic link from %r => %r", link, source)
if force and os.path.lexists(link):
del_file(link)
# Provide atomic update of symlink to avoid races with status --wait
# LP: #1962150
tmp_link = os.path.join(os.path.dirname(link), "tmp" + rand_str(8))
os.symlink(source, tmp_link)
Copy link
Copy Markdown
Contributor Author

@sparkiegeek sparkiegeek Apr 7, 2022

Choose a reason for hiding this comment

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

what guarantees do you have that tmp_link doesn't exist on the filesystem?

Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon Apr 7, 2022

Choose a reason for hiding this comment

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

We have none, but the rand_str(8) makes it pretty unlikely. I suppose we could switch to a uuid if we want to be absolutely sure.

Copy link
Copy Markdown
Contributor Author

@sparkiegeek sparkiegeek Apr 7, 2022

Choose a reason for hiding this comment

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

am just thinking about the Bad Guy ™️ who pre-creates this and then gets you to use it

Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon Apr 7, 2022

Choose a reason for hiding this comment

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

Sorry, pre-creates what?

Copy link
Copy Markdown
Collaborator

@blackboxsw blackboxsw Apr 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Scenario where bad_guy brute force creates all tmp links before we get there results in a traceback in cloud-init which will stop cloud-init in noisily with a Traceback instead of silently allowing malicious data injection (even though our use-cases for symlink don't really lead to anything malicious at the moment).

python3 -c 'import os, pathlib; pathlib.Path("/badguyfile1").touch(); os.symlink("/badguyfile1", "badguytmpfile"); os.symlink("/run/cloud-initfile", "badguytmpfile")'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
FileExistsError: [Errno 17] File exists: '/run/cloud-initfile' -> 'badguytmpfile'

Copy link
Copy Markdown
Collaborator

@blackboxsw blackboxsw Apr 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I suppose just the go the extra mile and avoid the traceback in the event that we happen to have a collision:

diff --git a/cloudinit/util.py b/cloudinit/util.py
index ae3311e3e..df841f210 100644
--- a/cloudinit/util.py
+++ b/cloudinit/util.py
@@ -1890,6 +1890,7 @@ def sym_link(source, link, force=False):
         # Provide atomic update of symlink to avoid races with status --wait
         # LP: #1962150
         tmp_link = os.path.join(os.path.dirname(link), "tmp" + rand_str(8))
+        del_file(tmp_link)
         os.symlink(source, tmp_link)
         os.replace(tmp_link, link)
         return

That would guarantee we never see a Traceback because we clean up our tmp_link before we even attempt to create it,. del_file handles the case where theENOENT where tmp_link doesn't exist so we won't error in this case and we won't collide with "Bad Guy" or any previous invocations that somehow broke before renaming the symlink.

Copy link
Copy Markdown
Contributor Author

@sparkiegeek sparkiegeek Apr 8, 2022

Choose a reason for hiding this comment

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

If you're fine with the traceback (I don't mind, up to you folks) then think actually omitting the del_file is cleaner since you can't guarantee that Bad Guy:tm: won't create the symlink between us deleting it and then creating it (and also has a higher chance of discovering the destination because of the interaction with the FS).

In other words, let's land this! Thanks for helping to get it over the line

Copy link
Copy Markdown
Collaborator

@blackboxsw blackboxsw Apr 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

+1 awesome thanks @sparkiegeek I agree and will add the del_file. and land this. Total mistread of your perfect English. Will leave out the del_file and we can land it, I don't expect we will hit this trace given the very limited scope and use of sym_link force throughout cloud-init, and the impact of a traceback in current use-cases is minimal to running cloud-init operations.

os.replace(tmp_link, link)
return
os.symlink(source, link)


Expand Down
5 changes: 4 additions & 1 deletion tests/unittests/test_util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,13 +376,16 @@ def test_sym_link_source_exists(self):
tmpd = self.tmp_dir()
link = self.tmp_path("link", tmpd)
target = self.tmp_path("target", tmpd)
target2 = self.tmp_path("target2", tmpd)
util.write_file(target, "hello")
util.write_file(target2, "hello2")

util.sym_link(target, link)
self.assertTrue(os.path.exists(link))

util.sym_link(target, link, force=True)
util.sym_link(target2, link, force=True)
self.assertTrue(os.path.exists(link))
self.assertEqual("hello2", util.load_file(link))

def test_sym_link_dangling_link(self):
tmpd = self.tmp_dir()
Expand Down