Re: Request for Comments: Potential leak of memory pointed to by 'name' about jvmciCodeInstaller

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: Request for Comments: Potential leak of memory pointed to by 'name' about jvmciCodeInstaller

David Blaikie via cfe-dev
Hi Doug,

Thanks for your kind response!


在 2019年03月18日 05:55, Doug Simon 写道:
> Hi Leslie,
>
> As a point of process, I think [hidden email] is probably a better list for JVMCI bugs.

Sorry for my wrong posting!

>
> In any case, thanks for the investigation. However, I don’t think this is a bug as RuntimeStub simply passes along the name argument which is eventually stored to CodeBlob::_name without any further copying. If we subsequently freed that argument, the CodeBlob::_name would become invalid.

Thanks for pointing out my fault!
I might brought in Use-after-free[2] issue even that I carefully free
the allocated memory in the end of `installCode` C2V_VMENTRY...
The reduced testcase is able to reproduce my fault:

----- 8< -------- 8< -------- 8< -------- 8< -------- 8< -------- 8< ---
$ cat t.cpp
#include <iostream>
#include <string.h>
#include <stdlib.h>

class CodeBlob {
public:
   CodeBlob(const char* name) : _name(name) {};

   const char* name() { return _name; }

protected:
   const char* _name;
};

class RuntimeBlob : public CodeBlob {
public:
   RuntimeBlob(const char* name) : CodeBlob(name) {}
};

class RuntimeStub : public RuntimeBlob {
private:
   RuntimeStub(const char* name) : RuntimeBlob(name) {}

public:
   static RuntimeStub* new_runtime_stub(const char* stub_name) {
     RuntimeStub* stub = new RuntimeStub(stub_name);
     return stub;
   }
};

static void install(CodeBlob*& cb, char*& name) {
   name = strdup("some stubName");
   cb = RuntimeStub::new_runtime_stub(name);
}

// To simulate  C2V_VMENTRY(jint, installCode, (JNIEnv *jniEnv, jobject,
jobject target, jobject compiled_code, jobject installed_code, jobject
speculation_log))
int main(int argc, char *argv[]) {
   CodeBlob* cb = NULL;
   char* cb_name = NULL;
   install(cb, cb_name);
   if (cb_name) free(cb_name);  // <--- MY FAULT
   std::cout << cb->name() << std::endl;
   return 0;
}
----- 8< -------- 8< -------- 8< -------- 8< -------- 8< -------- 8< ---

t.cpp:9:24: warning: Use of memory after it is freed
   const char* name() { return _name; }
                        ^~~~~~~~~~~~
t.cpp:42:3: warning: Potential leak of memory pointed to by 'cb'
   std::cout << cb->name() << std::endl;
   ^~~~~~~~~~~~~~~~~~~~~~~


So It might be Potential leak of memory pointed to by 'cb' about
jvmciCompilerToVM?  But the `cb` might be used in other place just like
`name` :)
Just comment MY FAULT //if (cb_name) free(cb_name);
And see what dynamic analysis say:

$ clang++ -fsanitize=address t.cpp
$ ./a.out

=================================================================
==4482==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 8 byte(s) in 1 object(s) allocated from:
     #0 0x12017b674  (/home/loongson/zhaixiang/a.out+0x12017b674)
     #1 0x12018097c  (/home/loongson/zhaixiang/a.out+0x12018097c)
     #2 0x12018074c  (/home/loongson/zhaixiang/a.out+0x12018074c)
     #3 0x1201804c4  (/home/loongson/zhaixiang/a.out+0x1201804c4)
     #4 0xfff6cc719c  (/lib64/libc.so.6+0x4319c)
     #5 0x12002b198  (/home/loongson/zhaixiang/a.out+0x12002b198)

Indirect leak of 14 byte(s) in 1 object(s) allocated from:
     #0 0x1200de204  (/home/loongson/zhaixiang/a.out+0x1200de204)
     #1 0x120180688  (/home/loongson/zhaixiang/a.out+0x120180688)
     #2 0x1201804c4  (/home/loongson/zhaixiang/a.out+0x1201804c4)
     #3 0xfff6cc719c  (/lib64/libc.so.6+0x4319c)
     #4 0x12002b198  (/home/loongson/zhaixiang/a.out+0x12002b198)

SUMMARY: AddressSanitizer: 22 byte(s) leaked in 2 allocation(s).

Thanks,
Leslie Zhai

>
> -Doug
>
>> On 17 Mar 2019, at 10:30, Leslie Zhai <[hidden email]> wrote:
>>
>> Hi,
>>
>> Bug reported[1] by the clang static analyzer.
>>
>> Description: Potential leak of memory pointed to by 'name'
>> File: /home/zhaixiang/jdk/src/hotspot/share/jvmci/jvmciCodeInstaller.cpp
>> Line: 653
>>
>> 652  char* name = strdup(java_lang_String::as_utf8_string(stubName));
>>
>>       5 ← Memory is allocated  →
>>
>> 653  cb = RuntimeStub::new_runtime_stub(name,
>>
>>       6 ← Potential leak of memory pointed to by 'name'
>>
>> I checked `install` function in src/hotspot/share/jvmci/jvmciCodeInstaller.cpp and `installCode` C2V_VMENTRY in src/hotspot/share/jvmci/jvmciCompilerToVM.cpp carefully.  There is no `free` to release the allocated memory, so I argue that it is a Memory leak issue, not a False positive[2]. May I file a bug if it is real potential leak of memory issue?
>>
>> Because I think webrev is related to BUGID[3], so I just paste my patch here:
>>
>>
>> ----- 8< -------- 8< -------- 8< -------- 8< -------- 8< -------- 8< ---
>> diff -r 1a18b8d56d73 src/hotspot/share/jvmci/jvmciCodeInstaller.cpp
>> --- a/src/hotspot/share/jvmci/jvmciCodeInstaller.cpp    Sat Mar 16 15:05:21 2019 -0700
>> +++ b/src/hotspot/share/jvmci/jvmciCodeInstaller.cpp    Sun Mar 17 17:06:50 2019 +0800
>> @@ -623,7 +623,7 @@
>> #endif // INCLUDE_AOT
>>   // constructor used to create a method
>> -JVMCIEnv::CodeInstallResult CodeInstaller::install(JVMCICompiler* compiler, Handle target, Handle compiled_code, CodeBlob*& cb, Handle installed_code, Handle speculation_log, TRAPS) {
>> +JVMCIEnv::CodeInstallResult CodeInstaller::install(JVMCICompiler* compiler, Handle target, Handle compiled_code, CodeBlob*& cb, char*& cb_name, Handle installed_code, Handle speculation_log, TRAPS) {
>>    CodeBuffer buffer("JVMCI Compiler CodeBuffer");
>>    jobject compiled_code_obj = JNIHandles::make_local(compiled_code());
>>    OopRecorder* recorder = new OopRecorder(&_arena, true);
>> @@ -649,8 +649,8 @@
>>      if (stubName == NULL) {
>>        JVMCI_ERROR_OK("stub should have a name");
>>      }
>> -    char* name = strdup(java_lang_String::as_utf8_string(stubName));
>> -    cb = RuntimeStub::new_runtime_stub(name,
>> +    cb_name = strdup(java_lang_String::as_utf8_string(stubName));
>> +    cb = RuntimeStub::new_runtime_stub(cb_name,
>>                                         &buffer,
>>                                         CodeOffsets::frame_never_safe,
>>                                         stack_slots,
>> diff -r 1a18b8d56d73 src/hotspot/share/jvmci/jvmciCodeInstaller.hpp
>> --- a/src/hotspot/share/jvmci/jvmciCodeInstaller.hpp    Sat Mar 16 15:05:21 2019 -0700
>> +++ b/src/hotspot/share/jvmci/jvmciCodeInstaller.hpp    Sun Mar 17 17:06:50 2019 +0800
>> @@ -207,7 +207,7 @@
>> #if INCLUDE_AOT
>>    JVMCIEnv::CodeInstallResult gather_metadata(Handle target, Handle compiled_code, CodeMetadata& metadata, TRAPS);
>> #endif
>> -  JVMCIEnv::CodeInstallResult install(JVMCICompiler* compiler, Handle target, Handle compiled_code, CodeBlob*& cb, Handle installed_code, Handle speculation_log, TRAPS);
>> +  JVMCIEnv::CodeInstallResult install(JVMCICompiler* compiler, Handle target, Handle compiled_code, CodeBlob*& cb, char*& cb_name, Handle installed_code, Handle speculation_log, TRAPS);
>>     static address runtime_call_target_address(oop runtime_call);
>>    static VMReg get_hotspot_reg(jint jvmciRegisterNumber, TRAPS);
>> diff -r 1a18b8d56d73 src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
>> --- a/src/hotspot/share/jvmci/jvmciCompilerToVM.cpp     Sat Mar 16 15:05:21 2019 -0700
>> +++ b/src/hotspot/share/jvmci/jvmciCompilerToVM.cpp     Sun Mar 17 17:06:50 2019 +0800
>> @@ -677,6 +677,7 @@
>>    Handle target_handle(THREAD, JNIHandles::resolve(target));
>>    Handle compiled_code_handle(THREAD, JNIHandles::resolve(compiled_code));
>>    CodeBlob* cb = NULL;
>> +  char* cb_name = NULL;
>>    Handle installed_code_handle(THREAD, JNIHandles::resolve(installed_code));
>>    Handle speculation_log_handle(THREAD, JNIHandles::resolve(speculation_log));
>> @@ -685,7 +686,7 @@
>>    TraceTime install_time("installCode", JVMCICompiler::codeInstallTimer());
>>    bool is_immutable_PIC = HotSpotCompiledCode::isImmutablePIC(compiled_code_handle) > 0;
>>    CodeInstaller installer(is_immutable_PIC);
>> -  JVMCIEnv::CodeInstallResult result = installer.install(compiler, target_handle, compiled_code_handle, cb, installed_code_handle, speculation_log_handle, CHECK_0);
>> +  JVMCIEnv::CodeInstallResult result = installer.install(compiler, target_handle, compiled_code_handle, cb, cb_name, installed_code_handle, speculation_log_handle, CHECK_0);
>>     if (PrintCodeCacheOnCompilation) {
>>      stringStream s;
>> @@ -722,6 +723,7 @@
>>        }
>>      }
>>    }
>> +  if (cb_name) free(cb_name);
>>    return result;
>> C2V_END
>>
>> ----- 8< -------- 8< -------- 8< -------- 8< -------- 8< -------- 8< ---
>>
>>
>> I ran clang static analyzer again, and it is not reproducible owing to I fixed the issue, not False negative :)
>>
>> hotspot:tier1 linux-x86_64-server-fastdebug 2 fail:
>>
>> * compiler/c2/Test8062950.java: it is also reproducible for mips64el without the patch
>> * runtime/classFileParserBug/TestEmptyBootstrapMethodsAttr.java: Test empty bootstrap_methods table within BootstrapMethods attribute
>>
>>
>> Please point out my any fault!
>>
>> Thanks,
>>
>> Leslie Zhai
>>
>> [1] https://raw.githubusercontent.com/xiangzhai/jdk-dev/master/jvmciCodeInstaller.cpp.png
>>
>> [2] https://bugs.llvm.org/show_bug.cgi?id=40913

Potential Use-after-free issue reported by clang static analyzer.

>>
>> [3] https://mail.openjdk.java.net/pipermail/jdk8u-dev/2018-September/007855.html
>>
>>


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Request for Comments: Potential leak of memory pointed to by 'name' about jvmciCodeInstaller

David Blaikie via cfe-dev


On 18 Mar 2019, at 03:55, Leslie Zhai <[hidden email]> wrote:

Hi Doug,

Thanks for your kind response!


在 2019年03月18日 05:55, Doug Simon 写道:
Hi Leslie,

As a point of process, I think [hidden email] is probably a better list for JVMCI bugs.

Sorry for my wrong posting!


In any case, thanks for the investigation. However, I don’t think this is a bug as RuntimeStub simply passes along the name argument which is eventually stored to CodeBlob::_name without any further copying. If we subsequently freed that argument, the CodeBlob::_name would become invalid.

Thanks for pointing out my fault!
I might brought in Use-after-free[2] issue even that I carefully free the allocated memory in the end of `installCode` C2V_VMENTRY...
The reduced testcase is able to reproduce my fault:

----- 8< -------- 8< -------- 8< -------- 8< -------- 8< -------- 8< ---
$ cat t.cpp
#include <iostream>
#include <string.h>
#include <stdlib.h>

class CodeBlob {
public:
 CodeBlob(const char* name) : _name(name) {};

 const char* name() { return _name; }

protected:
 const char* _name;
};

class RuntimeBlob : public CodeBlob {
public:
 RuntimeBlob(const char* name) : CodeBlob(name) {}
};

class RuntimeStub : public RuntimeBlob {
private:
 RuntimeStub(const char* name) : RuntimeBlob(name) {}

public:
 static RuntimeStub* new_runtime_stub(const char* stub_name) {
   RuntimeStub* stub = new RuntimeStub(stub_name);
   return stub;
 }
};

static void install(CodeBlob*& cb, char*& name) {
 name = strdup("some stubName");
 cb = RuntimeStub::new_runtime_stub(name);
}

// To simulate  C2V_VMENTRY(jint, installCode, (JNIEnv *jniEnv, jobject, jobject target, jobject compiled_code, jobject installed_code, jobject speculation_log))
int main(int argc, char *argv[]) {
 CodeBlob* cb = NULL;
 char* cb_name = NULL;
 install(cb, cb_name);
 if (cb_name) free(cb_name);  // <--- MY FAULT
 std::cout << cb->name() << std::endl;
 return 0;
}
----- 8< -------- 8< -------- 8< -------- 8< -------- 8< -------- 8< ---

t.cpp:9:24: warning: Use of memory after it is freed
 const char* name() { return _name; }
                      ^~~~~~~~~~~~
t.cpp:42:3: warning: Potential leak of memory pointed to by 'cb'
 std::cout << cb->name() << std::endl;
 ^~~~~~~~~~~~~~~~~~~~~~~


So It might be Potential leak of memory pointed to by 'cb' about jvmciCompilerToVM?  But the `cb` might be used in other place just like `name` :)

Indeed. RuntimeStubs are never freed as they are used as helpers for “normal” compiled code. Once created, their addresses are stored in static variables that live for the remainder of the VM process.

-Doug

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev