Clojure

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

Details

  • Type: Defect Defect
  • Status: Open Open
  • Priority: Minor Minor
  • Resolution: Unresolved
  • Affects Version/s: Release 1.5
  • Fix Version/s: Release 1.6
  • Component/s: None
  • Labels:
  • Patch:
    Code
  • Approval:
    Incomplete

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)
CompilerException java.lang.RuntimeException: Unable to resolve symbol: source in this context, compiling:(NO_SOURCE_PATH:1:1)

Approach: Have source temporarily bind read-eval to true while reading so it works regardless of the actual read-eval state.

Patch: 0001-CLJ-1176Bind-read-eval-true-in-clojure.repl-source.patch

Screened by: Alex Miller - my only question is whether this is ok from a security point of view. General response from questions on clojure-dev was that this was not a concern for the clojure.repl namespace which can be disallowed in sensitive environments.

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.

People

Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
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.