Clojure

clojure.repl/source fails when *read-eval* bound to :unknown

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: Release 1.5
  • Fix Version/s: Release 1.6
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

clojure.repl/source is broken in Clojure 1.5.1 when *read-eval* is bound to :unknown, since source-fn reads without binding.

user> (alter-var-root #'*read-eval* (constantly :unknown))
:unknown
user> (source drop-last)
RuntimeException Reading disallowed - *read-eval* bound to :unknown  clojure.lang.Util.runtimeException (Util.java:219)

Approach: Throw explicit error stating the cause in this case.

Patch: clj-1176-source-read-eval-3.patch

Screened by: Stuart Sierra

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Tim McCormack added a comment -

The attached patch just binds *read-eval* to true inside source-fn.

Show
Tim McCormack added a comment - The attached patch just binds *read-eval* to true inside source-fn.
Hide
Permalink
Stuart Halloway added a comment -

Note: Allowing this implies that you trust the data on your classpath. If there are reasons somebody might not, we should reject this patch and people will have to be explicit when calling source.

Show
Stuart Halloway added a comment - Note: Allowing this implies that you trust the data on your classpath. If there are reasons somebody might not, we should reject this patch and people will have to be explicit when calling source.
Hide
Permalink
Tim McCormack added a comment -

Ugh, that's a fair point when it comes to sandboxing. I'll check with the owners of clojurebot and lazybot.

Show
Tim McCormack added a comment - Ugh, that's a fair point when it comes to sandboxing. I'll check with the owners of clojurebot and lazybot.
Hide
Permalink
Tim McCormack added a comment -

I haven't come up with any scenarios where this is problematic, and I haven't heard anything back from the bot owners. As for sandboxing, clojure.repl can easily be excluded.

Show
Tim McCormack added a comment - I haven't come up with any scenarios where this is problematic, and I haven't heard anything back from the bot owners. As for sandboxing, clojure.repl can easily be excluded.
Hide
Permalink
Gabriel Horner added a comment -

Looks good

Show
Gabriel Horner added a comment - Looks good
Hide
Permalink
Alex Miller added a comment -

Would like to screen this one again. I think it has open questions and is worth a discussion somewhere.

Show
Alex Miller added a comment - Would like to screen this one again. I think it has open questions and is worth a discussion somewhere.
Hide
Permalink
Alex Miller added a comment -

To me, this seems like we would be opening a security hole and a cleverly concocted resource could exploit it.

Other alternatives:
1) do nothing (user can always bind read-eval around a call to source if they want to do this safely)
2) add a source-unsafe or other wrapper function that did this
3) change source-fn to use edn/read instead? This may introduce some cases where source using non-edn features could not be read. I'd be ok with that.

Show
Alex Miller added a comment - To me, this seems like we would be opening a security hole and a cleverly concocted resource could exploit it. Other alternatives: 1) do nothing (user can always bind read-eval around a call to source if they want to do this safely) 2) add a source-unsafe or other wrapper function that did this 3) change source-fn to use edn/read instead? This may introduce some cases where source using non-edn features could not be read. I'd be ok with that.
Hide
Permalink
Andy Fingerhut added a comment -

Maybe this is well known to everyone already, but in case not, doing a require or use on a namespace containing the following function on a Unix-like system to create and/or update the last modification time of the file bartleby.txt. If you remove that file, and then do (source badfn) while read-eval is bound to true, you can see that it will do the shell command again. Obviously much more dangerous side effects could be performed instead of that.

(ns bad.fn)

(defn badfn [x]
  (let [y [#=(clojure.java.shell/sh "touch" "bartleby.txt")]]
    x))

Avoiding that behavior in source-fn, yet still showing the source code, would require a different implementation of read other than clojure.core/read and clojure.edn/read.

Show
Andy Fingerhut added a comment - Maybe this is well known to everyone already, but in case not, doing a require or use on a namespace containing the following function on a Unix-like system to create and/or update the last modification time of the file bartleby.txt. If you remove that file, and then do (source badfn) while read-eval is bound to true, you can see that it will do the shell command again. Obviously much more dangerous side effects could be performed instead of that. 
(ns bad.fn)

(defn badfn [x]
  (let [y [#=(clojure.java.shell/sh "touch" "bartleby.txt")]]
    x))
Avoiding that behavior in source-fn, yet still showing the source code, would require a different implementation of read other than clojure.core/read and clojure.edn/read.
Hide
Permalink
Alex Miller added a comment -

Based on comments on the mailing list, most people are not concerned about this from a security point of view. I'm going to let this one through and Rich can decide further.

Show
Alex Miller added a comment - Based on comments on the mailing list, most people are not concerned about this from a security point of view. I'm going to let this one through and Rich can decide further.
Hide
Permalink
Rich Hickey added a comment -

If you haven't set read-eval and you need to read-eval, then you'd better set it, right? We're not going to do that for you. The only patch that will be accepted for this is one that generates a better error message.

Show
Rich Hickey added a comment - If you haven't set read-eval and you need to read-eval, then you'd better set it, right? We're not going to do that for you. The only patch that will be accepted for this is one that generates a better error message.
Hide
Permalink
Alex Miller added a comment -

Updated with new patch that detects and throws an error if calling source with read-eval is false.

Show
Alex Miller added a comment - Updated with new patch that detects and throws an error if calling source with read-eval is false.
Hide
Permalink
Stuart Sierra added a comment - - edited

Screened OK.

Note that this only changes the error message when read-eval is bound to false, not when it is bound to :unknown.

Show
Stuart Sierra added a comment - - edited Screened OK. Note that this only changes the error message when read-eval is bound to false, not when it is bound to :unknown.
Hide
Permalink
Stuart Sierra added a comment - - edited

Changed my mind: resetting to 'incomplete'.

This patch doesn't fix the situation in the original report. To improve the error message, it should handle the :unknown case.

If *read-eval* is false, then source still works as long as the source form doesn't contain #=

Show
Stuart Sierra added a comment - - edited Changed my mind: resetting to 'incomplete'. This patch doesn't fix the situation in the original report. To improve the error message, it should handle the :unknown case. If *read-eval* is false, then source still works as long as the source form doesn't contain #=
Hide
Permalink
Alex Miller added a comment -

Stuart - totally good catch. Things did not work how I thought they worked! I have updated the patch.

Show
Alex Miller added a comment - Stuart - totally good catch. Things did not work how I thought they worked! I have updated the patch.
Hide
Permalink
Stuart Sierra added a comment -

Screened ✔

Show
Stuart Sierra added a comment - Screened ✔

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4#649-r158309) | Report a problem
Powered by a free Atlassian JIRA open source license for Clojure. Try JIRA - bug tracking software for your team.