[PATCH] Improve Windows Minidump File Specification

[David Holmes]

On 30/07/2013 1:02 PM, Matthew Briggs wrote:
> 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.

That was a Review id, the actual bug is 8021940.

Thanks,
David

> - 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)")    \
>>>
>>>        \
>>>
>>