Re: Request for review: 7190897 (fs) Files.isWritable method returns false when the path is writable (win). - ver. 1

2013-03-04 Thread Alan Bateman 

On 04/03/2013 12:23, Alexey Utkin wrote:

:




We can have the situation with:
1) OpenThreadToken return 0 without exception - that is ok for thread 
without impersonation.
2) processTokenWithDuplicateAccess  is 0 for some reason (weak precess 
privileges)
For that case we have no access to process token without exception 
(hToken :=: 0).

boolean hasRight = false;
if (hToken != 0L) {

So, in upper line we need to check token for non-empty value.
! Do you concern about the [false] return value for that case?

try {
hasRight = AccessCheck(hToken, securityInfo, 
accessMask,
genericRead, genericWrite, genericExecute, 
genericAll);
Here is the actual work, that can make an exception (for example it 
happens for invalid SID)

} finally {
CloseHandle(hToken);

If token was open it have to be closed without excuses.

}
}
return hasRight;
}

implementation approach?
Can the handle to the token (hToken) be 0? In my comment I was 
suggesting:


long hToken = OpenThreadToken(...);
try {
...
} finally {
CloseHandle(hToken);
}

This mail is hard to read but I think the implementation is good.

-Alan.

Request for review: 7190897 (fs) Files.isWritable method returns false when the path is writable (win). - ver. 1

2013-03-01 Thread Alexey Utkin 

On 28.02.2013 19:41, Alan Bateman wrote:

On 28/02/2013 15:17, Alexey Utkin wrote:
That is not single, but 4 additional parameters ( FILE_GENERIC_READ, 
FILE_GENERIC_WRITE, FILE_GENERIC_EXECUTE, FILE_ALL_ACCESS) - that are 
relatively complicate masks. That parameters have to be changed 
consistently to avoid the problem (there is the analogy with 
orthogonal basis in geometry If you understand what I mean). Now we 
use the [AccessCheckForFile] just in [nio] package. We can extend the 
implementation any time we need it.
Okay, I can live with this but would be nice to get it to AccessCheck 
at most point.

I did.
checkFileAccess ignores the exception from AccessCheck whereas I 
should it should be translated to an IOException.
That is by design. Any problem with the [checkFileAccess] need to be 
converted to the [false] return value. At the end point - in the
[WindowsFileSystemProvider.checkAccess] function - the [false] return 
value would be converted to the [AccessDeniedException] exception - 
that is desired code flow.
My point is that AccessCheck can fail for other reasons too and it 
would be good to get these reason into the exception so that it is not 
lost. It might have to AccessDeniedException if there aren't specific 
errors documented but at least the reason will be in the exception 
message to help someone figure it the issue. So I think it would be 
better to translate the exception rather than returning a boolean.

Done.


Otherwise I think this is good. You don't have a test case but I 
can't think how this could be tested anyway as we already have tests 
for checkAccess and isWritable.
I have the test. It is attached to the bug as Netbeans project, but 
it need manual security setup in security tab of the [demofile.txt] 
file (as shown in attached screenshot). By changing the Write check 
box on the [demofile.txt] file security dialog, test result have 
varying accordingly.

Seems the web bug-db interface is not synchronized yet.

Thanks, I guessed that an automated test would not be possible.
That is possible, but includes pre-requirements for installed MS tools. 
That is not a good idea.


New version of the fix was prepared.
Bug description:
http://bugs.sun.com/view_bug.do?bug_id=7190897
https://jbs.oracle.com/bugs/browse/JDK-7190897
The suggested fix:
http://cr.openjdk.java.net/~uta/openjdk-webrevs/JDK-7190897/webrev.01/

Regards,
-uta

Re: Request for review: 7190897 (fs) Files.isWritable method returns false when the path is writable (win). - ver. 1

2013-03-01 Thread Alan Bateman 

On 01/03/2013 11:45, Alexey Utkin wrote:


That is possible, but includes pre-requirements for installed MS 
tools. That is not a good idea.

I agree.



New version of the fix was prepared.
Bug description:
http://bugs.sun.com/view_bug.do?bug_id=7190897
https://jbs.oracle.com/bugs/browse/JDK-7190897
The suggested fix:
http://cr.openjdk.java.net/~uta/openjdk-webrevs/JDK-7190897/webrev.01/

This looks much better, thank you.

A minor point is that in checkAccessMask then you could call 
OpenThreadToken before the try/finally. That way you wouldn't need to 
check hToken. Otherwise I think this is good to go and it's nice to 
finally fix this issue.


-Alan