RFR: 8289778: ZGC: incorrect use of os::free() for mountpoint string handling after JDK-8289633

[Thomas Stuefe]

On Wed, 6 Jul 2022 02:17:39 GMT, Jie Fu <jiefu at openjdk.org> wrote:

> Hi all,
> 
> ZGC crashes were observed by us after JDK-8289633 due to incorrect use of `os::free()` for mountpoint string handling.
> 
> For example, `line_mountpoint` and `line_filesystem` will be allocated by `sscanf` @line60. 
> And `line` will be allocated by `getline` @line84.
> 
> 
>  53 char* ZMountPoint::get_mountpoint(const char* line, const char* filesystem) const {
>  54   char* line_mountpoint = NULL;
>  55   char* line_filesystem = NULL;
>  56 
>  57   // Parse line and return a newly allocated string containing the mount point if
>  58   // the line contains a matching filesystem and the mount point is accessible by
>  59   // the current user.
>  60   if (sscanf(line, "%*u %*u %*u:%*u %*s %ms %*[^-]- %ms", &line_mountpoint, &line_filesystem) != 2 ||
>  61       strcmp(line_filesystem, filesystem) != 0 ||
>  62       access(line_mountpoint, R_OK|W_OK|X_OK) != 0) {
>  63     // Not a matching or accessible filesystem
>  64     os::free(line_mountpoint);
>  65     line_mountpoint = NULL;
>  66   }
>  67 
>  68   os::free(line_filesystem);
>  69 
>  70   return line_mountpoint;
>  71 }
>  72 
>  73 void ZMountPoint::get_mountpoints(const char* filesystem, ZArray<char*>* mountpoints) const {
>  74   FILE* fd = os::fopen(PROC_SELF_MOUNTINFO, "r");
>  75   if (fd == NULL) {
>  76     ZErrno err;
>  77     log_error_p(gc)("Failed to open %s: %s", PROC_SELF_MOUNTINFO, err.to_string());
>  78     return;
>  79   }
>  80 
>  81   char* line = NULL;
>  82   size_t length = 0;
>  83 
>  84   while (getline(&line, &length, fd) != -1) {
>  85     char* const mountpoint = get_mountpoint(line, filesystem);
>  86     if (mountpoint != NULL) {
>  87       mountpoints->append(mountpoint);
>  88     }
>  89   }
>  90 
>  91   os::free(line);
>  92   fclose(fd);
>  93 }
> 
> 
> See the anaylis of the crash reason in https://bugs.openjdk.org/browse/JDK-8289477
> 
> That means we have raw `::malloc() -> os::free()`, which is unbalanced.
> Raw `::malloc()` does not write the header `os::free()` expects.
> If NMT is on, we assert now, because NMT does not find its header in os::free().
> 
> 
> The fix just reverts `os::free()` to `::free()`.
> 
> Testing:
>   - hotspot/jtreg/gc/z on Linux/x64, all passed
> 
> Thanks.
> Best regards,
> Jie

Hi  @DamonFool ,

thanks for the quick fix. Embarrassing, I should have catched this.

Can you please add `#include "utilities/globalDefinitions.hpp"` for the ALLOW_... macros?

Otherwise, apart from the comment changes below, fine. 

Cheers, Thomas

src/hotspot/os/linux/gc/z/zMountPoint_linux.cpp line 64:

> 62:       access(line_mountpoint, R_OK|W_OK|X_OK) != 0) {
> 63:     // Not a matching or accessible filesystem
> 64:     ALLOW_C_FUNCTION(::free, ::free(line_mountpoint);) // *not* os::free

Can you please extend the comment like this: "sscanf, using %m, will return malloce'd memory. Needs raw ::free, not os::free."

src/hotspot/os/linux/gc/z/zMountPoint_linux.cpp line 91:

> 89:   }
> 90: 
> 91:   ALLOW_C_FUNCTION(::free, ::free(line);) // *not* os::free

Please extend comment: "readline will return malloc'd memory. Needs raw ::free, not os::free."

-------------

Changes requested by stuefe (Reviewer).

PR: https://git.openjdk.org/jdk/pull/9387