[
https://issues.apache.org/jira/browse/GROOVY-12461?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Paul King updated GROOVY-12461:
-------------------------------
Description:
*Summary:* {{@GrabConfig(systemProperties = ...)}} sets JVM system properties
at runtime even when Grape is disabled or unavailable
*Affects version:* 6.0.0
h2. Problem
When a class or script uses {{@Grab}} together with
{{@GrabConfig(systemProperties = ...)}}, {{GrabAnnotationTransformation}} (line
490 in 6.0.0) adds a static initializer that does two things in order:
# sets each listed system property, guarded by
{{System.getProperty("groovy.grape.enable", "true") == "true"}};
# calls {{Grape.grab(...)}}.
The {{@GrabConfig}} javadoc says the properties "must be set before invoking
the grab", for example SSL or proxy settings. Their only purpose is to support
the grab. But the guard looks at the system property alone. It ignores the
other two ways Grape can be off:
* *No engine on the classpath.* In Groovy 6, Grape engines live in
{{groovy-grape-ivy}} and {{groovy-grape-maven}}. With neither present,
{{Grape.getInstance()}} returns null, {{Grape.grab}} does nothing, and the log
says "Grapes disabled". The properties are still set.
* *{{Grape.setEnableGrapes(false)}}.* {{Grape.grab}} checks the static
{{enableGrapes}} flag and does nothing. The generated guard never looks at that
flag, so the properties are still set.
In both cases the class changes process-wide state, possibly trust-store or
proxy settings, for a grab that never happens. Hosts that turn Grape off, such
as embedders, tooling and notebook kernels, reasonably expect {{@GrabConfig}}
to be inert too.
A smaller inconsistency: {{Grape}} reads the property with
{{Boolean.parseBoolean}}, which ignores case, while the generated guard
compares it to the exact string "true". So {{-Dgroovy.grape.enable=TRUE}} turns
grabbing on but skips the properties.
h2. Reproducer
{code:groovy}
// G2.groovy
@GrabConfig(systemProperties = 'probe.flag=set-by-grabconfig')
@Grab('org.apache.commons:commons-lang3:3.14.0')
import java.util.List
println "flag=" + System.getProperty('probe.flag')
{code}
Results against Groovy 6.0.0 (JDK 17):
||Setup||Grab runs?||{{probe.flag}}||
|core jar only ({{java -cp groovy-6.0.0.jar groovy.ui.GroovyMain
G2.groovy}})|no ("Grapes disabled")|*set-by-grabconfig*|
|engines present, {{Grape.enableGrapes = false}} before evaluating with
{{GroovyShell}}|no|*set-by-grabconfig*|
|core jar only, {{-Dgroovy.grape.enable=false}}|no|null|
|engines present, {{GrabAnnotationTransformation}} disabled via
{{CompilerConfiguration.disabledGlobalASTTransformations}}|no|null|
|engines present, defaults|yes|set-by-grabconfig (expected)|
When {{@Grab}} is on a class rather than a script, the properties are set when
that class initializes, not when the script starts.
h2. Proposed fix
Set the properties only when a grab will actually run. The simplest form is to
add a small public helper to {{Grape}}, for example
{{Grape.isGrabAvailable()}}, returning {{enableGrapes && getInstance() !=
null}}. The generated guard then calls the helper instead of reading the system
property. This also removes the case mismatch, because the helper uses the same
parsed flag as {{Grape.grab}}.
An alternative is to pass the properties to {{Grape.grab}} in its arguments
map, which the compile-time path already does but no engine reads. The facade
would then apply them just before delegating to the engine. That changes
compile-time behaviour, because the properties would also be set in the
compiler's JVM. That should be a separate decision, so the minimal fix above is
preferred for this ticket.
h2. Compatibility
Behaviour changes only when Grape is disabled or no engine is available. That
is exactly when the properties had no effect on any grab. Code that relied on
{{@GrabConfig}} as a general way to set system properties, without a working
grab, would need to call {{System.setProperty}} itself. That seems unlikely,
and it could be noted in the release notes.
h2. Tests
* With no engine on the test classpath, a class using
{{@GrabConfig(systemProperties)}} leaves the property unset. Run it in a forked
JVM, or in {{groovy-grape-test}} with a classloader that hides the engines.
* After {{Grape.setEnableGrapes(false)}}, the same class leaves the property
unset. Restore the flag afterwards.
* With an engine present and grapes enabled, behaviour is unchanged.
h2. Context
Found while drafting GEP-32 (restricted compiler profile). The profile turns
off the Grab transform for each compilation, which avoids this. Hosts that rely
on keeping the engines off the classpath, or on
{{Grape.setEnableGrapes(false)}}, do not get that protection.
was:
h2. Problem
When a class or script uses {{@Grab}} together with
{{@GrabConfig(systemProperties = ...)}}, {{GrabAnnotationTransformation}} adds
a static initializer that does two things in order:
# sets each listed system property, guarded by
{{System.getProperty("groovy.grape.enable", "true") == "true"}};
# calls {{Grape.grab(...)}}.
The {{@GrabConfig}} javadoc says the properties "must be set before invoking
the grab", for example SSL or proxy settings. Their only purpose is to support
the grab. But the guard looks at the system property alone. It ignores the
other two ways Grape can be off:
* *No engine on the classpath.* In Groovy 6, Grape engines live in
{{groovy-grape-ivy}} and {{groovy-grape-maven}}. With neither present,
{{Grape.getInstance()}} returns null, {{Grape.grab}} does nothing, and the log
says "Grapes disabled". The properties are still set.
* *{{Grape.setEnableGrapes(false)}}.* {{Grape.grab}} checks the static
{{enableGrapes}} flag and does nothing. The generated guard never looks at that
flag, so the properties are still set.
In both cases the class changes process-wide state, possibly trust-store or
proxy settings, for a grab that never happens. Hosts that turn Grape off, such
as embedders, tooling and notebook kernels, reasonably expect {{@GrabConfig}}
to be inert too.
A smaller inconsistency: {{Grape}} reads the property with
{{Boolean.parseBoolean}}, which ignores case, while the generated guard
compares it to the exact string "true". So {{-Dgroovy.grape.enable=TRUE}} turns
grabbing on but skips the properties.
h2. Reproducer
{code:groovy}
// G2.groovy
@GrabConfig(systemProperties = 'probe.flag=set-by-grabconfig')
@Grab('org.apache.commons:commons-lang3:3.14.0')
import java.util.List
println "flag=" + System.getProperty('probe.flag')
{code}
Results against 6.0.0-beta-2:
||Setup||Grab runs?||{{probe.flag}}||
|core jar only ({{java -cp groovy-6.0.0-beta-2.jar groovy.ui.GroovyMain
G2.groovy}})|no ("Grapes disabled")|*set-by-grabconfig*|
|engines present, {{Grape.enableGrapes = false}} before evaluating with
{{GroovyShell}}|no|*set-by-grabconfig*|
|core jar only, {{-Dgroovy.grape.enable=false}}|no|null|
|engines present, {{GrabAnnotationTransformation}} disabled via
{{CompilerConfiguration.disabledGlobalASTTransformations}}|no|null|
|engines present, defaults|yes|set-by-grabconfig (expected)|
When {{@Grab}} is on a class rather than a script, the properties are set when
that class initializes, not when the script starts.
h2. Proposed fix
Set the properties only when a grab will actually run. The simplest form is to
add a small public helper to {{Grape}}, for example
{{Grape.isGrabAvailable()}}, returning {{enableGrapes && getInstance() !=
null}}. The generated guard then calls the helper instead of reading the system
property. This also removes the case mismatch, because the helper uses the same
parsed flag as {{Grape.grab}}.
An alternative is to pass the properties to {{Grape.grab}} in its arguments
map, which the compile-time path already does but no engine reads. The facade
would then apply them just before delegating to the engine. That changes
compile-time behaviour, because the properties would also be set in the
compiler's JVM. That should be a separate decision, so the minimal fix above is
preferred for this ticket.
h2. Compatibility
Behaviour changes only when Grape is disabled or no engine is available. That
is exactly when the properties had no effect on any grab. Code that relied on
{{@GrabConfig}} as a general way to set system properties, without a working
grab, would need to call {{System.setProperty}} itself. That seems unlikely,
and it could be noted in the release notes.
h2. Tests
* With no engine on the test classpath, a class using
{{@GrabConfig(systemProperties)}} leaves the property unset. Run it in a forked
JVM, or in {{groovy-grape-test}} with a classloader that hides the engines.
* After {{Grape.setEnableGrapes(false)}}, the same class leaves the property
unset. Restore the flag afterwards.
* With an engine present and grapes enabled, behaviour is unchanged.
h2. Context
Found while drafting GEP-32 (restricted compiler profile). The profile turns
off the Grab transform for each compilation, which avoids this. Hosts that rely
on keeping the engines off the classpath, or on
{{Grape.setEnableGrapes(false)}}, do not get that protection.
> @GrabConfig(systemProperties = ...) sets JVM system properties at runtime
> even when Grape is disabled or unavailable
> --------------------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12461
> URL: https://issues.apache.org/jira/browse/GROOVY-12461
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> *Summary:* {{@GrabConfig(systemProperties = ...)}} sets JVM system properties
> at runtime even when Grape is disabled or unavailable
> *Affects version:* 6.0.0
> h2. Problem
> When a class or script uses {{@Grab}} together with
> {{@GrabConfig(systemProperties = ...)}}, {{GrabAnnotationTransformation}}
> (line 490 in 6.0.0) adds a static initializer that does two things in order:
> # sets each listed system property, guarded by
> {{System.getProperty("groovy.grape.enable", "true") == "true"}};
> # calls {{Grape.grab(...)}}.
> The {{@GrabConfig}} javadoc says the properties "must be set before invoking
> the grab", for example SSL or proxy settings. Their only purpose is to
> support the grab. But the guard looks at the system property alone. It
> ignores the other two ways Grape can be off:
> * *No engine on the classpath.* In Groovy 6, Grape engines live in
> {{groovy-grape-ivy}} and {{groovy-grape-maven}}. With neither present,
> {{Grape.getInstance()}} returns null, {{Grape.grab}} does nothing, and the
> log says "Grapes disabled". The properties are still set.
> * *{{Grape.setEnableGrapes(false)}}.* {{Grape.grab}} checks the static
> {{enableGrapes}} flag and does nothing. The generated guard never looks at
> that flag, so the properties are still set.
> In both cases the class changes process-wide state, possibly trust-store or
> proxy settings, for a grab that never happens. Hosts that turn Grape off,
> such as embedders, tooling and notebook kernels, reasonably expect
> {{@GrabConfig}} to be inert too.
> A smaller inconsistency: {{Grape}} reads the property with
> {{Boolean.parseBoolean}}, which ignores case, while the generated guard
> compares it to the exact string "true". So {{-Dgroovy.grape.enable=TRUE}}
> turns grabbing on but skips the properties.
> h2. Reproducer
> {code:groovy}
> // G2.groovy
> @GrabConfig(systemProperties = 'probe.flag=set-by-grabconfig')
> @Grab('org.apache.commons:commons-lang3:3.14.0')
> import java.util.List
> println "flag=" + System.getProperty('probe.flag')
> {code}
> Results against Groovy 6.0.0 (JDK 17):
> ||Setup||Grab runs?||{{probe.flag}}||
> |core jar only ({{java -cp groovy-6.0.0.jar groovy.ui.GroovyMain
> G2.groovy}})|no ("Grapes disabled")|*set-by-grabconfig*|
> |engines present, {{Grape.enableGrapes = false}} before evaluating with
> {{GroovyShell}}|no|*set-by-grabconfig*|
> |core jar only, {{-Dgroovy.grape.enable=false}}|no|null|
> |engines present, {{GrabAnnotationTransformation}} disabled via
> {{CompilerConfiguration.disabledGlobalASTTransformations}}|no|null|
> |engines present, defaults|yes|set-by-grabconfig (expected)|
> When {{@Grab}} is on a class rather than a script, the properties are set
> when that class initializes, not when the script starts.
> h2. Proposed fix
> Set the properties only when a grab will actually run. The simplest form is
> to add a small public helper to {{Grape}}, for example
> {{Grape.isGrabAvailable()}}, returning {{enableGrapes && getInstance() !=
> null}}. The generated guard then calls the helper instead of reading the
> system property. This also removes the case mismatch, because the helper uses
> the same parsed flag as {{Grape.grab}}.
> An alternative is to pass the properties to {{Grape.grab}} in its arguments
> map, which the compile-time path already does but no engine reads. The facade
> would then apply them just before delegating to the engine. That changes
> compile-time behaviour, because the properties would also be set in the
> compiler's JVM. That should be a separate decision, so the minimal fix above
> is preferred for this ticket.
> h2. Compatibility
> Behaviour changes only when Grape is disabled or no engine is available. That
> is exactly when the properties had no effect on any grab. Code that relied on
> {{@GrabConfig}} as a general way to set system properties, without a working
> grab, would need to call {{System.setProperty}} itself. That seems unlikely,
> and it could be noted in the release notes.
> h2. Tests
> * With no engine on the test classpath, a class using
> {{@GrabConfig(systemProperties)}} leaves the property unset. Run it in a
> forked JVM, or in {{groovy-grape-test}} with a classloader that hides the
> engines.
> * After {{Grape.setEnableGrapes(false)}}, the same class leaves the property
> unset. Restore the flag afterwards.
> * With an engine present and grapes enabled, behaviour is unchanged.
> h2. Context
> Found while drafting GEP-32 (restricted compiler profile). The profile turns
> off the Grab transform for each compilation, which avoids this. Hosts that
> rely on keeping the engines off the classpath, or on
> {{Grape.setEnableGrapes(false)}}, do not get that protection.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
