RFR: 8262829: Native crash in Win32PrintServiceLookup.getAllPrinterNames()

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8262829: Native crash in Win32PrintServiceLookup.getAllPrinterNames()

Alexey Ivanov-2
**Root cause:**
`getPrinterNames()` has two calls to [`::EnumPrinters`](https://docs.microsoft.com/en-us/windows/win32/printdocs/enumprinters). The first call is to get the required buffer size to contain the structures and any strings. The second call is to get the list of printers.

Yet the list of printers or the names of printers can change between the two calls. If it happens, the second call to `EnumPrinters` fails but it is not checked.

I couldn't reproduce the crash myself. However, I reproduced the failure in the second call to `::EnumPrinters` by adding `::Sleep(500)` and by renaming the printers so that the data doesn't fit into the allocated buffer:

1: bResult: 0, cbNeeded: 504, cReturned: 0
2: bResult: 0, cbNeeded: 512, cReturned: 0
    !!! error

During my testing, `cReturned` has always been zero whenever `EnumPrinters` fails.

The crash dumps show that `cReturned` is non-zero but the `pPrinterEnum` buffer doesn't contain valid data. Reading `info4->pPrinterName` at the line
utf_str = JNU_NewStringPlatform(env, info4->pPrinterName);
raises Access Violation exception.

**The fix:**
Check the return value of `EnumPrinters` and allow for 5 retries. If the function still returns failure, make sure `cReturned` is set to zero.

I'm going to close [JDK-6996051](https://bugs.openjdk.java.net/browse/JDK-6996051) and [JDK-8182683](https://bugs.openjdk.java.net/browse/JDK-8182683) reported previously about the same crash as duplicate of the current [JDK-8262829](https://bugs.openjdk.java.net/browse/JDK-8262829).

**Testing:**
I used [`RemotePrinterStatusRefresh.java`](https://github.com/openjdk/jdk/blob/master/test/jdk/java/awt/print/RemotePrinterStatusRefresh/RemotePrinterStatusRefresh.java) for testing, it shows the list of currently available printers. In the background I ran `PrinterRename.ps1` PowerShell script which remains a printer, the script is attached to the JBS issue.

Without the fix, the list of available printers was empty occasionally because `EnumPrinters` returned failure and set `cReturned` to zero. With the fix, the list of printers is always filled.

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

Commit messages:
 - 8262829: Native crash in Win32PrintServiceLookup.getAllPrinterNames()

Changes: https://git.openjdk.java.net/jdk/pull/2836/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2836&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8262829
  Stats: 20 lines in 1 file changed: 13 ins; 0 del; 7 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2836.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2836/head:pull/2836

PR: https://git.openjdk.java.net/jdk/pull/2836
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262829: Native crash in Win32PrintServiceLookup.getAllPrinterNames()

Phil Race
On Thu, 4 Mar 2021 21:37:33 GMT, Alexey Ivanov <[hidden email]> wrote:

> **Root cause:**
> `getPrinterNames()` has two calls to [`::EnumPrinters`](https://docs.microsoft.com/en-us/windows/win32/printdocs/enumprinters). The first call is to get the required buffer size to contain the structures and any strings. The second call is to get the list of printers.
>
> Yet the list of printers or the names of printers can change between the two calls. If it happens, the second call to `EnumPrinters` fails but it is not checked.
>
> I couldn't reproduce the crash myself. However, I reproduced the failure in the second call to `::EnumPrinters` by adding `::Sleep(500)` and by renaming the printers so that the data doesn't fit into the allocated buffer:
>
> 1: bResult: 0, cbNeeded: 504, cReturned: 0
> 2: bResult: 0, cbNeeded: 512, cReturned: 0
>     !!! error
>
> During my testing, `cReturned` has always been zero whenever `EnumPrinters` fails.
>
> The crash dumps show that `cReturned` is non-zero but the `pPrinterEnum` buffer doesn't contain valid data. Reading `info4->pPrinterName` at the line
> utf_str = JNU_NewStringPlatform(env, info4->pPrinterName);
> raises Access Violation exception.
>
> **The fix:**
> Check the return value of `EnumPrinters` and allow for 5 retries. If the function still returns failure, make sure `cReturned` is set to zero.
>
> I'm going to close [JDK-6996051](https://bugs.openjdk.java.net/browse/JDK-6996051) and [JDK-8182683](https://bugs.openjdk.java.net/browse/JDK-8182683) reported previously about the same crash as duplicate of the current [JDK-8262829](https://bugs.openjdk.java.net/browse/JDK-8262829).
>
> **Testing:**
> I used [`RemotePrinterStatusRefresh.java`](https://github.com/openjdk/jdk/blob/master/test/jdk/java/awt/print/RemotePrinterStatusRefresh/RemotePrinterStatusRefresh.java) for testing, it shows the list of currently available printers. In the background I ran `PrinterRename.ps1` PowerShell script which remains a printer, the script is attached to the JBS issue.
>
> Without the fix, the list of available printers was empty occasionally because `EnumPrinters` returned failure and set `cReturned` to zero. With the fix, the list of printers is always filled.

I guess this is OK and yes we should have been checking this.
Not sure we really got to the bottom of the real world problem because I'd expect the 2nd call just milliseconds after the first. But it could be that this happens during some system updates and we get one printer reconfigured message followed by another ..

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

Marked as reviewed by prr (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2836
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262829: Native crash in Win32PrintServiceLookup.getAllPrinterNames()

Sergey Bylokhov-2
In reply to this post by Alexey Ivanov-2
On Thu, 4 Mar 2021 21:37:33 GMT, Alexey Ivanov <[hidden email]> wrote:

> **Root cause:**
> `getPrinterNames()` has two calls to [`::EnumPrinters`](https://docs.microsoft.com/en-us/windows/win32/printdocs/enumprinters). The first call is to get the required buffer size to contain the structures and any strings. The second call is to get the list of printers.
>
> Yet the list of printers or the names of printers can change between the two calls. If it happens, the second call to `EnumPrinters` fails but it is not checked.
>
> I couldn't reproduce the crash myself. However, I reproduced the failure in the second call to `::EnumPrinters` by adding `::Sleep(500)` and by renaming the printers so that the data doesn't fit into the allocated buffer:
>
> 1: bResult: 0, cbNeeded: 504, cReturned: 0
> 2: bResult: 0, cbNeeded: 512, cReturned: 0
>     !!! error
>
> During my testing, `cReturned` has always been zero whenever `EnumPrinters` fails.
>
> The crash dumps show that `cReturned` is non-zero but the `pPrinterEnum` buffer doesn't contain valid data. Reading `info4->pPrinterName` at the line
> utf_str = JNU_NewStringPlatform(env, info4->pPrinterName);
> raises Access Violation exception.
>
> **The fix:**
> Check the return value of `EnumPrinters` and allow for 5 retries. If the function still returns failure, make sure `cReturned` is set to zero.
>
> I'm going to close [JDK-6996051](https://bugs.openjdk.java.net/browse/JDK-6996051) and [JDK-8182683](https://bugs.openjdk.java.net/browse/JDK-8182683) reported previously about the same crash as duplicate of the current [JDK-8262829](https://bugs.openjdk.java.net/browse/JDK-8262829).
>
> **Testing:**
> I used [`RemotePrinterStatusRefresh.java`](https://github.com/openjdk/jdk/blob/master/test/jdk/java/awt/print/RemotePrinterStatusRefresh/RemotePrinterStatusRefresh.java) for testing, it shows the list of currently available printers. In the background I ran `PrinterRename.ps1` PowerShell script which remains a printer, the script is attached to the JBS issue.
>
> Without the fix, the list of available printers was empty occasionally because `EnumPrinters` returned failure and set `cReturned` to zero. With the fix, the list of printers is always filled.

src/java.desktop/windows/native/libawt/windows/WPrinterJob.cpp line 136:

> 134:
> 135:     try {
> 136:         ::EnumPrinters(flags, NULL, 4, NULL,

Don't we need to check that this method call succeeds? Probably it is a crash reason when the EnumPrinters fails but we use cbNeeded anyway for array allocation?

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

PR: https://git.openjdk.java.net/jdk/pull/2836
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262829: Native crash in Win32PrintServiceLookup.getAllPrinterNames()

Prasanta Sadhukhan-2
On Fri, 5 Mar 2021 01:27:47 GMT, Sergey Bylokhov <[hidden email]> wrote:

>> **Root cause:**
>> `getPrinterNames()` has two calls to [`::EnumPrinters`](https://docs.microsoft.com/en-us/windows/win32/printdocs/enumprinters). The first call is to get the required buffer size to contain the structures and any strings. The second call is to get the list of printers.
>>
>> Yet the list of printers or the names of printers can change between the two calls. If it happens, the second call to `EnumPrinters` fails but it is not checked.
>>
>> I couldn't reproduce the crash myself. However, I reproduced the failure in the second call to `::EnumPrinters` by adding `::Sleep(500)` and by renaming the printers so that the data doesn't fit into the allocated buffer:
>>
>> 1: bResult: 0, cbNeeded: 504, cReturned: 0
>> 2: bResult: 0, cbNeeded: 512, cReturned: 0
>>     !!! error
>>
>> During my testing, `cReturned` has always been zero whenever `EnumPrinters` fails.
>>
>> The crash dumps show that `cReturned` is non-zero but the `pPrinterEnum` buffer doesn't contain valid data. Reading `info4->pPrinterName` at the line
>> utf_str = JNU_NewStringPlatform(env, info4->pPrinterName);
>> raises Access Violation exception.
>>
>> **The fix:**
>> Check the return value of `EnumPrinters` and allow for 5 retries. If the function still returns failure, make sure `cReturned` is set to zero.
>>
>> I'm going to close [JDK-6996051](https://bugs.openjdk.java.net/browse/JDK-6996051) and [JDK-8182683](https://bugs.openjdk.java.net/browse/JDK-8182683) reported previously about the same crash as duplicate of the current [JDK-8262829](https://bugs.openjdk.java.net/browse/JDK-8262829).
>>
>> **Testing:**
>> I used [`RemotePrinterStatusRefresh.java`](https://github.com/openjdk/jdk/blob/master/test/jdk/java/awt/print/RemotePrinterStatusRefresh/RemotePrinterStatusRefresh.java) for testing, it shows the list of currently available printers. In the background I ran `PrinterRename.ps1` PowerShell script which remains a printer, the script is attached to the JBS issue.
>>
>> Without the fix, the list of available printers was empty occasionally because `EnumPrinters` returned failure and set `cReturned` to zero. With the fix, the list of printers is always filled.
>
> src/java.desktop/windows/native/libawt/windows/WPrinterJob.cpp line 136:
>
>> 134:
>> 135:     try {
>> 136:         ::EnumPrinters(flags, NULL, 4, NULL,
>
> Don't we need to check that this method call succeeds? Probably it is a crash reason when the EnumPrinters fails but we use cbNeeded anyway for array allocation?

I guess since we are changing this method anyway, can we use PRINTER_ENUM_CONNECTIONS flag instead of hardcoded 4 so that it is more informative!!

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

PR: https://git.openjdk.java.net/jdk/pull/2836
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262829: Native crash in Win32PrintServiceLookup.getAllPrinterNames()

Prasanta Sadhukhan-2
On Fri, 5 Mar 2021 03:42:29 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> src/java.desktop/windows/native/libawt/windows/WPrinterJob.cpp line 136:
>>
>>> 134:
>>> 135:     try {
>>> 136:         ::EnumPrinters(flags, NULL, 4, NULL,
>>
>> Don't we need to check that this method call succeeds? Probably it is a crash reason when the EnumPrinters fails but we use cbNeeded anyway for array allocation?
>
> I guess since we are changing this method anyway, can we use PRINTER_ENUM_CONNECTIONS flag instead of hardcoded 4 so that it is more informative!!

ok, it is for flags and not for level. please ignore

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

PR: https://git.openjdk.java.net/jdk/pull/2836
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262829: Native crash in Win32PrintServiceLookup.getAllPrinterNames()

Alexey Ivanov-2
On Fri, 5 Mar 2021 03:49:22 GMT, Prasanta Sadhukhan <[hidden email]> wrote:

>> I guess since we are changing this method anyway, can we use PRINTER_ENUM_CONNECTIONS flag instead of hardcoded 4 so that it is more informative!!
>
> ok, it is for flags and not for level. please ignore

>
>
> Don't we need to check that this method call succeeds? Probably it is a crash reason when the EnumPrinters fails but we use cbNeeded anyway for array allocation?

`EnumPrinters` always returns failure here because zero-sized buffer is too small to contain any data.

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

PR: https://git.openjdk.java.net/jdk/pull/2836
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262829: Native crash in Win32PrintServiceLookup.getAllPrinterNames()

Alexey Ivanov-2
In reply to this post by Phil Race
On Thu, 4 Mar 2021 22:09:55 GMT, Phil Race <[hidden email]> wrote:

> I guess this is OK and yes we should have been checking this.
> Not sure we really got to the bottom of the real world problem because I'd expect the 2nd call just milliseconds after the first. But it could be that this happens during some system updates and we get one printer reconfigured message followed by another ..

Most of the time, the crash happens when a client reconnects to their active session where a JVM is already running. On reconnect, the list of printers is synced with the local printers on the client. In such a scenario, other software could also update its list of printers as well as repaint its UI. If the system is under high load, it's not impossible to have a long enough pause between the calls.

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

PR: https://git.openjdk.java.net/jdk/pull/2836
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262829: Native crash in Win32PrintServiceLookup.getAllPrinterNames()

Prasanta Sadhukhan-2
In reply to this post by Alexey Ivanov-2
On Thu, 4 Mar 2021 21:37:33 GMT, Alexey Ivanov <[hidden email]> wrote:

> **Root cause:**
> `getPrinterNames()` has two calls to [`::EnumPrinters`](https://docs.microsoft.com/en-us/windows/win32/printdocs/enumprinters). The first call is to get the required buffer size to contain the structures and any strings. The second call is to get the list of printers.
>
> Yet the list of printers or the names of printers can change between the two calls. If it happens, the second call to `EnumPrinters` fails but it is not checked.
>
> I couldn't reproduce the crash myself. However, I reproduced the failure in the second call to `::EnumPrinters` by adding `::Sleep(500)` and by renaming the printers so that the data doesn't fit into the allocated buffer:
>
> 1: bResult: 0, cbNeeded: 504, cReturned: 0
> 2: bResult: 0, cbNeeded: 512, cReturned: 0
>     !!! error
>
> During my testing, `cReturned` has always been zero whenever `EnumPrinters` fails.
>
> The crash dumps show that `cReturned` is non-zero but the `pPrinterEnum` buffer doesn't contain valid data. Reading `info4->pPrinterName` at the line
> utf_str = JNU_NewStringPlatform(env, info4->pPrinterName);
> raises Access Violation exception.
>
> **The fix:**
> Check the return value of `EnumPrinters` and allow for 5 retries. If the function still returns failure, make sure `cReturned` is set to zero.
>
> I'm going to close [JDK-6996051](https://bugs.openjdk.java.net/browse/JDK-6996051) and [JDK-8182683](https://bugs.openjdk.java.net/browse/JDK-8182683) reported previously about the same crash as duplicate of the current [JDK-8262829](https://bugs.openjdk.java.net/browse/JDK-8262829).
>
> **Testing:**
> I used [`RemotePrinterStatusRefresh.java`](https://github.com/openjdk/jdk/blob/master/test/jdk/java/awt/print/RemotePrinterStatusRefresh/RemotePrinterStatusRefresh.java) for testing, it shows the list of currently available printers. In the background I ran `PrinterRename.ps1` PowerShell script which remains a printer, the script is attached to the JBS issue.
>
> Without the fix, the list of available printers was empty occasionally because `EnumPrinters` returned failure and set `cReturned` to zero. With the fix, the list of printers is always filled.

Marked as reviewed by psadhukhan (Reviewer).

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

PR: https://git.openjdk.java.net/jdk/pull/2836
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8262829: Native crash in Win32PrintServiceLookup.getAllPrinterNames()

Sergey Bylokhov-2
In reply to this post by Alexey Ivanov-2
On Thu, 4 Mar 2021 21:37:33 GMT, Alexey Ivanov <[hidden email]> wrote:

> **Root cause:**
> `getPrinterNames()` has two calls to [`::EnumPrinters`](https://docs.microsoft.com/en-us/windows/win32/printdocs/enumprinters). The first call is to get the required buffer size to contain the structures and any strings. The second call is to get the list of printers.
>
> Yet the list of printers or the names of printers can change between the two calls. If it happens, the second call to `EnumPrinters` fails but it is not checked.
>
> I couldn't reproduce the crash myself. However, I reproduced the failure in the second call to `::EnumPrinters` by adding `::Sleep(500)` and by renaming the printers so that the data doesn't fit into the allocated buffer:
>
> 1: bResult: 0, cbNeeded: 504, cReturned: 0
> 2: bResult: 0, cbNeeded: 512, cReturned: 0
>     !!! error
>
> During my testing, `cReturned` has always been zero whenever `EnumPrinters` fails.
>
> The crash dumps show that `cReturned` is non-zero but the `pPrinterEnum` buffer doesn't contain valid data. Reading `info4->pPrinterName` at the line
> utf_str = JNU_NewStringPlatform(env, info4->pPrinterName);
> raises Access Violation exception.
>
> **The fix:**
> Check the return value of `EnumPrinters` and allow for 5 retries. If the function still returns failure, make sure `cReturned` is set to zero.
>
> I'm going to close [JDK-6996051](https://bugs.openjdk.java.net/browse/JDK-6996051) and [JDK-8182683](https://bugs.openjdk.java.net/browse/JDK-8182683) reported previously about the same crash as duplicate of the current [JDK-8262829](https://bugs.openjdk.java.net/browse/JDK-8262829).
>
> **Testing:**
> I used [`RemotePrinterStatusRefresh.java`](https://github.com/openjdk/jdk/blob/master/test/jdk/java/awt/print/RemotePrinterStatusRefresh/RemotePrinterStatusRefresh.java) for testing, it shows the list of currently available printers. In the background I ran `PrinterRename.ps1` PowerShell script which remains a printer, the script is attached to the JBS issue.
>
> Without the fix, the list of available printers was empty occasionally because `EnumPrinters` returned failure and set `cReturned` to zero. With the fix, the list of printers is always filled.

Marked as reviewed by serb (Reviewer).

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

PR: https://git.openjdk.java.net/jdk/pull/2836
Reply | Threaded
Open this post in threaded view
|

Integrated: 8262829: Native crash in Win32PrintServiceLookup.getAllPrinterNames()

Alexey Ivanov-2
In reply to this post by Alexey Ivanov-2
On Thu, 4 Mar 2021 21:37:33 GMT, Alexey Ivanov <[hidden email]> wrote:

> **Root cause:**
> `getPrinterNames()` has two calls to [`::EnumPrinters`](https://docs.microsoft.com/en-us/windows/win32/printdocs/enumprinters). The first call is to get the required buffer size to contain the structures and any strings. The second call is to get the list of printers.
>
> Yet the list of printers or the names of printers can change between the two calls. If it happens, the second call to `EnumPrinters` fails but it is not checked.
>
> I couldn't reproduce the crash myself. However, I reproduced the failure in the second call to `::EnumPrinters` by adding `::Sleep(500)` and by renaming the printers so that the data doesn't fit into the allocated buffer:
>
> 1: bResult: 0, cbNeeded: 504, cReturned: 0
> 2: bResult: 0, cbNeeded: 512, cReturned: 0
>     !!! error
>
> During my testing, `cReturned` has always been zero whenever `EnumPrinters` fails.
>
> The crash dumps show that `cReturned` is non-zero but the `pPrinterEnum` buffer doesn't contain valid data. Reading `info4->pPrinterName` at the line
> utf_str = JNU_NewStringPlatform(env, info4->pPrinterName);
> raises Access Violation exception.
>
> **The fix:**
> Check the return value of `EnumPrinters` and allow for 5 retries. If the function still returns failure, make sure `cReturned` is set to zero.
>
> I'm going to close [JDK-6996051](https://bugs.openjdk.java.net/browse/JDK-6996051) and [JDK-8182683](https://bugs.openjdk.java.net/browse/JDK-8182683) reported previously about the same crash as duplicate of the current [JDK-8262829](https://bugs.openjdk.java.net/browse/JDK-8262829).
>
> **Testing:**
> I used [`RemotePrinterStatusRefresh.java`](https://github.com/openjdk/jdk/blob/master/test/jdk/java/awt/print/RemotePrinterStatusRefresh/RemotePrinterStatusRefresh.java) for testing, it shows the list of currently available printers. In the background I ran `PrinterRename.ps1` PowerShell script which remains a printer, the script is attached to the JBS issue.
>
> Without the fix, the list of available printers was empty occasionally because `EnumPrinters` returned failure and set `cReturned` to zero. With the fix, the list of printers is always filled.

This pull request has now been integrated.

Changeset: a6e34b3d
Author:    Alexey Ivanov <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/a6e34b3d
Stats:     20 lines in 1 file changed: 13 ins; 0 del; 7 mod

8262829: Native crash in Win32PrintServiceLookup.getAllPrinterNames()

Reviewed-by: prr, psadhukhan, serb

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

PR: https://git.openjdk.java.net/jdk/pull/2836