ibmruntimes / openj9-openjdk-jdk11

Extensions for OpenJDK 11 for Eclipse OpenJ9
GNU General Public License v2.0
31 stars 112 forks source link

Add NULL check for CRIU field retrieval #561

Closed JasonFengJ9 closed 2 years ago

JasonFengJ9 commented 2 years ago

The task could be null.

An internal test expects IllegalArgumentException when task is null and time < 0 [1].

[1] https://github.com/ibmruntimes/openj9-openjdk-jdk11/blob/20d13352ee5c9569416880e340d5c589161082f9/src/java.base/share/classes/java/util/Timer.java#L415-L417

Signed-off-by: Jason Feng fengj@ca.ibm.com

tajila commented 2 years ago

Do we have a test for this

JasonFengJ9 commented 2 years ago

There are internal tests covering two cases:

  1. task = null - NPE is expected
  2. task = null && time < 0 - IllegalArgumentException is expected
tajila commented 2 years ago

I meant a criu test

tajila commented 2 years ago

Also, if task = null - NPE is expected do we need to do anything? the task.criuAdjustRequired = true; will thrown the NPE

JasonFengJ9 commented 2 years ago

CRIU test - https://github.com/eclipse-openj9/openj9/blob/master/test/functional/cmdLineTests/criu/src/org/openj9/criu/TimeChangeTest.java

Also, if task = null - NPE is expected do we need to do anything? the task.criuAdjustRequired = true; will thrown the NPE

There is no problem for NPE test, just for test reference that the task is null. This fix is for IllegalArgumentException when time < 0.

tajila commented 2 years ago

isnt the (delay < 0) check always done first?

JasonFengJ9 commented 2 years ago

Yes, the test in question escaped first check, and the delay was set to 10000 + Long.MAX_VALUE - System.currentTimeMillis(), and time becomes negative after System.currentTimeMillis()+delay.

JasonFengJ9 commented 2 years ago

As per javadoc - java/util/Timer.html#schedule(java.util.TimerTask,long)

IllegalArgumentException - if delay is negative, or delay + System.currentTimeMillis() is negative.

The later checking time < 0 is required for the actual time within sched(TimerTask task, long time, long period), however task.criuAdjustRequired can't be moved there due to other fixed date schedule methods.

tajila commented 2 years ago

Okay, I understand now. Thanks

tajila commented 2 years ago

Jenkins test sanity xlinuxcriu jdk17

tajila commented 2 years ago

is there a jdk17 and jdknext version of this?

JasonFengJ9 commented 2 years ago

Created https://github.com/ibmruntimes/openj9-openjdk-jdk17/pull/129 & https://github.com/ibmruntimes/openj9-openjdk-jdk/pull/477, please review.