[PATCH] Improve Windows Minidump File Specification

[Matthew Briggs]

Interesting...  thanks for clarifying this.  I was a bit confused,
because the email I received from the Oracle Bug Report Daemon had the
subject line:

Your Report (Bug ID: 9005569 ) - Improve Windows Minidump File Specification

But when I attempted to go back in and search for this "bug id",
nothing was found.  Searching using the correct bug id that you
provided works much better :)

Thanks,
Matt


On Tue, Jul 30, 2013 at 10:55 PM, David Holmes <david.holmes at oracle.com> wrote:
> 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)")    \
>>>>
>>>>        \
>>>>
>>>
>