Invoke toJson forcibly even if DataSourceImpl is not public by dmikurube · Pull Request #26 · embulk/embulk-util-config

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

+        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

+            // 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

+            // 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

+        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

Navigation Menu

Invoke toJson forcibly even if DataSourceImpl is not public - #26

Invoke toJson forcibly even if DataSourceImpl is not public#26
dmikurube wants to merge 1 commit into
masterfrom
invoke-toJson-forcibly

dmikurube commented Feb 3, 2021 •

edited

Loading

Uh oh!

huylenq Feb 3, 2021

Uh oh!

dmikurube Feb 4, 2021

Uh oh!

huylenq Feb 3, 2021

Uh oh!

dmikurube Feb 4, 2021

Uh oh!

dmikurube left a comment

Uh oh!

dmikurube Feb 4, 2021

Uh oh!

dmikurube Feb 4, 2021

Uh oh!

dmikurube commented Feb 4, 2021

Uh oh!

huylenq commented Feb 4, 2021

Uh oh!

huylenq commented Feb 4, 2021 •

edited

Loading

Uh oh!

dmikurube commented Feb 4, 2021

Uh oh!

dmikurube commented Feb 4, 2021

Uh oh!

dmikurube commented Feb 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Conversation

dmikurube commented Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

huylenq Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

dmikurube Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

huylenq Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

dmikurube Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

dmikurube left a comment

Choose a reason for hiding this comment

Uh oh!

dmikurube Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

dmikurube Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

dmikurube commented Feb 4, 2021

Uh oh!

huylenq commented Feb 4, 2021

Uh oh!

huylenq commented Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmikurube commented Feb 4, 2021

Uh oh!

dmikurube commented Feb 4, 2021

Uh oh!

dmikurube commented Feb 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

dmikurube commented Feb 3, 2021 •

edited

Loading

huylenq commented Feb 4, 2021 •

edited

Loading