JDK-8361955 : [GCC static analyzer] libjdwp/threadControl.c threadControl_setPendingInterrupt error: dereference of NULL 'node'
  • Type: Bug
  • Component: core-svc
  • Sub-Component: debugger
  • Affected Version: 23,26
  • Priority: P4
  • Status: New
  • Resolution: Unresolved
  • OS: linux
  • CPU: generic
  • Submitted: 2025-07-11
  • Updated: 2025-07-15
Related Reports
Causes :  
JDK-8324868 - debug agent does not properly handle interrupts of a virtual thread
Description
GCC static analyzer (-fanalyzer) reports this issue :

jdk/src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c:2321:28: error: dereference of NULL 'node' [CWE-476] [-Werror=analyzer-null-dereference]
 2321 |     node->pendingInterrupt = JNI_TRUE;

We call node = findRunningThread(thread);  but unlike the other findRunningThread calls we do here a JDI_ASSERT and no result-check for node != NULL , maybe this should be adjusted.
Comments
It turns out that JDI_ASSERT failure is not noreturn! The supporting jdiAssertionFailed only exits if the BackendGlobalData has been allocated and it's assertFatal member is true. Otherwise, jdiAssertionFailed just returns. (I suggest that's pretty terrible naming...) So forget what I said about flow analysis issues. The compiler warning is correct.
15-07-2025
I would hope the static code analysis tool recognizes exit() and abort() as APIs that don't return.
14-07-2025
[~cjplummer] I think its the enabled assert (in a debug build) that likely causes problems. We have an explicit test for NULL and then a control path that (so far as the compiler can tell) later uses that known NULL value in an invalid way. Basically, the compiler effectively analyzes the code following the assertion twice, once knowing the value isn't NULL and once knowing it is. The _Noreturn cuts off that control path. We had the same kind of problem in HotSpot with our various debug macro when we were moving to gcc12, even without enabling sanitizers. There the solution was to add C++ [[noreturn]] attributes to the failure reporting functions. (JDK-8302189 and JDK-8303805)
14-07-2025
I don't think the EXIT_ERROR sites are reporting errors. This CR is for a place where we instead use JDI_ASSERT, which will exit if asserts are enabled, so probably the issue only turns up with product builds.
14-07-2025
The warning about using NULL is likely a flow analysis issue resulting from (1) testing for NULL, (2) calling an error reporting function in the NULL case, (3) flow analysis continues after the return from that call. This can be dealt with by declaring that error reporting function as _Noreturn (since C11, which we are using), assuming it doesn't actually return. Windows might make that more tricky; I don't know if it supports _Noreturn. For Windows one might need to use C++ [[noreturn]] (which is also what one should use for C23, but that's probably a long way off). A macro defined in some appropriate xxx_md.h file might be needed to hide that difference.
14-07-2025
I'm starting to think we should just go back to the original code that doesn't complain if the node is not found. The pendingInterrupt flag gets set if a monitor wait is forced to exit with an interrupt. Possibly this could be done on a thread the debug agent doesn't know about yet, and it should be harmless when that happens. I see other places where if findRunningThread returns NULL, and the API just returns NULL as a result, such as for threadControl_getStepRequest() and threadControl_getInvokeRequest. In that case it is up to the caller to handle the NULL result, which callers of threadControl_getStepRequest() and threadControl_getInvokeRequest() do, although there is one case where the caller threadControl_getInvokeRequest() doesn't produce an error, and I'm not so sure that is the correct thing to do, but wouldn't suggest tinkering with it at this point since it has been in place for so long.
12-07-2025
Hi Chris, I can open a PR and change the coding GCC-analyzer complained about in the recommended way. Do you think we should also for other calls of find(Running)Thread (e.g. in moveNode function) change to EXIT_ERROR when NULL is returned ? Currently it is a bit inconsistent.
12-07-2025
This was introduced by JDK-8324868: https://github.com/openjdk/jdk/pull/17989/files#diff-65eb4035b1d9bb5ceaab430461cd782069fe88c0a7d0438964c041caf0aa5eecL2311 We used to have: node = findThread(&runningThreads, thread); if (node != NULL) { node->pendingInterrupt = JNI_TRUE; } However, I don't think that is correct either. We shouldn't be hiding failures to lookup the thread, so that's why it was changed to an assert. I think checking for NULL and producing a fatal error is the correct approach: node = findRunningThread(thread); if (node == NULL) { EXIT_ERROR(AGENT_ERROR_NULL_POINTER,"thread list corrupted"); }
11-07-2025