[
https://issues.apache.org/jira/browse/HADOOP-11321?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14244957#comment-14244957
]
Colin Patrick McCabe edited comment on HADOOP-11321 at 12/12/14 11:14 PM:
--------------------------------------------------------------------------
Great. Let me take a look.
{code}
- if (permission != null) {
+ // A local file system implementation may choose to create the file and set
+ // permissions immediately in a single syscall. If so, then skip setting
+ // permissions here.
+ if (permission != null && !(fs instanceof RawLocalFileSystem &&
+ Path.WINDOWS && NativeIO.isAvailable())) {
{code}
Do we need this part? Earlier in this function, we invoke the
{{FileSystem#create}} method that takes an {{FsPermission}}, right? Then the
FileSystem handles it atomically (hopefully) or non-atomically (ew). Surely
we're not going to try to work around {{FileSystem}} instances that don't honor
the permission passed to {{create}}? (And surely we don't have any of those...
do we?)
{code}
/**
* Create a directory with permissions set to the specified mode. By
setting
* permissions at creation time, we avoid issues related to the user lacking
* WRITE_DAC rights on subsequent chmod calls. One example where this can
* occur is writing to an SMB share where the user does not have Full
Control
* rights, and therefore WRITE_DAC is denied. This method mimics the
* contract of {@link java.io.File#mkdir()}. All exceptions are caught and
* reported by returning {@code false} to the caller.
*
* @param path directory to create
* @param mode permissions of new directory
* @return boolean true if directory creation succeeded
*/
public static boolean createDirectoryWithMode(File path, int mode) {
{code}
I think we should just pass the exception through here. We don't want to keep
people guessing if there is an I/O error. Just like the new JDK7 File
functions throw an IOException on error, we should throw an IOE if the IO can't
be done. Then the caller can catch it and ignore it if appropriate. I realize
we need the (bad) "return false" semantics in FileSystem, but by putting this
here, we encourage people to use it for other stuff.
{code}
#ifdef UNIX
THROW(env, "java/io/IOException",
"The function Windows.createDirectoryWithMode0() is not supported on Unix");
#endif
#ifdef WINDOWS
...
{code}
Would it make more sense to have an {{#else}} after the {{#ifdef WINDOWS}}
clause? [edit: and change the message to say "is not supported on this
platform" rather than "not supported on Unix"]
in {{libwinutils.c}}:
{code}
*ppSD = pSD;
done:
if (dwRtnCode != ERROR_SUCCESS) {
LocalFree(pSD);
}
{code}
I guess this is a nitpick, but perhaps we should only set {{ppSD}} when the
return code is success? Of course it won't matter for callers that check the
return code, as they should.
was (Author: cmccabe):
Great. Let me take a look.
{code}
- if (permission != null) {
+ // A local file system implementation may choose to create the file and set
+ // permissions immediately in a single syscall. If so, then skip setting
+ // permissions here.
+ if (permission != null && !(fs instanceof RawLocalFileSystem &&
+ Path.WINDOWS && NativeIO.isAvailable())) {
{code}
Do we need this part? Earlier in this function, we invoke the
{{FileSystem#create}} method that takes an {{FsPermission}}, right? Then the
FileSystem handles it atomically (hopefully) or non-atomically (ew). Surely
we're not going to try to work around {{FileSystem}} instances that don't honor
the permission passed to {{create}}? (And surely we don't have any of those...
do we?)
{code}
/**
* Create a directory with permissions set to the specified mode. By
setting
* permissions at creation time, we avoid issues related to the user lacking
* WRITE_DAC rights on subsequent chmod calls. One example where this can
* occur is writing to an SMB share where the user does not have Full
Control
* rights, and therefore WRITE_DAC is denied. This method mimics the
* contract of {@link java.io.File#mkdir()}. All exceptions are caught and
* reported by returning {@code false} to the caller.
*
* @param path directory to create
* @param mode permissions of new directory
* @return boolean true if directory creation succeeded
*/
public static boolean createDirectoryWithMode(File path, int mode) {
{code}
I think we should just pass the exception through here. We don't want to keep
people guessing if there is an I/O error. Just like the new JDK7 File
functions throw an IOException on error, we should throw an IOE if the IO can't
be done. Then the caller can catch it and ignore it if appropriate. I realize
we need the (bad) "return false" semantics in FileSystem, but by putting this
here, we encourage people to use it for other stuff.
{code}
#ifdef UNIX
THROW(env, "java/io/IOException",
"The function Windows.createDirectoryWithMode0() is not supported on Unix");
#endif
#ifdef WINDOWS
...
{code}
Would it make more sense to have an {{#else}} after the {{#ifdef WINDOWS}}
clause?
in {{libwinutils.c}}:
{code}
*ppSD = pSD;
done:
if (dwRtnCode != ERROR_SUCCESS) {
LocalFree(pSD);
}
{code}
I guess this is a nitpick, but perhaps we should only set {{ppSD}} when the
return code is success? Of course it won't matter for callers that check the
return code, as they should.
> copyToLocal cannot save a file to an SMB share unless the user has Full
> Control permissions.
> --------------------------------------------------------------------------------------------
>
> Key: HADOOP-11321
> URL: https://issues.apache.org/jira/browse/HADOOP-11321
> Project: Hadoop Common
> Issue Type: Bug
> Components: fs
> Affects Versions: 2.6.0
> Reporter: Chris Nauroth
> Assignee: Chris Nauroth
> Attachments: HADOOP-11321.003.patch, HADOOP-11321.004.patch,
> HADOOP-11321.005.patch, HADOOP-11321.1.patch, HADOOP-11321.2.patch,
> winutils.tmp.patch
>
>
> In Hadoop 2, it is impossible to use {{copyToLocal}} to copy a file from HDFS
> to a destination on an SMB share. This is because in Hadoop 2, the
> {{copyToLocal}} maps to 2 underlying {{RawLocalFileSystem}} operations:
> {{create}} and {{setPermission}}. On an SMB share, the user may be
> authorized for the {{create}} but denied for the {{setPermission}}. Windows
> denies the {{WRITE_DAC}} right required by {{setPermission}} unless the user
> has Full Control permissions. Granting Full Control isn't feasible for most
> deployments, because it's insecure. This is a regression from Hadoop 1,
> where {{copyToLocal}} only did a {{create}} and didn't do a separate
> {{setPermission}}.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
