darcs patch: fix potential hole in userCode.

Fri Oct 12 11:02:53 EDT 2007 David Roundy

On Friday 12 October 2007 10:03:52 David Roundy wrote:
Fri Oct 12 11:02:53 EDT 2007 David Roundy
* fix potential hole in userCode. This makes userCode catch errors even when the user does something like (return undefined).
Applied.

On Fri, Oct 12, 2007 at 10:11:13AM -0500, Spencer Janssen wrote:
On Friday 12 October 2007 10:03:52 David Roundy wrote:
Fri Oct 12 11:02:53 EDT 2007 David Roundy
* fix potential hole in userCode. This makes userCode catch errors even when the user does something like (return undefined). Applied.
I was thinking that we might get a little more benefit from adding just a tad of strictness to catchX itself. It could call seq on the return value to catch at least the grossest errors that might be hidden in lazy values. That still wouldn't obsolete this change to userCode, since the advantage when we've got a () return value is that we can safely ignore it and keep any changes that might have been made. -- David Roundy Department of Physics Oregon State University

droundy:
Oe Fri, Oct 12, 2007 at 10:11:13AM -0500, Spencer Janssen wrote:
On Friday 12 October 2007 10:03:52 David Roundy wrote:
Fri Oct 12 11:02:53 EDT 2007 David Roundy
* fix potential hole in userCode. This makes userCode catch errors even when the user does something like (return undefined). Applied.
I was thinking that we might get a little more benefit from adding just a tad of strictness to catchX itself. It could call seq on the return value to catch at least the grossest errors that might be hidden in lazy values. That still wouldn't obsolete this change to userCode, since the advantage when we've got a () return value is that we can safely ignore it and keep any changes that might have been made.
Fwiw, here's the exception catching code from lambdabot. This has to evaluate completely untrusted code, of course: case s of Left e -> mapM_ putStrLn e Right v -> Control.Exception.catch (putStrLn v) (\e -> Control.Exception.handle (const $ putStrLn "Exception") $ do e' <- Control.Exception.evaluate e putStrLn $ "Exception: " ++ take 1024 (show e')) Right means there was no compile error. So we then show the value, forcing it. Note that it can throw an exception whose thrown value is an exception. 'evaluate' takes care of some of the work for us. -- Don

On Fri, Oct 12, 2007 at 10:32:48AM -0700, Don Stewart wrote:
droundy:
Oe Fri, Oct 12, 2007 at 10:11:13AM -0500, Spencer Janssen wrote:
On Friday 12 October 2007 10:03:52 David Roundy wrote:
Fri Oct 12 11:02:53 EDT 2007 David Roundy
* fix potential hole in userCode. This makes userCode catch errors even when the user does something like (return undefined). Applied.
I was thinking that we might get a little more benefit from adding just a tad of strictness to catchX itself. It could call seq on the return value to catch at least the grossest errors that might be hidden in lazy values. That still wouldn't obsolete this change to userCode, since the advantage when we've got a () return value is that we can safely ignore it and keep any changes that might have been made.
Fwiw, here's the exception catching code from lambdabot. This has to evaluate completely untrusted code, of course:
case s of Left e -> mapM_ putStrLn e Right v -> Control.Exception.catch (putStrLn v) (\e -> Control.Exception.handle (const $ putStrLn "Exception") $ do e' <- Control.Exception.evaluate e putStrLn $ "Exception: " ++ take 1024 (show e'))
Right means there was no compile error. So we then show the value, forcing it. Note that it can throw an exception whose thrown value is an exception. 'evaluate' takes care of some of the work for us.
That's interesting. Although Config can be assumed to be less hostile than lambdabot users, we've got a slightly harder problem in catchX, since we know of no way to force the value (as showing it does for lambdabot). Probably worrying about calls to (error (error "Gotcha!")) is beyond the scope of catchX, as we don't need to deal with malicious code, just buggy code, and I can't imagine how someone would accidentally do something like that. Malicious people, of course, will just write bug-free code that deletes the user's home directory, which catchX can't prevent. Of course, if IO were broken up into smaller monads, we could restrict code to not touch the filesystem if we wanted. Or even better, if we had a really tricky monad, code could be restricted to only touch the ~/.xmonadcontrib/ directory... :) -- David Roundy Department of Physics Oregon State University

On Fri, Oct 12, 2007 at 01:56:17PM -0400, David Roundy wrote:
On Fri, Oct 12, 2007 at 10:32:48AM -0700, Don Stewart wrote:
case s of Left e -> mapM_ putStrLn e Right v -> Control.Exception.catch (putStrLn v) (\e -> Control.Exception.handle (const $ putStrLn "Exception") $ do e' <- Control.Exception.evaluate e putStrLn $ "Exception: " ++ take 1024 (show e'))
Right means there was no compile error. So we then show the value, forcing it. Note that it can throw an exception whose thrown value is an exception. 'evaluate' takes care of some of the work for us.
Lambdabot forks anyway, so all it really has to worry about is stopping an output flood.
That's interesting. Although Config can be assumed to be less hostile than lambdabot users, we've got a slightly harder problem in catchX, since we know of no way to force the value (as showing it does for lambdabot).
Probably worrying about calls to (error (error "Gotcha!")) is beyond the scope of catchX, as we don't need to deal with malicious code, just buggy code, and I can't imagine how someone would accidentally do something like that. Malicious people, of course, will just write bug-free code that deletes the user's home directory, which catchX can't prevent.
Of course, if IO were broken up into smaller monads, we could restrict code to not touch the filesystem if we wanted. Or even better, if we had a really tricky monad, code could be restricted to only touch the ~/.xmonadcontrib/ directory... :)
The main issue is: modify (\x -> posionWithBottoms x) That will not fail immediatly, but will cripple xmonad by causing all commands that read the state to fail, including mod-q. Your only choice is to quit xmonad wholesale. My proposal (shared, I believe, with sjanssen) is "fail early, fail often, and force the people who commit bugs to wear imaginary dunce caps". Stefan

On Fri, Oct 12, 2007 at 08:15:52PM -0700, Stefan O'Rear wrote:
The main issue is:
modify (\x -> posionWithBottoms x)
That will not fail immediatly, but will cripple xmonad by causing all commands that read the state to fail, including mod-q. Your only choice is to quit xmonad wholesale.
My proposal (shared, I believe, with sjanssen) is "fail early, fail often, and force the people who commit bugs to wear imaginary dunce caps".
Another option is to deepseq the state after every command. (That bugs like this exist in the first place is IMO a serious failing of lazy languages; where's SML-with-qualified-types-and-good-syntax when you need it? *grin*) Stefan
participants (4)
-
David Roundy
-
Don Stewart
-
Spencer Janssen
-
Stefan O'Rear