[PATCH] Improve Windows Minidump File Specification

[Matthew Briggs]

Hello David,

Thank you very much for the follow up.  Per your response, I've:

- Signed the Oracle Contributor Agreement and emailed a scanned image
of my completed form to oracle-ca_us at oracle.com

- For tracking purposes, I've created a new feature request in the Bug
Database (http://bugs.sun.com/bugdatabase/).  The Bug ID is 9005569.

- I realized my original patch posting didn't address some
alternatives I had considered, so I added some information about that
to the feature request as well.

Thanks again!
Matt

On Mon, Jul 29, 2013 at 12:22 AM, David Holmes <david.holmes at oracle.com> wrote:
> 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)")    \
>>
>>       \
>>
>