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

[Quuxplusone]

Bugzilla Link	PR40913
Status	CONFIRMED
Importance	P normal
Reported by	Leslie Zhai (zhaixiang@loongson.cn)
Reported on	2019-02-28 17:48:37 -0800
Last modified on	2020-09-19 07:24:42 -0700
Version	trunk
Hardware	Other Linux
CC	dcoughlin@apple.com, lebedev.ri@gmail.com, llvm-bugs@lists.llvm.org, noqnoqneo@gmail.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

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

[Quuxplusone]

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

[Quuxplusone]

https://reviews.llvm.org/D59054

[Quuxplusone]

Whoops, wrong revision :D

Anyway, i'm looking into this.

[Quuxplusone]

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.

[Quuxplusone]

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

[Quuxplusone]

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