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

reset_machine_id: ask for forgiveness, not permission #67

Merged
merged 1 commit into from Mar 1, 2017

Conversation

fsateler
Copy link
Member

Avoid TOCTOU by always deleting and handling the FileNotFoundError

mkosi Outdated
except FileNotFoundError:
pass

if has_dbus_machine_id:
Copy link
Member

Choose a reason for hiding this comment

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

OK. But why not use try..else, and get rid of the boolean?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what the else would be for. I could lump the symlink into the same try block though..

Copy link
Member

Choose a reason for hiding this comment

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

try:
  os.unlink(dbus_machine_id)
except FileNotFoundError:
  pass
else:
  os.symlink('../../../etc/machine-id', dbus_machine_id)

Copy link
Member

Choose a reason for hiding this comment

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

i figure that was my bad suggestion. I forgot that you can do the "else" syntax when catching exceptions...

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, what I want is to create the symlink only if the file already existed. I do not want to create it if we didn't remove it. That would imply creating the /var/lib/dbus dir if it doesn't exist, which I don't think we should be doing. That leaves us with either this PR or:

try:
  os.unlink(dbus_machine_id)
  os.symlink('../../../etc/machine-id', dbus_machine_id)
except FileNotFoundError:
  pass

Copy link
Member

Choose a reason for hiding this comment

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

My code does what you described.

Copy link
Member

Choose a reason for hiding this comment

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

@fsateler no. the "else" branch of a "try" statement is only executed when no exception is thrown. The difference between the version you are now proposing and @keszybz's is that in your version "os.symlink()" throwing FileNotFoundError is ignored, while in @keszybz's version it is not. And I think @keszybz' version is right here: we should only ignore the errors we really want to ignore, and that's the ones from os.unlink().

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, its clear I did not know how the else clause works. Force pushed a new version using that.

Thanks for the patience!

Avoid TOCTOU by always deleting and handling the FileNotFoundError
@poettering poettering merged commit 3657e9b into systemd:master Mar 1, 2017
vbatts pushed a commit to vbatts/systemd-mkosi that referenced this pull request Aug 24, 2017
add changable compression algorithms via the new --compression= switch, support gzip
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants