Instance methods level control#1027
Conversation
(some-> x :field) expands to (if (nil? x) nil (:field x)) so adds some unnecessary overhead when fields are never nil
Does not interfere with :allow :all
E.g.
(sci/eval-string "(.toString \"your name\")"
{:classes {'java.lang.String
{:class java.lang.String
:instance-methods {'toString
(fn [_s]
:dude)}}}})
|
This approach also works nicely with the new 1.12+ way of referring to an instance method: 'java.io.BufferedReader {:class java.io.BufferedReader
:instance-methods
{:deny true
'close java.io.BufferedReader/.close}} |
Feedback from ClaudeHere's my review of PR #1027: PR #1027: Instance methods level control Concept: Adds per-method control for instance method interop — you can override individual methods with custom functions, or deny all non-listed methods. This is a useful security feature for sandboxing. Issues I see: 1. Analyzer change (analyzer.cljc) is a refactor with a subtle bug The some-> rewrite changes behavior. Original: 2. normal-interop closure allocated on every invocation In the evaluator, normal-interop is defined as a fn inside the let — this creates a new closure object on every method call. This is a hot path (every instance method invocation goes through here). The original code just had the expressions 3. args are not evaluated before being passed to the custom function When a custom :instance-methods function is found, the PR does: 4. Method arity mismatch for overrides The custom function receives (instance-expr* ...args), but the PR comment/test asks "should toString also receive the class like :static-methods?" This API inconsistency should be decided before merge. 5. :deny key lives inside the :instance-methods map alongside symbol keys Mixing config keys (:deny) with data keys (method symbols) in the same map is a bit awkward. A symbol can never collide with :deny, so it works, but a cleaner design might be: 6. No support for static methods, fields The PR description acknowledges this. It's fine for a first iteration, but the design should anticipate extending to static methods/fields. 7. Tests use tu/eval* with when-not tu/native? The tests correctly guard against native mode, but per the CLAUDE.md guidelines, tests that pass custom :classes opts should use sci/eval-string directly instead of tu/eval* — this makes the intent clearer and avoids the native-mode shelling. Summary The concept is solid and useful for sandboxing. The critical issue is #3 (args not evaluated) — this will cause runtime failures for any overridden method that takes arguments. The performance concern (#2) should also be addressed given this :black_circle_for_record: Looking at the hot path in eval-instance-method-invocation: 1. Closure allocation on every call — The normal-interop fn is the main concern. It's created on every instance method invocation regardless of which branch is taken. On the JVM, the JIT may optimize this via escape analysis, but it's not 2. For the :allow :all path (babashka's default config) — the overhead is minimal beyond the closure: Same number of lookups, just the extra closure indirection. 3. For the per-class config path (no :allow :all) — there's one additional map lookup per call: This extra :instance-methods lookup happens even when the feature isn't used. Bottom line: The closure is the only real concern. The extra map lookups are negligible. You could eliminate the closure by inlining normal-interop at its 3 call sites (it's only 3 lines). For a hot-path function in an interpreter loop,
I'm still uncertain whether the custom functions passed via :static-methods {'forName (fn [_Class _forName] :dude)}I don't know why this |
Because a static method is called on a class. But instance methods we don't need this. Some additional feedback:
At evaluator.cljc:162,
|
|
This work could also help block forever running code like (.pow (biginteger 10) 10000000) From #1038 (comment) |
To avoid surprises (e.g. because of typos) I only allow interop of instance methods when the I will look into the (failing) cljs tests now |
java.math.BigInteger/.pow |
jeroenvandijk
changed the title
|
Looks really clean to me. Before I did into the code: are there any costs for existing SCI users when NOT using the extra feature? |
|
I think there is effectively one extra lookup and check around Also I am wondering if not more things can move to the analyzer phase. |
|
If that check is only at analysis time and not at runtime, then that's fine |
|
The check is at runtime so I will have a look how I can change that. It is a bit hard to do a good benchmark, but it seems there is some overhead when the feature is not being used (see test results below). Test resultsvs Test code(require '[sci.core :as sci]
'[criterium.core :refer :all])
;; To minimize overhead let Sci generate a closure that we can call directly with Criterium
(let [f (sci/eval-string "(fn [] (.toString true))" {:classes {'java.lang.Boolean {}}})]
(bench (f)))master (9fa8e46)io.github.jeroenvandijk/sci (66121ce) |
|
If you can please move the check to analysis time |
|
I don't think the check can be moved to analysis time because the instance class is only known at runtime. The best I can think of is that we do something with method-cache in interop. E.g. this cache could be prepopulated with the options given in the context. I think this only needs a small wrapper change to make it work for both functions and methods. For cljs there is no caching mechanism we can reuse, so the new code would still have overhead compared to the current approach. Having said that, it could be an option to move the permission checks to the interop part and cache this as well. I'm guessing it would be the same as the current implementation or maybe even a bit faster because the current permission checks would only be executed once. I can try this route as well if you think it could be ok. |
|
Here is a POC for moving the permissions checks to interop as I mentioned as alternative above 9fa8e46...jeroenvandijk:sci:instance-methods-control-v3 Note that I didn't implement the caching yet because i need to get all the arguments right, but this would be more or less the structure I believe. The tests pass for both jvm and node, but I have a feeling maybe there is some missing coverage as I didn't implement permissions for the node part. TLDR; this direction is probably too big of a chance. |
|
I'll take a look at this soon-ish. The most important bit is to get the API right, the rest will probably work out somehow |
|
API + tests I mean |
|
I just realized I've missed the case where a method is overriden for a public class (implementation will make the code less clean). (testing "method override for child instances"
(is (= #{:b} (tu/eval* "(.keySet {:a 1})"
{:classes {'java.util.Map {:instance-methods {'keySet (fn [_m] #{:b})}}
:public-class (fn [o]
(when (instance? java.util.Map o) java.util.Map))}})))) |
|
I've added an implementation to support the above case. But now I'm thinking what to do in this case: (is (= #{:b} (tu/eval* "(.keySet {:a 1})"
{:classes {'clojure.lang.PersistentArrayMap {:instance-methods {'toString (fn [_m] #{:c})}}
'java.util.Map {:instance-methods {'keySet (fn [_m] #{:b})}}
:public-class (fn [o]
(when (instance? java.util.Map o) java.util.Map))}})))With the current implementation it will throw because |
2 participants
Please answer the following questions and leave the below in as part of your PR.
I have read the developer documentation.
This PR corresponds to an issue with a clear problem statement.
This PR contains a test to prevent against future regressions
Sci already offers control over what functions can and can't be called, and each function can get a custom implementation. For interop there is no such feature yet. Right now, you can either call an instance method as per the original implementation or you can't (configured via
:classes). Ideally, there would be a middle ground where you can offer an alternative implementation to an otherwise potentially harmful method, or in some cases deny certain methods entirely.This is a first take at adding method level based control to Sci. It should allow to give access to functionality of configured classes while blocking other undesirable functionality, e.g. think access control of a filesystem.
The implementation in this PR allows to add a custom implementation of a method (as a function) and other methods can be denied.
E.g.
I believe the overhead per method invocation of this feature to traditional paths is limited to:
The option
:allowis checked first and if true, the rest is ignored and interop is applied directly (like before), so no overhead in that case.The option
:denyin:instance-methodscould be extended to static methods, static fields and instance fields.