On Sat, 11 Jun 2022 08:46:18 GMT, Peter Levart <[email protected]> wrote:
>> Hi,
>> I think the synchronized block was redundant already in original code. Since
>> the entire handle method is `static synchronized` and it is the only method
>> that modifies the `handlers` and `signals` maps.
>> But even with so much redundant synchronization, the Signal class is not
>> without races. There are multiple possible races in `dispatch(int number)`
>> method which is called from native code to dispatch a signal:
>> - race no. 1: dispatch method (not synchronized) performs 2 independent
>> lookups into `signals` and `handlers` maps respectively, assuming their
>> inter-referenced state is consistent. But `handle` method may be
>> concurrently modifying them, so `dispatch` may see updated state of
>> `signals` map while seeing old state of `handlers` map or vice versa.
>> - race no. 2: besides `signals` and `handlers` there is a 3rd map in native
>> code, updated with `handle0` native method. Native code dispatches signals
>> according to that native map, forwarding them to either native handlers or
>> to `dispatch` Java method. But `handle` method may be modifying that native
>> map concurrently, so dispatch may be called as a consequence of updated
>> native map while seeing old states of `signals` and `handlers` maps.
>>
>> I'm sure I might have missed some additional races.
>>
>> How to fix this? Is it even necessary? I think that this internal API is not
>> used frequently so this was hardly an issue. But anyway, it would be a
>> challenge. While the two java maps: `handlers` and `signals` could be
>> replaced with a single map, there is the 3rd map in native code. We would
>> not want to make `dispatch` method synchronized, so with careful ordering of
>> modifications it is perhaps possible to account for races and make them
>> harmless...
>> WDYT?
>
> Well, studying this further, I think some races are already accounted for and
> are harmless except one. The `handle` method 1st attempts to register handler
> in native code via call to `handle0` and then updates the `signals` map.. As
> soon as native code registers the handler, `dispatch` may be called and the
> lookup into `signals` map may return null which would cause NPE in the
> following lookup into `handlers` map. So there are two possibilities to fix
> this:
> - guard for null result from `singnals` lookup, or
> - swap the order of modifying the `signals` map and a call to `handle0`
> native method. You could even update the `signals` map with `.putIfAbsent()`
> to avoid multiple reassignment with otherwise equal instances. This may
> unnecessarily establish a mapping for a Signal the can later not be
> registered, so you can remove it from the `signals` map in that case to
> clean-up.
> The possible case when the lookup into `handlers` map returns null Handler is
> already taken into account, but there is a possible case where a
> NativeHandler is replaced with a java Handler implementation and `dispatch`
> is therefore already called, but `handlers` map lookup still returns a
> NativeHandler. In that case the NativeHandler::handle method will throw
> exception. To avoid that, NativeHandler::handle could be an empty method
> instead.
I tried to reproduce this possible problem via jcstress, but never caught this
race.
package org.openjdk.jcstress.samples.jmm.basic;
import org.openjdk.jcstress.annotations.*;
import org.openjdk.jcstress.infra.results.ZZZZZZZZ_Result;
import sun.misc.Signal;
import sun.misc.SignalHandler;
import java.io.IOException;
import static org.openjdk.jcstress.annotations.Expect.*;
@JCStressTest
@Outcome(id = "false, false, false, false, false, false, false, false", expect
= ACCEPTABLE, desc = "All results are ok")
@Outcome(id = ".*", expect = ACCEPTABLE_INTERESTING, desc = "All results are
ok")
@State
public class BasicJMM_11_Signal {
/*
How to run this test:
$ /home/turbanoff/.jdks/liberica-18.0.1.1/bin/java -jar
jcstress-samples/target/jcstress.jar -t BasicJMM_11_Signal -v
$
/home/turbanoff/Projects/official_jdk/build/linux-x86_64-server-fastdebug/jdk/bin/java
-jar jcstress-samples/target/jcstress.jar -t BasicJMM_11_Signal -v
$
/home/turbanoff/Projects/jdk2/build/linux-x86_64-server-release/jdk/bin/java
-jar jcstress-samples/target/jcstress.jar -t BasicJMM_11_Signal -v
*/
private static final Signal[] signals = new Signal[]{
new Signal("URG"),
// new Signal("USR2"),
// new Signal("ALRM"),
// new Signal("USR1"),
new Signal("CHLD"),
new Signal("XFSZ"),
new Signal("CONT"),
// new Signal("POLL"),
new Signal("WINCH"),
// new Signal("IO"),
new Signal("PIPE"),
// new Signal("HUP"),
// new Signal("POLL"),
// new Signal("PROF"),
// new Signal("INT"),
// new Signal("STKFLT"),
new Signal("TSTP"),
// new Signal("SYS"),
// new Signal("TERM"),
// new Signal("TRAP"),
// new Signal("ABRT"),
new Signal("TTOU"),
new Signal("TTIN"),
// new Signal("BUS"),
// new Signal("VTALRM"),
// new Signal("XCPU"),
// new Signal("PWR")
};
private static final String[][] commands = new String[signals.length][];
private static final long pid = ProcessHandle.current().pid();
static {
for (int i = 0; i < commands.length; i++) {
commands[i] = new String[]{ "kill", "-" + signals[i].getName(),
Long.toString(pid) };
}
}
@Actor
public void thread1() {
for (String[] command : commands) {
try {
new ProcessBuilder(command)
.directory(null)
.start();
} catch (IOException e) {
e.printStackTrace();
}
}
}
@Actor
public void thread2(ZZZZZZZZ_Result r) {
for (int i = 0; i < signals.length; i++) {
Signal signal = signals[i];
Signal.handle(signal, new MySignalHandler(i, r));
}
}
private static class MySignalHandler implements SignalHandler {
private final int num;
private final ZZZZZZZZ_Result r;
public MySignalHandler(int num, ZZZZZZZZ_Result r) {
this.num = num;
this.r = r;
}
@Override
public void handle(Signal sig) {
switch (num) {
case 0:
r.r1 = true;
break;
case 1:
r.r2 = true;
break;
case 2:
r.r3 = true;
break;
case 3:
r.r4 = true;
case 4:
r.r5 = true;
break;
case 5:
r.r6 = true;
break;
case 6:
r.r7 = true;
break;
case 7:
r.r8 = true;
break;
}
}
}
}
-------------
PR: https://git.openjdk.org/jdk/pull/9100
