Invoke toJson forcibly even if DataSourceImpl is not public #26

dmikurube · 2021-02-03T08:40:22Z

As described in #24, this pull-request is to save a case of :

The user is using Embulk v0.10.2 or earlier (typically, v0.9.23), AND
The user is using another plugin with embulk-util-config:0.1.4 or earlier.

But, as reviewers can see, this is too hacky, and dangerous potentially. In addition, this is not a critical issue when all the plugins are using embulk-util-config:0.2.1+ with #24.

I basically believe we don't need this although I made this pull-request. (So, this is not included in the Milestone v0.2.1 as of now.)

Wondered if I could have a comment from reviewer(s).

huylenq · 2021-02-03T12:08:09Z

src/main/java/org/embulk/util/config/Compat.java

+        synchronized (toJson) {
+            try {
+                toJson.setAccessible(true);
+            } catch (final SecurityException ex) {
+                throw new IllegalStateException(
+                        "Method#setAccessible(true) is forbidden for thie method DataSource(Impl)#toJson. "
+                            + "DataSource class: " + source.getClass().toString()
+                            + "toJson method: " + toJson.toString(),
+                        ex);
+            }
+
+            try {
+                return toJson.invoke(source);
+            } catch (final InvocationTargetException ex) {
+                final Throwable targetException = ex.getTargetException();
+                if (targetException instanceof RuntimeException) {
+                    throw (RuntimeException) targetException;
+                }
+                if (targetException instanceof Error) {
+                    throw (Error) targetException;
+                }
+                throw new IllegalStateException("DataSource#toJson() threw unexpected Exception.", targetException);
+            } catch (final IllegalAccessException ex) {
+                throw new IllegalStateException(
+                        "DataSource(Impl)#toJson is not accessible even after Method#setAccessible(true). "
+                            + "DataSource class: " + source.getClass().toString()
+                            + "toJson method: " + toJson.toString(),
+                        ex);
+            } finally {
+                toJson.setAccessible(false);
+            }
+        }


Q: If we narrow the scope of toJson by obtaining a new local variable Method toJson = getToJsonMethod(source), is there a need for synchronizing and restore the accessibility here? In case there's no need, do you still consider it potentially dangerous?

I thought it can still be dangerous because:

Method#setAccessible is modifying the "method".

The toJson instance, which is retrieved by Class#getMethod("toJson"), is expected to be the same instance if another Class#getMethod("toJson") is called in another thread.

Yeah, we have to consider a lot of possibility with this type of hacks -- then, as I mentioned, I'm not very positive to merge this.

huylenq · 2021-02-03T12:18:45Z

src/main/java/org/embulk/util/config/Compat.java

+            // then invoking "toJson" of "org.embulk.util.config.DataSourceImpl" caused IllegalAccessException.
+            // https://github.com/embulk/embulk-util-config/pull/20
+            //
+            // Pass-through to the next step to setAccessible(true) for the "toJson" Method instance.


Q: I assumed a pass-through here means there's a possibility of another type of exception is thrown but could be resolved with the "accessible = true" hack?

My thought is,

If toJson.invoke(source); throws a type of Exception:

If the Exception is InvocationTargetException: all rethrown to fail explicittly.

If the Exception is IllegalAccessException: passing-through intentionally.

Otherwise: thrown-up to fail.

And those are intended and expected. The case 1 and case 3 should fail immediately. Only the case 2 should pass-through to the next step.

Then, only in the case 2, the accessible flag should be set back false by toJson.setAccessible(false); in finally.

dmikurube

Thanks for your comments!

dmikurube · 2021-02-04T04:08:19Z

src/main/java/org/embulk/util/config/Compat.java

+            // then invoking "toJson" of "org.embulk.util.config.DataSourceImpl" caused IllegalAccessException.
+            // https://github.com/embulk/embulk-util-config/pull/20
+            //
+            // Pass-through to the next step to setAccessible(true) for the "toJson" Method instance.


My thought is,

If toJson.invoke(source); throws a type of Exception:

If the Exception is InvocationTargetException: all rethrown to fail explicittly.

If the Exception is IllegalAccessException: passing-through intentionally.

Otherwise: thrown-up to fail.

And those are intended and expected. The case 1 and case 3 should fail immediately. Only the case 2 should pass-through to the next step.

Then, only in the case 2, the accessible flag should be set back false by toJson.setAccessible(false); in finally.

dmikurube · 2021-02-04T04:09:33Z

src/main/java/org/embulk/util/config/Compat.java

+        synchronized (toJson) {
+            try {
+                toJson.setAccessible(true);
+            } catch (final SecurityException ex) {
+                throw new IllegalStateException(
+                        "Method#setAccessible(true) is forbidden for thie method DataSource(Impl)#toJson. "
+                            + "DataSource class: " + source.getClass().toString()
+                            + "toJson method: " + toJson.toString(),
+                        ex);
+            }
+
+            try {
+                return toJson.invoke(source);
+            } catch (final InvocationTargetException ex) {
+                final Throwable targetException = ex.getTargetException();
+                if (targetException instanceof RuntimeException) {
+                    throw (RuntimeException) targetException;
+                }
+                if (targetException instanceof Error) {
+                    throw (Error) targetException;
+                }
+                throw new IllegalStateException("DataSource#toJson() threw unexpected Exception.", targetException);
+            } catch (final IllegalAccessException ex) {
+                throw new IllegalStateException(
+                        "DataSource(Impl)#toJson is not accessible even after Method#setAccessible(true). "
+                            + "DataSource class: " + source.getClass().toString()
+                            + "toJson method: " + toJson.toString(),
+                        ex);
+            } finally {
+                toJson.setAccessible(false);
+            }
+        }


I thought it can still be dangerous because:

Method#setAccessible is modifying the "method".

The toJson instance, which is retrieved by Class#getMethod("toJson"), is expected to be the same instance if another Class#getMethod("toJson") is called in another thread.

Yeah, we have to consider a lot of possibility with this type of hacks -- then, as I mentioned, I'm not very positive to merge this.

dmikurube · 2021-02-04T04:11:15Z

As mentioned, I'm not very aggressive to merge this pull-request. If you have concerns, let's skip this at least for now, and prioritize merging #23, #24, and #25 to release v0.2.1.

huylenq · 2021-02-04T04:24:27Z

Hi Dai, I understood. Please see this discussion as a sidetrack one and prioritize the other three. It's just interesting to me to discuss on this one out.

huylenq · 2021-02-04T04:32:06Z

I thought it can still be dangerous because:
Method#setAccessible is modifying the "method".

This is where I think I misaligned: IIUC, Method#setAccessible only modifies the particular representative method metadata instance, it doesn't modify the "method" itself (as at the VM-level)?

The toJson instance, which is retrieved by Class#getMethod("toJson"), is expected to be the same instance if another Class#getMethod("toJson") is called in another thread.

Pretty much the same idea as the above, I thought different calls to getMethod returns different metadata instances?

(I'm trying to evaluate the "hacki-ness" of this one and weighing the trade-off of having a hard-to-spot bug when this combination of Embulk and embulk-util-config is met (I totally agree that this rarely will happen)).

dmikurube · 2021-02-04T07:13:56Z

Hmm, not sure why this pull-request is closed...

dmikurube · 2021-02-04T07:14:47Z

Reopening.

dmikurube · 2021-02-04T07:30:23Z

This is where I think I misaligned: IIUC, Method#setAccessible only modifies the particular representative method metadata instance, it doesn't modify the "method" itself (as at the VM-level)?

AFAIU yes, but actually, not 100% confident. Maybe not. But anyway, the Javadoc does not clearly describe that point. https://docs.oracle.com/javase/8/docs/api/java/lang/reflect/AccessibleObject.html#setAccessible-boolean-

Also, the specification may change. For instance, Java 11 (or 12?) changed final fields unmodifiable even via reflection. I had no guarantee that similar does not happen, and just always choosing a "safe" side in such reflection handling.

Invoke toJson forcibly even if DataSourceImpl is not public

f18a1f1

dmikurube mentioned this pull request Feb 3, 2021

Find toJson method even from DataSourceImpl against Embulk v0.10.2 or earlier, whose DataSource interace does not have "toJson" #24

Merged

dmikurube requested a review from huylenq February 3, 2021 09:07

huylenq reviewed Feb 3, 2021

View reviewed changes

dmikurube commented Feb 4, 2021

View reviewed changes

dmikurube closed this in #24 Feb 4, 2021

dmikurube reopened this Feb 4, 2021

dmikurube changed the base branch from find-toJson-even-from-util-DataSourceImpl to master February 4, 2021 07:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Invoke toJson forcibly even if DataSourceImpl is not public #26

Invoke toJson forcibly even if DataSourceImpl is not public #26

dmikurube commented Feb 3, 2021 •

edited

Loading

huylenq Feb 3, 2021

dmikurube Feb 4, 2021

huylenq Feb 3, 2021

dmikurube Feb 4, 2021

dmikurube left a comment

dmikurube Feb 4, 2021

dmikurube Feb 4, 2021

dmikurube commented Feb 4, 2021

huylenq commented Feb 4, 2021

huylenq commented Feb 4, 2021 •

edited

Loading

dmikurube commented Feb 4, 2021

dmikurube commented Feb 4, 2021

dmikurube commented Feb 4, 2021

Invoke toJson forcibly even if DataSourceImpl is not public #26

Are you sure you want to change the base?

Invoke toJson forcibly even if DataSourceImpl is not public #26

Conversation

dmikurube commented Feb 3, 2021 • edited Loading

huylenq Feb 3, 2021

Choose a reason for hiding this comment

dmikurube Feb 4, 2021

Choose a reason for hiding this comment

huylenq Feb 3, 2021

Choose a reason for hiding this comment

dmikurube Feb 4, 2021

Choose a reason for hiding this comment

dmikurube left a comment

Choose a reason for hiding this comment

dmikurube Feb 4, 2021

Choose a reason for hiding this comment

dmikurube Feb 4, 2021

Choose a reason for hiding this comment

dmikurube commented Feb 4, 2021

huylenq commented Feb 4, 2021

huylenq commented Feb 4, 2021 • edited Loading

dmikurube commented Feb 4, 2021

dmikurube commented Feb 4, 2021

dmikurube commented Feb 4, 2021

dmikurube commented Feb 3, 2021 •

edited

Loading

huylenq commented Feb 4, 2021 •

edited

Loading