False positive about Use of memory after it is freed for OpenJDK

[xiangzhai]

Bugzilla Link	40913
Version	trunk
OS	Linux
CC	@devincoughlin,@LebedevRI,@haoNoQ

Extended Description

Hi,

Bug reported by the clang static analyzer.

Description: Use of memory after it is freed File: /home/loongson/zhaixiang/jdk12-mips-llvm/src/java.base/share/native/libverify/check_code.c[1] Line: 1328

Preprocessed file[2] is available.

I argue that Use of memory after it is freed is False Positive

----- 8< -------- 8< -------- 8< -------- 8< -------- 8< -------- 8< --- src/java.base/share/native/libverify/check_code.c:1328:22: warning: Use of memory after it is freed clazz_info = cp_index_to_class_fullinfo(context, key, ^~~~~~~~~~~~

----- 8< -------- 8< -------- 8< -------- 8< -------- 8< -------- 8< ---

Full analyzer log and invocation[3] is available too. Please change include file path, for example, /home/loongson/zhaixiang/jdk12-mips-llvm/src/java.base/share/native/libjava change to YOUR_OPENJDK_SRC_DIR/src/java.base/share/native/libjava

Perhaps it doesn't need to include the build directories, otherwise it is difficult to reproduce the issue :)

Cheers,

Leslie Zhai

[1] http://hg.openjdk.java.net/jdk/jdk12/file/0276cba45aac/src/java.base/share/native/libverify/check_code.c#l1328

[2] https://raw.githubusercontent.com/xiangzhai/jdk-dev/master/check_code.c

[3] https://raw.githubusercontent.com/xiangzhai/jdk-dev/master/check_code_analyzer.log

[xiangzhai]

Hi Artem,

How is going?

exposes a bug in OpenJDK?

But I have no idea how to reproduce the Use-after-free:

diff -r bdc20ee1a68d src/java.base/share/native/libverify/check_code.c
--- a/src/java.base/share/native/libverify/check_code.c Fri Sep 04 23:51:26 2020 -0400
+++ b/src/java.base/share/native/libverify/check_code.c Sat Sep 19 22:21:00 2020 +0800
@@ -1310,6 +1310,7 @@
         is_internal = methodname[0] == '<';
         pop_and_free(context);

+        assert(context != NULL);
         clazz_info = cp_index_to_class_fullinfo(context, key,
                                                 JVM_CONSTANT_Methodref);
         this_idata->operand.i = key;

Thanks, Leslie Zhai

[xiangzhai]

Hi Roman,

Sorry for my late response!

I tried to write a reduced testcase to reproduce the bug, but failed to reproduce...

I often use[1] huge and real OSS to analysis. It is able to find the bugs in analyzer side and testcase side.

Hi Artem,

Long time no see :)

In this case when you then do free(context->allocated_memory), it either causes undefined behavior or releases the malloc()ed memory that's aliased with context. In both cases, the bug report is valid.

I need to check the latest OpenJDK source code carefully, if there is UB or other issue, I will file a bug in JBS[4].

The problem here is that we've "pruned" the execution path within check_and_push(), i.e. we don't display it to the user because we don't realize that there's something interesting in it.

Yes, there is no check_and_push (be pruned) in the report[2][3].

Cheers, Leslie Zhai

[1] https://www.leetcode.cn/2016/11/analyzing-code-for-kde-qt-open-source-components.html [2] https://raw.githubusercontent.com/xiangzhai/jdk-dev/master/check_code01.png [3] https://raw.githubusercontent.com/xiangzhai/jdk-dev/master/check_code02.png [4] https://bugs.openjdk.java.net/browse/JDK-8219074

[haoNoQ]

Hmm, so, even though it's not obvious, the Analyzer is in fact referring to the following execution path:

static void check_and_push(context_type context, const void ptr, int kind) { // ... // Taking true branch. if (context->alloc_stack_top < 16) p = &(context->alloc_stack[context->alloc_stack_top++]); // ... context->allocated_memory = p; }

This causes context->allocated_memory to effectively become equal to context + N, where N is a simple offset.

In this case when you then do free(context->allocated_memory), it either causes undefined behavior or releases the malloc()ed memory that's aliased with context. In both cases, the bug report is valid.

The problem here is that we've "pruned" the execution path within check_and_push(), i.e. we don't display it to the user because we don't realize that there's something interesting in it. In this case the pruned path is 10 pieces long and the unpruned path is 29 pieces long. Given that we have no really good solution for figuring out if the path is interesting, should we disable pruning when unpruned path is short enough?

@​Leslie Zhai, does this make sense to you? Our notes are bad, but do you think that the execution path that the Static Analyzer is complaining about is actually feasible and exposes a bug in OpenJDK?

In any case, the over-eager path pruning deserves a bug report.

[haoNoQ]

Whoops, wrong revision :D

Anyway, i'm looking into this.

[haoNoQ]

https://reviews.llvm.org/D59054

[LebedevRI]

You may want to produce more smaller, fine-grained, self-contained reproducer, that is not "build jdk and then analyse it".

[xiangzhai]

assigned to @devincoughlin