<AWT Dev> Review Request: JDK-8179014 JFileChooser with Windows look and feel crashes on win 10 + jre-8 131

[Phil Race]

OK +1 to this fix. Please follow RDP2 procedures to request this fix in 9 ..

-phil.


On 05/04/2017 02:55 AM, Prem Balakrishnan wrote:
>
> Thanks Kevin and Phil for the review.
>
> I shouldn't have missed the 'break' statement. Mistake on my part :(
>
> The crash fix null check is in thrid switch case.
>
> As a good practice of making the method robust, I added the null check 
> in first switch case as well.
>
> The null checks have been added in method:
>
> static jstring jstringFromSTRRET(JNIEnv* env, LPITEMIDLIST pidl, 
> STRRET* pStrret)
>
> This method is invoked from below methods in ShellFolder2.cpp
>
> getDisplayNameOf()
>
> doGetColumnValue()
>
> createColumnInfo()
>
> The crash was due to function call getDisplayNameOf() --- which gets 
> fixed if we add null check only in third swich case.
>
> The null check in first switch case is more of making the code robust 
> against possible crash.
>
> I examined calls to these native methods getDisplayNameOf(), 
> doGetColumnValue() & createColumnInfo() -
>
> Only getDisplayNameOf() return has null checks on java side. There are 
> no checks on java side for string being null for other two methods.
>
> These two methods are invoked from FilePane.java class, which is used 
> only in XXXXFileChooserUI classes.
>
> I haven't seen any side effect in my jtreg test runs of JFileChooser 
> with this fix.
>
> The null check for first switch statement never existed and there were 
> no crashes reported. What I have done is more of preventive check.
>
> Request you to review.
>
> http://cr.openjdk.java.net/~pkbalakr/8179014/webrev.01/ 
> <http://cr.openjdk.java.net/%7Epkbalakr/8179014/webrev.01/>
>
> Regards,
>
> Prem
>
> *From:*Phil Race
> *Sent:* Saturday, April 29, 2017 11:00 PM
> *To:* Kevin Rushforth
> *Cc:* Prem Balakrishnan; Alexander Scherbatiy; Sergey Bylokhov; 
> awt-dev at openjdk.java.net <mailto:awt-dev at openjdk.java.net>
> *Subject:* Re: <AWT Dev> Review Request: JDK-8179014 JFileChooser with 
> Windows look and feel crashes on win 10 + jre-8 131
>
> I agree. This switch statement clearly was written in the expectation 
> that each case executed return.
>
> And as I said offline pls confirm that you are confident that all 
> return paths actually handle that null which may not have been tested 
> in practice before now. There seemed to be several code paths.
>
> -Phil.
>
>
> On Apr 29, 2017, at 5:33 AM, Kevin Rushforth 
> <kevin.rushforth at oracle.com <mailto:kevin.rushforth at oracle.com>> wrote:
>
>     I'm not a "R"eviewer for AWT, but this part of the change looks
>     wrong to me:
>
>               case STRRET_CSTR :
>
>     +            if (pStrret->cStr != NULL) {
>
>                   return JNU_NewStringPlatform(env, reinterpret_cast<const char*>(pStrret->cStr));
>
>     +            }
>
>               case STRRET_OFFSET :
>
>
>
>     Did you really intended that a NULL pStrret->cStr pointer should
>     fall through to the next case (STRRET_OFFSET)? If so, then a
>     comment is in order because this seems odd.
>
>     -- Kevin
>
>
>     Prem Balakrishnan wrote:
>
>     Hi*,*
>
>     Please review fix for JDK 9,
>
>     *Bug:*https://bugs.openjdk.java.net/browse/JDK-8179014
>
>     *Webrev:*http://cr.openjdk.java.net/~pkbalakr/8179014/webrev.00/
>     <http://cr.openjdk.java.net/%7Epkbalakr/8179014/webrev.00/>
>
>     *Issue:*
>
>     JFileChooser with Windows LAF crashes on Windows 10.
>
>     This issue is reproducible only on Windows 10 (v1703).
>
>     Creating GodMode directory is the only way we are able to
>     reproduce this issue.
>
>     *Cause:*
>
>     IShellFolder::GetDisplayNameOf() method is used to retrieve the
>     display name of folder/subfolder using the GUID.
>
>     This method  returns NULL for GodMode directory [i.e., folder
>     created with name: GodMode.{ED7BA470-8E54-465E-825C-99712043E01C}].
>
>     In previous version of windows (tested on v1511) this call returns
>     name “GodMode”.
>
>     Hence we get crash when processing the return data from the this
>     method.
>
>     Why only on Windows LAF?
>
>     Unlike other LAFs in Windows LAF “useSystemExtensionHiding” is set
>     to true.
>
>     This check avoids the call to IShellFolder::GetDisplayNameOf()
>     method in all other LAF’s.
>
>     Hence crash is not reported on other LAFs.
>
>     *Fix:*
>
>     Added missing NULL checks in native method.
>
>     This results in native method returning NULL in this case, which
>     has already been handled on java side, hence no additional change
>     is needed.
>
>     Regards,
>
>     Prem
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/awt-dev/attachments/20170505/ce2eb032/attachment-0001.html>