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

(FOR REVIEW) (PUP-3521) Fix Puppet's exception swallowing #3302

Merged
merged 1 commit into from Dec 15, 2014

Conversation

@MSLilah
Copy link
Contributor

MSLilah commented Nov 12, 2014

FOR REVIEW

This PR attempts to fix Puppet's exception swallowing. It does this by changing places where Puppet rescues and swallows an exception to have a more granular rescue clause.

This PR is going to need some review to ensure these changes are sane.

@cprice404

This comment has been minimized.

Copy link
Contributor

cprice404 commented Nov 12, 2014

@joshcooper @pcarlisle we are going to do some reviews of this, but before it gets merged I'd really like to get you guys' input.

@MSLilah

This comment has been minimized.

Copy link
Contributor Author

MSLilah commented Nov 12, 2014

I didn't touch any of the error handling in install.rb, as I wasn't sure if that was something I should change for this ticket or not. I can easily go fix those as well though if deemed necessary.

@hlindberg

This comment has been minimized.

Copy link
Contributor

hlindberg commented Nov 12, 2014

oh, this is potentially nasty.. not all exceptions that needs to be caught/logged/whatever are StandardErrors. Notably ScriptErrors (raised by some yaml implementations when the yaml is faulty, and raised by ruby when code cannot be loaded). Also, it is not safe to assume that all 3d party code raises exceptions that are StandardError - they could raise errors derived from Exception directly. (Yeah, It is a total mess)

@@ -178,7 +178,7 @@ def parse_log_level(puppet_path,cmdline_debug)
loglevel = :notice
log_err("Failed to determine loglevel, defaulting to #{loglevel}")
end
rescue Exception => e
rescue StandardError => e

This comment has been minimized.

Copy link
@cprice404

cprice404 Nov 12, 2014

Contributor

@joshcooper is this file used in the windows installer? Whatever it is for, it won't be running in the server, so we don't necessarily need to changes these ones.

This comment has been minimized.

Copy link
@joshcooper

joshcooper Nov 14, 2014

Member

@cprice404 @fpringvaldsen This code runs the windows service. It is a ruby program started by the Service Control Manager. It doesn't load any puppet code. Instead it sits in a loop executing puppet agent --onetime every runinterval seconds. So to your question @cprice404, it is not necessary to change (from puppet's perspective).

@@ -110,7 +110,7 @@ def with_client
@client = client_class.new
rescue SystemExit,NoMemoryError
raise
rescue Exception => detail
rescue StandardError => detail

This comment has been minimized.

Copy link
@cprice404

cprice404 Nov 12, 2014

Contributor

This file shouldn't be getting run on the server either, but these changes still seem sane to me.

@hlindberg

This comment has been minimized.

rescue Exception
# Damn, but I hate this: we just ignore errors here, no matter what
# class they are. Meh.
rescue NameError

This comment has been minimized.

Copy link
@cprice404

cprice404 Nov 12, 2014

Contributor

How'd you decide on NameError here?

This comment has been minimized.

Copy link
@MSLilah

MSLilah Nov 12, 2014

Author Contributor

I tried drilling down through the code and running some things in the irb, and the only error I could find/manage to cause was a NameError. Can definitely go to StandardError if NameError is too specific though.

This comment has been minimized.

Copy link
@pcarlisle

pcarlisle Nov 13, 2014

Contributor

seems like a low risk part of the code, but I'd feel better about narrowing it down if I actually understood the reason it's written this way

This comment has been minimized.

Copy link
@joshcooper

joshcooper Dec 3, 2014

Member

I think it was written that way to ensure one poorly written application didn't cause puppet help to fall ungracefully. For example if you install a module that defines a new application, and now suddenly puppet help blows up. Previously, if there was a syntax error in the application, we would have silently swallowed the error (which seems bad), and now we might blow up...

It might be better to keep this as StandardError and if that happens specify an error message for the application summary like we do for face-based applications:

          result << [ "! #{appname}", "! Subcommand unavailable due to error. Check error logs." ]

This comment has been minimized.

Copy link
@MSLilah

MSLilah Dec 4, 2014

Author Contributor

@joshcooper I've changed this as per your suggestions

@puppetcla

This comment has been minimized.

Copy link

puppetcla commented Nov 13, 2014

CLA signed by all contributors.

@@ -47,7 +47,7 @@ def run(client_options = {})
lock { client.run(client_args) }
rescue SystemExit,NoMemoryError

This comment has been minimized.

Copy link
@pcarlisle

pcarlisle Nov 13, 2014

Contributor

This rescue can be removed now

@@ -110,7 +110,7 @@ def with_client
@client = client_class.new
rescue SystemExit,NoMemoryError

This comment has been minimized.

Copy link
@pcarlisle

pcarlisle Nov 13, 2014

Contributor

remove this

@@ -178,7 +178,7 @@ def run_internal(options)
end
rescue SystemExit,NoMemoryError

This comment has been minimized.

Copy link
@pcarlisle

pcarlisle Nov 13, 2014

Contributor

remove

@@ -297,7 +297,7 @@ def retrieve_new_catalog(query_options)
result
rescue SystemExit,NoMemoryError

This comment has been minimized.

Copy link
@pcarlisle

pcarlisle Nov 13, 2014

Contributor

remove

@@ -369,7 +369,7 @@ def need_to_run?
end
rescue SystemExit,NoMemoryError

This comment has been minimized.

Copy link
@pcarlisle

pcarlisle Nov 13, 2014

Contributor

remove

@@ -501,7 +501,7 @@ def do_execute_changes
end
rescue SystemExit,NoMemoryError

This comment has been minimized.

Copy link
@pcarlisle

pcarlisle Nov 13, 2014

Contributor

remove

@@ -323,7 +323,7 @@ def wait_for_cert(time)
return if certificate
rescue SystemExit,NoMemoryError

This comment has been minimized.

Copy link
@pcarlisle

pcarlisle Nov 13, 2014

Contributor

remove

@@ -532,7 +532,7 @@ def width #:nodoc:
x = Curses::cols
Curses::close_screen
x
rescue Exception
rescue StandardError

This comment has been minimized.

Copy link
@pcarlisle

pcarlisle Nov 13, 2014

Contributor

wow nice vendored code too

This comment has been minimized.

Copy link
@pcarlisle

pcarlisle Nov 13, 2014

Contributor

this seems unsafe, this won't catch errors in requiring curses which I assume is inside the block for a reason
you could add scripterror or possibly not change this one since it's vendored code

@@ -17,7 +17,7 @@ def add(name, options = {})
if block_given?
begin
result = yield
rescue Exception => detail
rescue StandardError => detail

This comment has been minimized.

Copy link
@pcarlisle

pcarlisle Nov 13, 2014

Contributor

This needs to also rescue ScriptError

@@ -88,7 +88,7 @@ def load_library(lib, name)
require lib
rescue SystemExit,NoMemoryError
raise
rescue Exception
rescue LoadError

This comment has been minimized.

Copy link
@pcarlisle

pcarlisle Nov 13, 2014

Contributor

This should be ScriptError instead of LoadError

@@ -88,7 +88,7 @@ def load_library(lib, name)
require lib
rescue SystemExit,NoMemoryError

This comment has been minimized.

Copy link
@pcarlisle

pcarlisle Nov 13, 2014

Contributor

remove

@@ -3,7 +3,7 @@ module Puppet::Util::MonkeyPatches

begin
Process.maxgroups = 1024
rescue Exception
rescue NotImplementedError

This comment has been minimized.

Copy link
@pcarlisle

pcarlisle Nov 13, 2014

Contributor

you checked that this is what actually happens on unsupported platforms?

This comment has been minimized.

Copy link
@MSLilah

MSLilah Nov 13, 2014

Author Contributor

Yup. I tested it on JRuby, which is one of the unsupported platforms.

This comment has been minimized.

Copy link
@joshcooper

joshcooper Nov 14, 2014

Member

Also windows does the same:

irb(main):001:0> Process.maxgroups = 1024
NotImplementedError: maxgroups=() function is unimplemented on this machine
@MSLilah MSLilah force-pushed the MSLilah:PUP-3521-error-swallowing branch from 6eb7b39 to 75c99b3 Nov 13, 2014
@MSLilah

This comment has been minimized.

Copy link
Contributor Author

MSLilah commented Nov 13, 2014

@pcarlisle I've addressed your comments.

@hlindberg I think I've addressed some of your concerns while responding to @pcarlisle's feedback, let me know if you still have concerns about this PR.

@hlindberg

This comment has been minimized.

Copy link
Contributor

hlindberg commented Nov 14, 2014

@fpringvaldsen I think all my general comments were better described by @pcarlisle's more detailed dito.

@@ -17,7 +17,7 @@ def add(name, options = {})
if block_given?
begin
result = yield
rescue Exception => detail
rescue StandardError,ScriptError => detail

This comment has been minimized.

Copy link
@joshcooper

joshcooper Nov 14, 2014

Member

A module author can add new features of the form:

Puppet.features.add(:myfeature) do
   ...
end

and raise arbitrary exceptions in the process. Do we need to catch more than StandardError & ScriptError? On ruby 1.9 the top-level exceptions are:

BasicObject
  Exception
    MonitorMixin::ConditionVariable::Timeout
    NoMemoryError
    ScriptError
    SecurityError
    SignalException
    StandardError
    SystemExit
    SystemStackError
    fatal

I have no idea what SecurityException and MonitorMixin::ConditionVariable::Timeout are used for... I'm not sure if ruby 2 adds more...

This comment has been minimized.

Copy link
@MSLilah

MSLilah Nov 14, 2014

Author Contributor

Hmm...it sounds like I should revert this change unless @hlindberg, @cprice404, or @pcarlisle have any objections.

This comment has been minimized.

Copy link
@cprice404

cprice404 Nov 14, 2014

Contributor

@joshcooper is probably a better judge of this than I am, but I'd still prefer to do something more targeted than just catching 'Exception' here. e.g. if a user writes a feature that simply allocates memory in a loop until we get an OOM, I don't want there to be a chance of it getting swallowed here.

I'd be fine with broadening it beyond just Standard and Script if we think it's necessary, but would probably vote against just leaving it as Exception.

This comment has been minimized.

Copy link
@MSLilah

MSLilah Nov 14, 2014

Author Contributor

The big problem that seems to jump out to me here is the possibility of someone writing a custom feature and then throwing a custom exception that inherits from Exception, in which case that won't be caught if we don't do rescue Exception. Not sure if that's something we want to allow or not. @cprice404 I also think you have a valid point about exceptions such as OOM that really shouldn't be swallowed, I think it's important for those to bubble up to the top.

This comment has been minimized.

Copy link
@joshcooper

joshcooper Nov 16, 2014

Member

Here's a list of all features from forge modules (as of 9-30-2014). https://gist.github.com/joshcooper/06892d3a05660f436c7c I think it would be fine to rescue StandardError, ScriptError and call it a day.

This comment has been minimized.

Copy link
@MSLilah

MSLilah Nov 17, 2014

Author Contributor

Alright, then I'll leave it as-is.

service.log_exception(e)
end
end
@run_thread.join

rescue Exception => e
rescue StandardError => e

This comment has been minimized.

Copy link
@joshcooper

joshcooper Nov 14, 2014

Member

I am a litle concerned that the SCM might send a SignalException, and that would cause the service to terminate without logging why (aside from the 'Service stopped' message below). I'm thinking we should not change daemon.rb for now, and file a PUP ticket to have us investigate.

This comment has been minimized.

Copy link
@MSLilah

MSLilah Nov 14, 2014

Author Contributor

That sounds like a good idea, I'll revert the changes to daemon.rb.

rescue SystemExit,NoMemoryError
raise
rescue Exception => detail
rescue StandardError => detail

This comment has been minimized.

Copy link
@joshcooper

joshcooper Nov 14, 2014

Member

This can be shortened to just rescue => detail or we can keep the longer explicit form. I'm fine with explicit, and less magic.

This comment has been minimized.

Copy link
@cprice404

cprice404 Nov 14, 2014

Contributor

+1 for explicit from me :)

This comment has been minimized.

Copy link
@MSLilah

MSLilah Nov 14, 2014

Author Contributor

I also think it would be good to keep it explicit, so I'll leave it for now.

@MSLilah MSLilah force-pushed the MSLilah:PUP-3521-error-swallowing branch from 75c99b3 to 419738f Nov 14, 2014
@MSLilah

This comment has been minimized.

Copy link
Contributor Author

MSLilah commented Nov 14, 2014

@joshcooper I've addressed your feedback minus the comment about the rescue in feature.rb, as it seems the discussion on that comment is still ongoing.

@MSLilah

This comment has been minimized.

Copy link
Contributor Author

MSLilah commented Nov 17, 2014

I believe all the comments on this PR have now been addressed.

@nwolfe nwolfe added the PL label Nov 20, 2014
Fix Puppet's exception swallowing by finding places where an
Exception is rescued and swallowed and making their rescues
more granular.
@MSLilah MSLilah force-pushed the MSLilah:PUP-3521-error-swallowing branch from 419738f to eccd3b4 Dec 4, 2014
cprice404 pushed a commit that referenced this pull request Dec 15, 2014
(FOR REVIEW) (PUP-3521) Fix Puppet's exception swallowing
@cprice404 cprice404 merged commit d7f7bb4 into puppetlabs:master Dec 15, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@MSLilah MSLilah deleted the MSLilah:PUP-3521-error-swallowing branch Jan 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.