[PATCH] Improve Windows Minidump File Specification

[David Holmes]

Hi Matthew,

Welcome!

The contribution process is described here:

http://openjdk.java.net/contribute/

First you need to sign the OCA:

http://www.oracle.com/technetwork/oca-405177.pdf

Meanwhile someone will hopefully be able to review this and sponsor it 
for you if deemed suitable.

Thanks,
David

On 29/07/2013 1:50 AM, Matthew Briggs wrote:
> Hello,
>
> In running a Java application in a Windows Service context, minidump
> files have proven helpful in diagnosing crashes due to faulty native
> code (invoked through JNI).  In production contexts I'm familiar with,
> these minidumps are often not produced, though, due to a combination of
> configuration and limitations in the JVM's minidump location choices.
>   Specifically, the JVM only allows for minidump files to be created in
> the working directory of the Windows Service process.  A typical
> production deployment runs the Windows Service under the limited NETWORK
> SERVICE account, which in particular cannot write to its working
> directory (usually located under C:\Program Files).
>
> The included patch establishes a new -XX:MinidumpFile JVM option, in the
> spirit of the existing -XX:ErrorFile option.  The handling code for the
> latter is adapted to maintain similar structure and functionality.  A
> Windows minidump file may thus be specified by a user (including support
> for %p syntax), falling back to a default filename targeted in the
> process working directory and then an OS temporary directory.
>
> I've manually tested the patch using a simple Java application that
> intentionally causes an access violation through JNI invocation of
> native code.  I've tested explicit user specification of
> -XX:MinidumpFile, with and without %p,  along with the two fallback cases.
>
> This is my first interaction with the OpenJDK community, so I apologize
> in advance if I've failed to follow proper patch submission guidelines
> and procedures.  I'm eager to learn more about how the community
> operates, and to hear if this patch in particular is of interest.  Any
> feedback is appreciated.
>
> Thanks,
> Matt
>
> diff -r 9d7b55c8a0c4 src/os/windows/vm/os_windows.cpp
> --- a/src/os/windows/vm/os_windows.cppThu Jul 25 03:18:31 2013 -0700
> +++ b/src/os/windows/vm/os_windows.cppSun Jul 28 14:50:32 2013 +0100
> @@ -934,6 +934,55 @@
>     }
>   }
> +/** Expand a pattern into a buffer starting at pos and create a file
> using constructed path */
> +static HANDLE expand_and_create(const char* pattern, char* buf, size_t
> buflen, size_t pos) {
> +  HANDLE file = INVALID_HANDLE_VALUE;
> +  if (Arguments::copy_expand_pid(pattern, strlen(pattern), &buf[pos],
> buflen - pos)) {
> +file = CreateFile(buf, GENERIC_WRITE, 0, NULL, CREATE_ALWAYS,
> FILE_ATTRIBUTE_NORMAL, NULL);
> +  }
> +  return file;
> +}
> +
> +/**
> + * Construct file name for a minidump file and return it's file handle.
> + * Name and location depends on pattern, default_pattern params and access
> + * permissions.
> + */
> +static HANDLE prepare_minidump_file(const char* pattern, const char*
> default_pattern, char* buf, size_t buflen) {
> +  HANDLE file = INVALID_HANDLE_VALUE;
> +
> +  // If possible, use specified pattern to construct file name
> +  if (pattern != NULL) {
> +    file = expand_and_create(pattern, buf, buflen, 0);
> +  }
> +
> +  // Either user didn't specify, or the user's location failed,
> +  // so use the default name in the current directory
> +  if (file == INVALID_HANDLE_VALUE) {
> +    const char* cwd = os::get_current_directory(buf, buflen);
> +    if (cwd != NULL) {
> +      size_t pos = strlen(cwd);
> +      int fsep_len = jio_snprintf(&buf[pos], buflen-pos, "%s",
> os::file_separator());
> +      pos += fsep_len;
> +      if (fsep_len > 0) {
> +        file = expand_and_create(default_pattern, buf, buflen, pos);
> +      }
> +    }
> +  }
> +
> +  // try temp directory if it exists.
> +  if (file == INVALID_HANDLE_VALUE) {
> +    const char* tmpdir = os::get_temp_directory();
> +    if (tmpdir != NULL && strlen(tmpdir) > 0) {
> +      int pos = jio_snprintf(buf, buflen, "%s%s", tmpdir,
> os::file_separator());
> +      if (pos > 0) {
> +        file = expand_and_create(default_pattern, buf, buflen, pos);
> +      }
> +    }
> +  }
> +
> +  return file;
> +}
>   static BOOL  (WINAPI *_MiniDumpWriteDump)  ( HANDLE, DWORD, HANDLE,
> MINIDUMP_TYPE, PMINIDUMP_EXCEPTION_INFORMATION,
>
>   PMINIDUMP_USER_STREAM_INFORMATION, PMINIDUMP_CALLBACK_INFORMATION);
> @@ -948,7 +997,6 @@
>     DWORD processId = GetCurrentProcessId();
>     HANDLE dumpFile;
>     MINIDUMP_TYPE dumpType;
> -  static const char* cwd;
>   // Default is to always create dump for debug builds, on product
> builds only dump on server versions of Windows.
>   #ifndef ASSERT
> @@ -994,10 +1042,7 @@
>       MiniDumpWithUnloadedModules);
>   #endif
> -  cwd = get_current_directory(NULL, 0);
> -  jio_snprintf(buffer, bufferSize, "%s\\hs_err_pid%u.mdmp",cwd,
> current_process_id());
> -  dumpFile = CreateFile(buffer, GENERIC_WRITE, 0, NULL, CREATE_ALWAYS,
> FILE_ATTRIBUTE_NORMAL, NULL);
> -
> +  dumpFile = prepare_minidump_file(MinidumpFile, "hs_err_pid%p.mdmp",
> buffer, bufferSize);
>     if (dumpFile == INVALID_HANDLE_VALUE) {
>       VMError::report_coredump_status("Failed to create file for
> dumping", false);
>       return;
> diff -r 9d7b55c8a0c4 src/share/vm/runtime/globals.hpp
> --- a/src/share/vm/runtime/globals.hppThu Jul 25 03:18:31 2013 -0700
> +++ b/src/share/vm/runtime/globals.hppSun Jul 28 14:50:32 2013 +0100
> @@ -825,6 +825,10 @@
>     product(bool, CreateMinidumpOnCrash, false,
>        \
>             "Create minidump on VM fatal error")
>       \
>
>       \
> +  product(ccstr, MinidumpFile, NULL,
>       \
> +         "If a minidump is created, save to this file"
>       \
> +         "[default: ./hs_err_pid%p.mdmp] (%p replaced with pid)")
>      \
> +
>       \
>     product_pd(bool, UseOSErrorReporting,
>        \
>             "Let VM fatal error propagate to the OS (ie. WER on
> Windows)")    \
>
>       \
>