[jira] [Work logged] (HIVE-26887) Make sure dirPath has the correct permissions
[
https://issues.apache.org/jira/browse/HIVE-26887?focusedWorklogId=857269=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-857269
]
ASF GitHub Bot logged work on HIVE-26887:
-
Author: ASF GitHub Bot
Created on: 17/Apr/23 00:19
Start Date: 17/Apr/23 00:19
Worklog Time Spent: 10m
Work Description: github-actions[bot] closed pull request #3894:
HIVE-26887 Make sure dirPath has the correct permissions
URL: https://github.com/apache/hive/pull/3894
Issue Time Tracking
---
Worklog Id: (was: 857269)
Time Spent: 2h 20m (was: 2h 10m)
> Make sure dirPath has the correct permissions
> -
>
> Key: HIVE-26887
> URL: https://issues.apache.org/jira/browse/HIVE-26887
> Project: Hive
> Issue Type: Improvement
>Reporter: Zhang Dongsheng
>Priority: Major
> Labels: backward-incompatible, pull-request-available
> Time Spent: 2h 20m
> Remaining Estimate: 0h
>
> In the QueryResultsCache function of class QueryResultsCache, there is the
> following code segment
> {code:java}
> private QueryResultsCache(HiveConf configuration) throws IOException {
> ..
> FileSystem fs = cacheDirPath.getFileSystem(conf);
> FsPermission fsPermission = new FsPermission("700");
> fs.mkdirs(cacheDirPath, fsPermission);
> ..
> }
> {code}
> It can be seen that the function will use the mkdirs to create cacheDirPath,
> and the parameters passed in include the path variable cacheDirPath and a
> permission 700. But we haven't confirmed whether the permission is correctly
> assigned to the file.
> The above question is raised because there are two mkdir functions of hadoop,
> {code:java}
> mkdirs(Path f, FsPermission permission)
> {code}
> and
> {code:java}
> mkdirs(FileSystem fs, Path dir, FsPermission permission)
> {code}
> and the first one is used here. The permissions of this function will be
> affected by the underlying umask. Although 700 here will hardly be affected
> by umask, but I think from a rigorous point of view, we should have one more
> permission check and permission grant here.
> And I find same issue in other three methods here.
> In class Context
> {code:java}
> private Path getScratchDir(String scheme, String authority,
> boolean mkdir, String scratchDir) {
> ..
> FileSystem fs = dirPath.getFileSystem(conf);
> dirPath = new Path(fs.makeQualified(dirPath).toString());
> FsPermission fsPermission = new FsPermission(scratchDirPermission);
> if (!fs.mkdirs(dirPath, fsPermission)) {
> throw new RuntimeException("Cannot make directory: "
> + dirPath.toString());
> ..
> }
> {code}
> In class SessionState
> {code:java}
> static void createPath(HiveConf conf, Path path, String permission, boolean
> isLocal,
> boolean isCleanUp) throws IOException {
> FsPermission fsPermission = new FsPermission(permission);
> FileSystem fs;
> ..
> if (!fs.mkdirs(path, fsPermission)) {
> throw new IOException("Failed to create directory " + path + " on fs "
> + fs.getUri());
> }
> ..
> }
> {code}
> and in class TezSessionState
> {code:java}
> private Path createTezDir(String sessionId, String suffix) throws IOException
> {
> ..
> Path tezDir = new Path(hdfsScratchDir, TEZ_DIR);
> FileSystem fs = tezDir.getFileSystem(conf);
> FsPermission fsPermission = new FsPermission(HiveConf.getVar(conf,
> HiveConf.ConfVars.SCRATCHDIRPERMISSION));
> fs.mkdirs(tezDir, fsPermission);
> ..
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Work logged] (HIVE-26887) Make sure dirPath has the correct permissions
[
https://issues.apache.org/jira/browse/HIVE-26887?focusedWorklogId=855635=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-855635
]
ASF GitHub Bot logged work on HIVE-26887:
-
Author: ASF GitHub Bot
Created on: 09/Apr/23 00:21
Start Date: 09/Apr/23 00:21
Worklog Time Spent: 10m
Work Description: github-actions[bot] commented on PR #3894:
URL: https://github.com/apache/hive/pull/3894#issuecomment-1501004985
This pull request has been automatically marked as stale because it has not
had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the d...@hive.apache.org list if the patch is in
need of reviews.
Issue Time Tracking
---
Worklog Id: (was: 855635)
Time Spent: 2h 10m (was: 2h)
> Make sure dirPath has the correct permissions
> -
>
> Key: HIVE-26887
> URL: https://issues.apache.org/jira/browse/HIVE-26887
> Project: Hive
> Issue Type: Improvement
>Reporter: Zhang Dongsheng
>Priority: Major
> Labels: backward-incompatible, pull-request-available
> Time Spent: 2h 10m
> Remaining Estimate: 0h
>
> In the QueryResultsCache function of class QueryResultsCache, there is the
> following code segment
> {code:java}
> private QueryResultsCache(HiveConf configuration) throws IOException {
> ..
> FileSystem fs = cacheDirPath.getFileSystem(conf);
> FsPermission fsPermission = new FsPermission("700");
> fs.mkdirs(cacheDirPath, fsPermission);
> ..
> }
> {code}
> It can be seen that the function will use the mkdirs to create cacheDirPath,
> and the parameters passed in include the path variable cacheDirPath and a
> permission 700. But we haven't confirmed whether the permission is correctly
> assigned to the file.
> The above question is raised because there are two mkdir functions of hadoop,
> {code:java}
> mkdirs(Path f, FsPermission permission)
> {code}
> and
> {code:java}
> mkdirs(FileSystem fs, Path dir, FsPermission permission)
> {code}
> and the first one is used here. The permissions of this function will be
> affected by the underlying umask. Although 700 here will hardly be affected
> by umask, but I think from a rigorous point of view, we should have one more
> permission check and permission grant here.
> And I find same issue in other three methods here.
> In class Context
> {code:java}
> private Path getScratchDir(String scheme, String authority,
> boolean mkdir, String scratchDir) {
> ..
> FileSystem fs = dirPath.getFileSystem(conf);
> dirPath = new Path(fs.makeQualified(dirPath).toString());
> FsPermission fsPermission = new FsPermission(scratchDirPermission);
> if (!fs.mkdirs(dirPath, fsPermission)) {
> throw new RuntimeException("Cannot make directory: "
> + dirPath.toString());
> ..
> }
> {code}
> In class SessionState
> {code:java}
> static void createPath(HiveConf conf, Path path, String permission, boolean
> isLocal,
> boolean isCleanUp) throws IOException {
> FsPermission fsPermission = new FsPermission(permission);
> FileSystem fs;
> ..
> if (!fs.mkdirs(path, fsPermission)) {
> throw new IOException("Failed to create directory " + path + " on fs "
> + fs.getUri());
> }
> ..
> }
> {code}
> and in class TezSessionState
> {code:java}
> private Path createTezDir(String sessionId, String suffix) throws IOException
> {
> ..
> Path tezDir = new Path(hdfsScratchDir, TEZ_DIR);
> FileSystem fs = tezDir.getFileSystem(conf);
> FsPermission fsPermission = new FsPermission(HiveConf.getVar(conf,
> HiveConf.ConfVars.SCRATCHDIRPERMISSION));
> fs.mkdirs(tezDir, fsPermission);
> ..
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Work logged] (HIVE-26887) Make sure dirPath has the correct permissions
[
https://issues.apache.org/jira/browse/HIVE-26887?focusedWorklogId=844096=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-844096
]
ASF GitHub Bot logged work on HIVE-26887:
-
Author: ASF GitHub Bot
Created on: 07/Feb/23 11:44
Start Date: 07/Feb/23 11:44
Worklog Time Spent: 10m
Work Description: zabetak commented on PR #3894:
URL: https://github.com/apache/hive/pull/3894#issuecomment-1420640941
@skysiders Apologies for not following up on this but I am still pretty
busy with various things.
> Regarding the second point "programming pattern" you mentioned, in fact,
it is also possible to use Hadoop's underlying FileSystem.create(fs, path,
perm) here.
If this pattern is preferred then why not adopting it here? I saw your
comments above about a potential perf benefit. If that is true then it would be
better to propose this perf improvement in the "Hadoop HDFS" project. It would
be better to have this `if` block (or whatever) in one place rather in many
different places (5 times X 5 projects).
Other than that I don't really like the fact that `FileSystem.create` and
`fs.create` have different behavior; this will always be a problem for
developers when trying to pick which API to use. Again this is not something to
handle here but maybe discuss with the HDFS folks.
Issue Time Tracking
---
Worklog Id: (was: 844096)
Time Spent: 2h (was: 1h 50m)
> Make sure dirPath has the correct permissions
> -
>
> Key: HIVE-26887
> URL: https://issues.apache.org/jira/browse/HIVE-26887
> Project: Hive
> Issue Type: Improvement
>Reporter: Zhang Dongsheng
>Priority: Major
> Labels: backward-incompatible, pull-request-available
> Time Spent: 2h
> Remaining Estimate: 0h
>
> In the QueryResultsCache function of class QueryResultsCache, there is the
> following code segment
> {code:java}
> private QueryResultsCache(HiveConf configuration) throws IOException {
> ..
> FileSystem fs = cacheDirPath.getFileSystem(conf);
> FsPermission fsPermission = new FsPermission("700");
> fs.mkdirs(cacheDirPath, fsPermission);
> ..
> }
> {code}
> It can be seen that the function will use the mkdirs to create cacheDirPath,
> and the parameters passed in include the path variable cacheDirPath and a
> permission 700. But we haven't confirmed whether the permission is correctly
> assigned to the file.
> The above question is raised because there are two mkdir functions of hadoop,
> {code:java}
> mkdirs(Path f, FsPermission permission)
> {code}
> and
> {code:java}
> mkdirs(FileSystem fs, Path dir, FsPermission permission)
> {code}
> and the first one is used here. The permissions of this function will be
> affected by the underlying umask. Although 700 here will hardly be affected
> by umask, but I think from a rigorous point of view, we should have one more
> permission check and permission grant here.
> And I find same issue in other three methods here.
> In class Context
> {code:java}
> private Path getScratchDir(String scheme, String authority,
> boolean mkdir, String scratchDir) {
> ..
> FileSystem fs = dirPath.getFileSystem(conf);
> dirPath = new Path(fs.makeQualified(dirPath).toString());
> FsPermission fsPermission = new FsPermission(scratchDirPermission);
> if (!fs.mkdirs(dirPath, fsPermission)) {
> throw new RuntimeException("Cannot make directory: "
> + dirPath.toString());
> ..
> }
> {code}
> In class SessionState
> {code:java}
> static void createPath(HiveConf conf, Path path, String permission, boolean
> isLocal,
> boolean isCleanUp) throws IOException {
> FsPermission fsPermission = new FsPermission(permission);
> FileSystem fs;
> ..
> if (!fs.mkdirs(path, fsPermission)) {
> throw new IOException("Failed to create directory " + path + " on fs "
> + fs.getUri());
> }
> ..
> }
> {code}
> and in class TezSessionState
> {code:java}
> private Path createTezDir(String sessionId, String suffix) throws IOException
> {
> ..
> Path tezDir = new Path(hdfsScratchDir, TEZ_DIR);
> FileSystem fs = tezDir.getFileSystem(conf);
> FsPermission fsPermission = new FsPermission(HiveConf.getVar(conf,
> HiveConf.ConfVars.SCRATCHDIRPERMISSION));
> fs.mkdirs(tezDir, fsPermission);
> ..
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Work logged] (HIVE-26887) Make sure dirPath has the correct permissions
[
https://issues.apache.org/jira/browse/HIVE-26887?focusedWorklogId=839996=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-839996
]
ASF GitHub Bot logged work on HIVE-26887:
-
Author: ASF GitHub Bot
Created on: 18/Jan/23 14:54
Start Date: 18/Jan/23 14:54
Worklog Time Spent: 10m
Work Description: skysiders commented on PR #3894:
URL: https://github.com/apache/hive/pull/3894#issuecomment-1387202288
Hi @zabetak , thanks for your review.
You mentioned that this is "kind of a breaking change", but I don't agree
with this. In setting file permissions, most of the time we can rely on the
umask of the underlying file system, such as the most commonly used fs.create
function, but in In my fix, all file permissions are explicitly set. I think
this explicit permission setting is due to the developer thinking that the file
should be set to this explicit permission. If the development of this pair of
files does not require explicit permissions, the underlying umask can indeed be
used to constrain it, but once the permissions are clear, the underlying umask
may cause the file permissions to be too strict and make the files unusable. I
would like to give an inappropriate example here. For example, the umask of the
underlying file system is 777, and the file permissions are 000, so the
upper-level files will not have any permissions. Therefore, for such files with
clearly set permissions, I think it should be Make sure they are properly
assigned permissions.
Regarding the second point "programming pattern" you mentioned, in fact, it
is also possible to use Hadoop's underlying FileSystem.create(fs, path, perm)
here. In fact, I now think that such a "programming pattern" should be adopted,
because this It is safe and more reliable than fs.create(path,perm). This kind
of repair is mainly aimed at API misuse. I have mentioned this problem in[
HBASE-26994](https://github.com/apache/hbase/pull/4391), which means that the
developer originally intended to Set special permissions here, but mistakenly
think that fs.create(path, perm) can set special permissions perm for the path.
In fact, this is wrong. In the chat with the hbase developer, I pointed out his
mistake and Got his approval.
Finally, I want to say that this fix is necessary in my opinion. I have
searched in hive and found these four API misuse problems, so I point out this
problem here.
Issue Time Tracking
---
Worklog Id: (was: 839996)
Time Spent: 1h 50m (was: 1h 40m)
> Make sure dirPath has the correct permissions
> -
>
> Key: HIVE-26887
> URL: https://issues.apache.org/jira/browse/HIVE-26887
> Project: Hive
> Issue Type: Improvement
>Reporter: Zhang Dongsheng
>Priority: Major
> Labels: backward-incompatible, pull-request-available
> Time Spent: 1h 50m
> Remaining Estimate: 0h
>
> In the QueryResultsCache function of class QueryResultsCache, there is the
> following code segment
> {code:java}
> private QueryResultsCache(HiveConf configuration) throws IOException {
> ..
> FileSystem fs = cacheDirPath.getFileSystem(conf);
> FsPermission fsPermission = new FsPermission("700");
> fs.mkdirs(cacheDirPath, fsPermission);
> ..
> }
> {code}
> It can be seen that the function will use the mkdirs to create cacheDirPath,
> and the parameters passed in include the path variable cacheDirPath and a
> permission 700. But we haven't confirmed whether the permission is correctly
> assigned to the file.
> The above question is raised because there are two mkdir functions of hadoop,
> {code:java}
> mkdirs(Path f, FsPermission permission)
> {code}
> and
> {code:java}
> mkdirs(FileSystem fs, Path dir, FsPermission permission)
> {code}
> and the first one is used here. The permissions of this function will be
> affected by the underlying umask. Although 700 here will hardly be affected
> by umask, but I think from a rigorous point of view, we should have one more
> permission check and permission grant here.
> And I find same issue in other three methods here.
> In class Context
> {code:java}
> private Path getScratchDir(String scheme, String authority,
> boolean mkdir, String scratchDir) {
> ..
> FileSystem fs = dirPath.getFileSystem(conf);
> dirPath = new Path(fs.makeQualified(dirPath).toString());
> FsPermission fsPermission = new FsPermission(scratchDirPermission);
> if (!fs.mkdirs(dirPath, fsPermission)) {
> throw new RuntimeException("Cannot make directory: "
> + dirPath.toString());
> ..
> }
> {code}
> In class SessionState
> {code:java}
> static void createPath(HiveConf conf, Path path, String
[jira] [Work logged] (HIVE-26887) Make sure dirPath has the correct permissions
[
https://issues.apache.org/jira/browse/HIVE-26887?focusedWorklogId=839917=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-839917
]
ASF GitHub Bot logged work on HIVE-26887:
-
Author: ASF GitHub Bot
Created on: 18/Jan/23 11:17
Start Date: 18/Jan/23 11:17
Worklog Time Spent: 10m
Work Description: zabetak commented on PR #3894:
URL: https://github.com/apache/hive/pull/3894#issuecomment-1386890501
Thanks for the elaborate analysis and discussion @skysiders @cnauroth !
Looking into the changes it seems that this is kind of a breaking change
since depending on the configuration permissions will be set differently.
Moreover the proposed changes make the code more verbose and less
straightforward.
Furthermore, I am not sure we want to enforce a programming pattern where we
do `fs.mkdirs` and then `fs.setPermission` since like that we essentially
by-pass the umask that is the expected way of creating directories with the
appropriate permissions
(https://issues.apache.org/jira/browse/HDFS-1322?focusedCommentId=13072984=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-13072984).
For the reasons above, I would prefer if we didn't merge these changes.
Issue Time Tracking
---
Worklog Id: (was: 839917)
Time Spent: 1h 40m (was: 1.5h)
> Make sure dirPath has the correct permissions
> -
>
> Key: HIVE-26887
> URL: https://issues.apache.org/jira/browse/HIVE-26887
> Project: Hive
> Issue Type: Improvement
>Reporter: Zhang Dongsheng
>Priority: Major
> Labels: pull-request-available
> Time Spent: 1h 40m
> Remaining Estimate: 0h
>
> In the QueryResultsCache function of class QueryResultsCache, there is the
> following code segment
> {code:java}
> private QueryResultsCache(HiveConf configuration) throws IOException {
> ..
> FileSystem fs = cacheDirPath.getFileSystem(conf);
> FsPermission fsPermission = new FsPermission("700");
> fs.mkdirs(cacheDirPath, fsPermission);
> ..
> }
> {code}
> It can be seen that the function will use the mkdirs to create cacheDirPath,
> and the parameters passed in include the path variable cacheDirPath and a
> permission 700. But we haven't confirmed whether the permission is correctly
> assigned to the file.
> The above question is raised because there are two mkdir functions of hadoop,
> {code:java}
> mkdirs(Path f, FsPermission permission)
> {code}
> and
> {code:java}
> mkdirs(FileSystem fs, Path dir, FsPermission permission)
> {code}
> and the first one is used here. The permissions of this function will be
> affected by the underlying umask. Although 700 here will hardly be affected
> by umask, but I think from a rigorous point of view, we should have one more
> permission check and permission grant here.
> And I find same issue in other three methods here.
> In class Context
> {code:java}
> private Path getScratchDir(String scheme, String authority,
> boolean mkdir, String scratchDir) {
> ..
> FileSystem fs = dirPath.getFileSystem(conf);
> dirPath = new Path(fs.makeQualified(dirPath).toString());
> FsPermission fsPermission = new FsPermission(scratchDirPermission);
> if (!fs.mkdirs(dirPath, fsPermission)) {
> throw new RuntimeException("Cannot make directory: "
> + dirPath.toString());
> ..
> }
> {code}
> In class SessionState
> {code:java}
> static void createPath(HiveConf conf, Path path, String permission, boolean
> isLocal,
> boolean isCleanUp) throws IOException {
> FsPermission fsPermission = new FsPermission(permission);
> FileSystem fs;
> ..
> if (!fs.mkdirs(path, fsPermission)) {
> throw new IOException("Failed to create directory " + path + " on fs "
> + fs.getUri());
> }
> ..
> }
> {code}
> and in class TezSessionState
> {code:java}
> private Path createTezDir(String sessionId, String suffix) throws IOException
> {
> ..
> Path tezDir = new Path(hdfsScratchDir, TEZ_DIR);
> FileSystem fs = tezDir.getFileSystem(conf);
> FsPermission fsPermission = new FsPermission(HiveConf.getVar(conf,
> HiveConf.ConfVars.SCRATCHDIRPERMISSION));
> fs.mkdirs(tezDir, fsPermission);
> ..
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Work logged] (HIVE-26887) Make sure dirPath has the correct permissions
[
https://issues.apache.org/jira/browse/HIVE-26887?focusedWorklogId=838507=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-838507
]
ASF GitHub Bot logged work on HIVE-26887:
-
Author: ASF GitHub Bot
Created on: 11/Jan/23 03:01
Start Date: 11/Jan/23 03:01
Worklog Time Spent: 10m
Work Description: skysiders commented on PR #3894:
URL: https://github.com/apache/hive/pull/3894#issuecomment-1378181317
Hi @zabetak , could you please have a look at this?
Issue Time Tracking
---
Worklog Id: (was: 838507)
Time Spent: 1.5h (was: 1h 20m)
> Make sure dirPath has the correct permissions
> -
>
> Key: HIVE-26887
> URL: https://issues.apache.org/jira/browse/HIVE-26887
> Project: Hive
> Issue Type: Improvement
>Reporter: Zhang Dongsheng
>Priority: Major
> Labels: pull-request-available
> Time Spent: 1.5h
> Remaining Estimate: 0h
>
> In the QueryResultsCache function of class QueryResultsCache, there is the
> following code segment
> {code:java}
> private QueryResultsCache(HiveConf configuration) throws IOException {
> ..
> FileSystem fs = cacheDirPath.getFileSystem(conf);
> FsPermission fsPermission = new FsPermission("700");
> fs.mkdirs(cacheDirPath, fsPermission);
> ..
> }
> {code}
> It can be seen that the function will use the mkdirs to create cacheDirPath,
> and the parameters passed in include the path variable cacheDirPath and a
> permission 700. But we haven't confirmed whether the permission is correctly
> assigned to the file.
> The above question is raised because there are two mkdir functions of hadoop,
> {code:java}
> mkdirs(Path f, FsPermission permission)
> {code}
> and
> {code:java}
> mkdirs(FileSystem fs, Path dir, FsPermission permission)
> {code}
> and the first one is used here. The permissions of this function will be
> affected by the underlying umask. Although 700 here will hardly be affected
> by umask, but I think from a rigorous point of view, we should have one more
> permission check and permission grant here.
> And I find same issue in other three methods here.
> In class Context
> {code:java}
> private Path getScratchDir(String scheme, String authority,
> boolean mkdir, String scratchDir) {
> ..
> FileSystem fs = dirPath.getFileSystem(conf);
> dirPath = new Path(fs.makeQualified(dirPath).toString());
> FsPermission fsPermission = new FsPermission(scratchDirPermission);
> if (!fs.mkdirs(dirPath, fsPermission)) {
> throw new RuntimeException("Cannot make directory: "
> + dirPath.toString());
> ..
> }
> {code}
> In class SessionState
> {code:java}
> static void createPath(HiveConf conf, Path path, String permission, boolean
> isLocal,
> boolean isCleanUp) throws IOException {
> FsPermission fsPermission = new FsPermission(permission);
> FileSystem fs;
> ..
> if (!fs.mkdirs(path, fsPermission)) {
> throw new IOException("Failed to create directory " + path + " on fs "
> + fs.getUri());
> }
> ..
> }
> {code}
> and in class TezSessionState
> {code:java}
> private Path createTezDir(String sessionId, String suffix) throws IOException
> {
> ..
> Path tezDir = new Path(hdfsScratchDir, TEZ_DIR);
> FileSystem fs = tezDir.getFileSystem(conf);
> FsPermission fsPermission = new FsPermission(HiveConf.getVar(conf,
> HiveConf.ConfVars.SCRATCHDIRPERMISSION));
> fs.mkdirs(tezDir, fsPermission);
> ..
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Work logged] (HIVE-26887) Make sure dirPath has the correct permissions
[
https://issues.apache.org/jira/browse/HIVE-26887?focusedWorklogId=837727=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-837727
]
ASF GitHub Bot logged work on HIVE-26887:
-
Author: ASF GitHub Bot
Created on: 08/Jan/23 08:58
Start Date: 08/Jan/23 08:58
Worklog Time Spent: 10m
Work Description: skysiders commented on PR #3894:
URL: https://github.com/apache/hive/pull/3894#issuecomment-1374765844
Hi @cnauroth @abstractdog , I reviewed this part of the code again today,
and I found that a previous patch here is
[HIVE-4487](https://github.com/brockn/hive-parquet/commit/87bce7e0a6813996ab671a7f8371aee8f4101e1),
we can clearly see that the developer deliberately set permissions here, and
then this code was modified again in
[HIVE-8015](https://github.com/apache/hive/commit/8a2954d417105aa8c994f0766ee36b09b0c4e1fc)
I used to be in
[HBASE-26994](https://github.com/apache/hbase/pull/4391#discussion_r866360992)
mentioned the concept of API misuse, I think the second patch here should be
the misuse of API.
Issue Time Tracking
---
Worklog Id: (was: 837727)
Time Spent: 1h 20m (was: 1h 10m)
> Make sure dirPath has the correct permissions
> -
>
> Key: HIVE-26887
> URL: https://issues.apache.org/jira/browse/HIVE-26887
> Project: Hive
> Issue Type: Improvement
>Reporter: Zhang Dongsheng
>Priority: Major
> Labels: pull-request-available
> Time Spent: 1h 20m
> Remaining Estimate: 0h
>
> In the QueryResultsCache function of class QueryResultsCache, there is the
> following code segment
> {code:java}
> private QueryResultsCache(HiveConf configuration) throws IOException {
> ..
> FileSystem fs = cacheDirPath.getFileSystem(conf);
> FsPermission fsPermission = new FsPermission("700");
> fs.mkdirs(cacheDirPath, fsPermission);
> ..
> }
> {code}
> It can be seen that the function will use the mkdirs to create cacheDirPath,
> and the parameters passed in include the path variable cacheDirPath and a
> permission 700. But we haven't confirmed whether the permission is correctly
> assigned to the file.
> The above question is raised because there are two mkdir functions of hadoop,
> {code:java}
> mkdirs(Path f, FsPermission permission)
> {code}
> and
> {code:java}
> mkdirs(FileSystem fs, Path dir, FsPermission permission)
> {code}
> and the first one is used here. The permissions of this function will be
> affected by the underlying umask. Although 700 here will hardly be affected
> by umask, but I think from a rigorous point of view, we should have one more
> permission check and permission grant here.
> And I find same issue in other three methods here.
> In class Context
> {code:java}
> private Path getScratchDir(String scheme, String authority,
> boolean mkdir, String scratchDir) {
> ..
> FileSystem fs = dirPath.getFileSystem(conf);
> dirPath = new Path(fs.makeQualified(dirPath).toString());
> FsPermission fsPermission = new FsPermission(scratchDirPermission);
> if (!fs.mkdirs(dirPath, fsPermission)) {
> throw new RuntimeException("Cannot make directory: "
> + dirPath.toString());
> ..
> }
> {code}
> In class SessionState
> {code:java}
> static void createPath(HiveConf conf, Path path, String permission, boolean
> isLocal,
> boolean isCleanUp) throws IOException {
> FsPermission fsPermission = new FsPermission(permission);
> FileSystem fs;
> ..
> if (!fs.mkdirs(path, fsPermission)) {
> throw new IOException("Failed to create directory " + path + " on fs "
> + fs.getUri());
> }
> ..
> }
> {code}
> and in class TezSessionState
> {code:java}
> private Path createTezDir(String sessionId, String suffix) throws IOException
> {
> ..
> Path tezDir = new Path(hdfsScratchDir, TEZ_DIR);
> FileSystem fs = tezDir.getFileSystem(conf);
> FsPermission fsPermission = new FsPermission(HiveConf.getVar(conf,
> HiveConf.ConfVars.SCRATCHDIRPERMISSION));
> fs.mkdirs(tezDir, fsPermission);
> ..
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Work logged] (HIVE-26887) Make sure dirPath has the correct permissions
[
https://issues.apache.org/jira/browse/HIVE-26887?focusedWorklogId=837715=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-837715
]
ASF GitHub Bot logged work on HIVE-26887:
-
Author: ASF GitHub Bot
Created on: 08/Jan/23 02:20
Start Date: 08/Jan/23 02:20
Worklog Time Spent: 10m
Work Description: sonarcloud[bot] commented on PR #3894:
URL: https://github.com/apache/hive/pull/3894#issuecomment-1374689288
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive=3894)
[](https://sonarcloud.io/project/issues?id=apache_hive=3894=false=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive=3894=false=BUG)
[0
Bugs](https://sonarcloud.io/project/issues?id=apache_hive=3894=false=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive=3894=false=VULNERABILITY)
[](https://sonarcloud.io/project/issues?id=apache_hive=3894=false=VULNERABILITY)
[0
Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive=3894=false=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive=3894=false=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive=3894=false=SECURITY_HOTSPOT)
[0 Security
Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive=3894=false=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive=3894=false=CODE_SMELL)
[](https://sonarcloud.io/project/issues?id=apache_hive=3894=false=CODE_SMELL)
[1 Code
Smell](https://sonarcloud.io/project/issues?id=apache_hive=3894=false=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive=3894=coverage=list)
No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive=3894=duplicated_lines_density=list)
No Duplication information
Issue Time Tracking
---
Worklog Id: (was: 837715)
Time Spent: 1h 10m (was: 1h)
> Make sure dirPath has the correct permissions
> -
>
> Key: HIVE-26887
> URL: https://issues.apache.org/jira/browse/HIVE-26887
> Project: Hive
> Issue Type: Improvement
>Reporter: Zhang Dongsheng
>Priority: Major
> Labels: pull-request-available
> Time Spent: 1h 10m
> Remaining Estimate: 0h
>
> In the QueryResultsCache function of class QueryResultsCache, there is the
> following code segment
> {code:java}
> private QueryResultsCache(HiveConf configuration) throws IOException {
> ..
> FileSystem fs = cacheDirPath.getFileSystem(conf);
> FsPermission fsPermission = new FsPermission("700");
> fs.mkdirs(cacheDirPath, fsPermission);
> ..
> }
> {code}
> It can be seen that the function will use the mkdirs to create cacheDirPath,
> and the parameters passed in include the path variable cacheDirPath and a
> permission 700. But we haven't confirmed whether the permission is correctly
> assigned to the file.
> The above question is raised because there are two mkdir functions of hadoop,
> {code:java}
> mkdirs(Path f, FsPermission permission)
> {code}
> and
> {code:java}
> mkdirs(FileSystem fs, Path dir, FsPermission permission)
> {code}
> and the first one is used here. The permissions of this function will be
> affected by the underlying umask. Although 700 here will hardly be affected
> by
[jira] [Work logged] (HIVE-26887) Make sure dirPath has the correct permissions
[
https://issues.apache.org/jira/browse/HIVE-26887?focusedWorklogId=837714=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-837714
]
ASF GitHub Bot logged work on HIVE-26887:
-
Author: ASF GitHub Bot
Created on: 08/Jan/23 01:34
Start Date: 08/Jan/23 01:34
Worklog Time Spent: 10m
Work Description: skysiders commented on PR #3894:
URL: https://github.com/apache/hive/pull/3894#issuecomment-1374681627
Hi @cnauroth , thanks for your review. I'm glad to see your reply. In the
MAPREDUCE-7375 you mentioned, I saw your approval of this fix, as well as the
extremely special situation that you mentioned earlier that the umask would not
be set to 777, and I very much agree with these. The repairs I submitted in
other projects are also based on the way of thinking I mentioned above, that
is, after we create a file, should we check and judge the permissions of this
file and grant permissions to it. I think this matter is necessary, because as
an upper-level application, it is difficult to determine the umask setting of
the underlying file system (for example, the umask under Linux is set by the
system administrator root, and the umask of the distributed filesystem is set
by the underlying hdfs The user decides when starting dfs, while hive or other
systems are run by program administrators, and these two users may not be the
same user) This is my most fundamental point of view, and it is also the reason
why I insist on this repair.
I would like to discuss this issue with you further. Do you think that this
repair method should be applied to those permissions of 770 instead of
permissions like 700 (because 770 may be defaulted by the file system umask=022
affect)?
Finally, I am very happy to have such an in-depth discussion with you on the
issue of file permissions. Thank you for your views on my point of view.
Hi @abstractdog @ayushtkn , could you please have a look at this?
Issue Time Tracking
---
Worklog Id: (was: 837714)
Time Spent: 1h (was: 50m)
> Make sure dirPath has the correct permissions
> -
>
> Key: HIVE-26887
> URL: https://issues.apache.org/jira/browse/HIVE-26887
> Project: Hive
> Issue Type: Improvement
>Reporter: Zhang Dongsheng
>Priority: Major
> Labels: pull-request-available
> Time Spent: 1h
> Remaining Estimate: 0h
>
> In the QueryResultsCache function of class QueryResultsCache, there is the
> following code segment
> {code:java}
> private QueryResultsCache(HiveConf configuration) throws IOException {
> ..
> FileSystem fs = cacheDirPath.getFileSystem(conf);
> FsPermission fsPermission = new FsPermission("700");
> fs.mkdirs(cacheDirPath, fsPermission);
> ..
> }
> {code}
> It can be seen that the function will use the mkdirs to create cacheDirPath,
> and the parameters passed in include the path variable cacheDirPath and a
> permission 700. But we haven't confirmed whether the permission is correctly
> assigned to the file.
> The above question is raised because there are two mkdir functions of hadoop,
> {code:java}
> mkdirs(Path f, FsPermission permission)
> {code}
> and
> {code:java}
> mkdirs(FileSystem fs, Path dir, FsPermission permission)
> {code}
> and the first one is used here. The permissions of this function will be
> affected by the underlying umask. Although 700 here will hardly be affected
> by umask, but I think from a rigorous point of view, we should have one more
> permission check and permission grant here.
> And I find same issue in other three methods here.
> In class Context
> {code:java}
> private Path getScratchDir(String scheme, String authority,
> boolean mkdir, String scratchDir) {
> ..
> FileSystem fs = dirPath.getFileSystem(conf);
> dirPath = new Path(fs.makeQualified(dirPath).toString());
> FsPermission fsPermission = new FsPermission(scratchDirPermission);
> if (!fs.mkdirs(dirPath, fsPermission)) {
> throw new RuntimeException("Cannot make directory: "
> + dirPath.toString());
> ..
> }
> {code}
> In class SessionState
> {code:java}
> static void createPath(HiveConf conf, Path path, String permission, boolean
> isLocal,
> boolean isCleanUp) throws IOException {
> FsPermission fsPermission = new FsPermission(permission);
> FileSystem fs;
> ..
> if (!fs.mkdirs(path, fsPermission)) {
> throw new IOException("Failed to create directory " + path + " on fs "
> + fs.getUri());
> }
> ..
> }
> {code}
> and in class TezSessionState
> {code:java}
> private Path createTezDir(String sessionId, String suffix) throws IOException
> {
> ..
> Path
[jira] [Work logged] (HIVE-26887) Make sure dirPath has the correct permissions
[
https://issues.apache.org/jira/browse/HIVE-26887?focusedWorklogId=837628=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-837628
]
ASF GitHub Bot logged work on HIVE-26887:
-
Author: ASF GitHub Bot
Created on: 06/Jan/23 22:04
Start Date: 06/Jan/23 22:04
Worklog Time Spent: 10m
Work Description: cnauroth commented on code in PR #3894:
URL: https://github.com/apache/hive/pull/3894#discussion_r1063820911
##
ql/src/java/org/apache/hadoop/hive/ql/cache/results/QueryResultsCache.java:
##
@@ -371,6 +371,9 @@ private QueryResultsCache(HiveConf configuration) throws
IOException {
FsPermission fsPermission = new FsPermission("700");
fs.mkdirs(cacheDirPath, fsPermission);
+if
(!fsPermission.equals(fsPermission.applyUMask(FsPermission.getUMask(conf {
+fs.setPermission(cacheDirPath, fsPermission);
Review Comment:
Indentation is off on this line.
Issue Time Tracking
---
Worklog Id: (was: 837628)
Time Spent: 50m (was: 40m)
> Make sure dirPath has the correct permissions
> -
>
> Key: HIVE-26887
> URL: https://issues.apache.org/jira/browse/HIVE-26887
> Project: Hive
> Issue Type: Improvement
>Reporter: Zhang Dongsheng
>Priority: Major
> Labels: pull-request-available
> Time Spent: 50m
> Remaining Estimate: 0h
>
> In the QueryResultsCache function of class QueryResultsCache, there is the
> following code segment
> {code:java}
> private QueryResultsCache(HiveConf configuration) throws IOException {
> ..
> FileSystem fs = cacheDirPath.getFileSystem(conf);
> FsPermission fsPermission = new FsPermission("700");
> fs.mkdirs(cacheDirPath, fsPermission);
> ..
> }
> {code}
> It can be seen that the function will use the mkdirs to create cacheDirPath,
> and the parameters passed in include the path variable cacheDirPath and a
> permission 700. But we haven't confirmed whether the permission is correctly
> assigned to the file.
> The above question is raised because there are two mkdir functions of hadoop,
> {code:java}
> mkdirs(Path f, FsPermission permission)
> {code}
> and
> {code:java}
> mkdirs(FileSystem fs, Path dir, FsPermission permission)
> {code}
> and the first one is used here. The permissions of this function will be
> affected by the underlying umask. Although 700 here will hardly be affected
> by umask, but I think from a rigorous point of view, we should have one more
> permission check and permission grant here.
> And I find same issue in other three methods here.
> In class Context
> {code:java}
> private Path getScratchDir(String scheme, String authority,
> boolean mkdir, String scratchDir) {
> ..
> FileSystem fs = dirPath.getFileSystem(conf);
> dirPath = new Path(fs.makeQualified(dirPath).toString());
> FsPermission fsPermission = new FsPermission(scratchDirPermission);
> if (!fs.mkdirs(dirPath, fsPermission)) {
> throw new RuntimeException("Cannot make directory: "
> + dirPath.toString());
> ..
> }
> {code}
> In class SessionState
> {code:java}
> static void createPath(HiveConf conf, Path path, String permission, boolean
> isLocal,
> boolean isCleanUp) throws IOException {
> FsPermission fsPermission = new FsPermission(permission);
> FileSystem fs;
> ..
> if (!fs.mkdirs(path, fsPermission)) {
> throw new IOException("Failed to create directory " + path + " on fs "
> + fs.getUri());
> }
> ..
> }
> {code}
> and in class TezSessionState
> {code:java}
> private Path createTezDir(String sessionId, String suffix) throws IOException
> {
> ..
> Path tezDir = new Path(hdfsScratchDir, TEZ_DIR);
> FileSystem fs = tezDir.getFileSystem(conf);
> FsPermission fsPermission = new FsPermission(HiveConf.getVar(conf,
> HiveConf.ConfVars.SCRATCHDIRPERMISSION));
> fs.mkdirs(tezDir, fsPermission);
> ..
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Work logged] (HIVE-26887) Make sure dirPath has the correct permissions
[
https://issues.apache.org/jira/browse/HIVE-26887?focusedWorklogId=837442=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-837442
]
ASF GitHub Bot logged work on HIVE-26887:
-
Author: ASF GitHub Bot
Created on: 06/Jan/23 09:31
Start Date: 06/Jan/23 09:31
Worklog Time Spent: 10m
Work Description: skysiders commented on PR #3894:
URL: https://github.com/apache/hive/pull/3894#issuecomment-1373394928
Hi @cnauroth ,I thought about this question again, and I think that compared
with FileSystem.mkdirs(fs,path,perms) mentioned by you, my patch may be a
little faster. The mkdirs(fs,path,perms) function does two things, first
creating a file with default permissions and then assigning permissions to that
file. What I do can be divided into three steps, first create the file, then
determine if the file permissions are correct, if not set permissions. I think
the reason for the speed here is that the permission judgment is faster than
setting the permission directly, only if the file itself has the same
permission and the permission to set, it can be about 4 times faster. According
to what you said, this patch should modify all mkdirs(path,perms) into
mkdirs(fs,path,perms), but as I just said, I think it will be a little slower
than my current patch, but this kind of file creation is also rigorous.I‘m
looking forward to your reply.
Issue Time Tracking
---
Worklog Id: (was: 837442)
Time Spent: 40m (was: 0.5h)
> Make sure dirPath has the correct permissions
> -
>
> Key: HIVE-26887
> URL: https://issues.apache.org/jira/browse/HIVE-26887
> Project: Hive
> Issue Type: Improvement
>Reporter: Zhang Dongsheng
>Priority: Major
> Labels: pull-request-available
> Time Spent: 40m
> Remaining Estimate: 0h
>
> In the QueryResultsCache function of class QueryResultsCache, there is the
> following code segment
> {code:java}
> private QueryResultsCache(HiveConf configuration) throws IOException {
> ..
> FileSystem fs = cacheDirPath.getFileSystem(conf);
> FsPermission fsPermission = new FsPermission("700");
> fs.mkdirs(cacheDirPath, fsPermission);
> ..
> }
> {code}
> It can be seen that the function will use the mkdirs to create cacheDirPath,
> and the parameters passed in include the path variable cacheDirPath and a
> permission 700. But we haven't confirmed whether the permission is correctly
> assigned to the file.
> The above question is raised because there are two mkdir functions of hadoop,
> {code:java}
> mkdirs(Path f, FsPermission permission)
> {code}
> and
> {code:java}
> mkdirs(FileSystem fs, Path dir, FsPermission permission)
> {code}
> and the first one is used here. The permissions of this function will be
> affected by the underlying umask. Although 700 here will hardly be affected
> by umask, but I think from a rigorous point of view, we should have one more
> permission check and permission grant here.
> And I find same issue in other three methods here.
> In class Context
> {code:java}
> private Path getScratchDir(String scheme, String authority,
> boolean mkdir, String scratchDir) {
> ..
> FileSystem fs = dirPath.getFileSystem(conf);
> dirPath = new Path(fs.makeQualified(dirPath).toString());
> FsPermission fsPermission = new FsPermission(scratchDirPermission);
> if (!fs.mkdirs(dirPath, fsPermission)) {
> throw new RuntimeException("Cannot make directory: "
> + dirPath.toString());
> ..
> }
> {code}
> In class SessionState
> {code:java}
> static void createPath(HiveConf conf, Path path, String permission, boolean
> isLocal,
> boolean isCleanUp) throws IOException {
> FsPermission fsPermission = new FsPermission(permission);
> FileSystem fs;
> ..
> if (!fs.mkdirs(path, fsPermission)) {
> throw new IOException("Failed to create directory " + path + " on fs "
> + fs.getUri());
> }
> ..
> }
> {code}
> and in class TezSessionState
> {code:java}
> private Path createTezDir(String sessionId, String suffix) throws IOException
> {
> ..
> Path tezDir = new Path(hdfsScratchDir, TEZ_DIR);
> FileSystem fs = tezDir.getFileSystem(conf);
> FsPermission fsPermission = new FsPermission(HiveConf.getVar(conf,
> HiveConf.ConfVars.SCRATCHDIRPERMISSION));
> fs.mkdirs(tezDir, fsPermission);
> ..
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
[jira] [Work logged] (HIVE-26887) Make sure dirPath has the correct permissions
[
https://issues.apache.org/jira/browse/HIVE-26887?focusedWorklogId=837251=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-837251
]
ASF GitHub Bot logged work on HIVE-26887:
-
Author: ASF GitHub Bot
Created on: 05/Jan/23 14:49
Start Date: 05/Jan/23 14:49
Worklog Time Spent: 10m
Work Description: skysiders commented on PR #3894:
URL: https://github.com/apache/hive/pull/3894#issuecomment-1372314783
Hi @cnauroth ,thank for your review!
What I want to talk about here is a programming mode, which is implemented
for special permissions. For example, we should check this permission after
setting the special permission. You are right, the 700 permissions here will
not cause defects with a high probability, but I think a good programming style
should be maintained to ensure the correct granting of file permissions, so
here I first judge the file permissions and check the file permissions Whether
it is correctly granted, if not, then set the permissions for them. However, if
the permissions here are changed to 770 or other permissions that may be
affected by the umask in the future, there will be defects. Therefore, from a
rigorous point of view, I think it is necessary to further check the
permissions here.
I have mentioned similar flaws in
[STORM-3862](https://github.com/apache/storm/pull/3478)
,[TEZ-4412](https://github.com/apache/tez/pull/209),[HBASE-26994](https://github.com/apache/hbase/pull/4391)
Finally, regarding the FileSystem.mkdirs(fs, path, perm) you mentioned, I
agree with your point of view, but what I still want to say is that the idea of
this kind of repair is not to set permissions on files, but out of file
permissions Check, as shown in the code, if the permissions are correct, we
don't need to set permissions on it again.
Issue Time Tracking
---
Worklog Id: (was: 837251)
Time Spent: 0.5h (was: 20m)
> Make sure dirPath has the correct permissions
> -
>
> Key: HIVE-26887
> URL: https://issues.apache.org/jira/browse/HIVE-26887
> Project: Hive
> Issue Type: Improvement
>Reporter: Zhang Dongsheng
>Priority: Major
> Labels: pull-request-available
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> In the QueryResultsCache function of class QueryResultsCache, there is the
> following code segment
> {code:java}
> private QueryResultsCache(HiveConf configuration) throws IOException {
> ..
> FileSystem fs = cacheDirPath.getFileSystem(conf);
> FsPermission fsPermission = new FsPermission("700");
> fs.mkdirs(cacheDirPath, fsPermission);
> ..
> }
> {code}
> It can be seen that the function will use the mkdirs to create cacheDirPath,
> and the parameters passed in include the path variable cacheDirPath and a
> permission 700. But we haven't confirmed whether the permission is correctly
> assigned to the file.
> The above question is raised because there are two mkdir functions of hadoop,
> {code:java}
> mkdirs(Path f, FsPermission permission)
> {code}
> and
> {code:java}
> mkdirs(FileSystem fs, Path dir, FsPermission permission)
> {code}
> and the first one is used here. The permissions of this function will be
> affected by the underlying umask. Although 700 here will hardly be affected
> by umask, but I think from a rigorous point of view, we should have one more
> permission check and permission grant here.
> And I find same issue in other three methods here.
> In class Context
> {code:java}
> private Path getScratchDir(String scheme, String authority,
> boolean mkdir, String scratchDir) {
> ..
> FileSystem fs = dirPath.getFileSystem(conf);
> dirPath = new Path(fs.makeQualified(dirPath).toString());
> FsPermission fsPermission = new FsPermission(scratchDirPermission);
> if (!fs.mkdirs(dirPath, fsPermission)) {
> throw new RuntimeException("Cannot make directory: "
> + dirPath.toString());
> ..
> }
> {code}
> In class SessionState
> {code:java}
> static void createPath(HiveConf conf, Path path, String permission, boolean
> isLocal,
> boolean isCleanUp) throws IOException {
> FsPermission fsPermission = new FsPermission(permission);
> FileSystem fs;
> ..
> if (!fs.mkdirs(path, fsPermission)) {
> throw new IOException("Failed to create directory " + path + " on fs "
> + fs.getUri());
> }
> ..
> }
> {code}
> and in class TezSessionState
> {code:java}
> private Path createTezDir(String sessionId, String suffix) throws IOException
> {
> ..
> Path tezDir = new Path(hdfsScratchDir, TEZ_DIR);
> FileSystem fs = tezDir.getFileSystem(conf);
> FsPermission fsPermission = new
[jira] [Work logged] (HIVE-26887) Make sure dirPath has the correct permissions
[
https://issues.apache.org/jira/browse/HIVE-26887?focusedWorklogId=835579=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-835579
]
ASF GitHub Bot logged work on HIVE-26887:
-
Author: ASF GitHub Bot
Created on: 24/Dec/22 14:25
Start Date: 24/Dec/22 14:25
Worklog Time Spent: 10m
Work Description: sonarcloud[bot] commented on PR #3894:
URL: https://github.com/apache/hive/pull/3894#issuecomment-1364537607
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_hive=3894)
[](https://sonarcloud.io/project/issues?id=apache_hive=3894=false=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive=3894=false=BUG)
[0
Bugs](https://sonarcloud.io/project/issues?id=apache_hive=3894=false=BUG)
[](https://sonarcloud.io/project/issues?id=apache_hive=3894=false=VULNERABILITY)
[](https://sonarcloud.io/project/issues?id=apache_hive=3894=false=VULNERABILITY)
[0
Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_hive=3894=false=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive=3894=false=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/security_hotspots?id=apache_hive=3894=false=SECURITY_HOTSPOT)
[0 Security
Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_hive=3894=false=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_hive=3894=false=CODE_SMELL)
[](https://sonarcloud.io/project/issues?id=apache_hive=3894=false=CODE_SMELL)
[1 Code
Smell](https://sonarcloud.io/project/issues?id=apache_hive=3894=false=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_hive=3894=coverage=list)
No Coverage information
[](https://sonarcloud.io/component_measures?id=apache_hive=3894=duplicated_lines_density=list)
No Duplication information
Issue Time Tracking
---
Worklog Id: (was: 835579)
Time Spent: 20m (was: 10m)
> Make sure dirPath has the correct permissions
> -
>
> Key: HIVE-26887
> URL: https://issues.apache.org/jira/browse/HIVE-26887
> Project: Hive
> Issue Type: Improvement
>Reporter: Zhang Dongsheng
>Priority: Major
> Labels: pull-request-available
> Time Spent: 20m
> Remaining Estimate: 0h
>
> In the QueryResultsCache function of class QueryResultsCache, there is the
> following code segment
> {code:java}
> private QueryResultsCache(HiveConf configuration) throws IOException {
> ..
> FileSystem fs = cacheDirPath.getFileSystem(conf);
> FsPermission fsPermission = new FsPermission("700");
> fs.mkdirs(cacheDirPath, fsPermission);
> ..
> }
> {code}
> It can be seen that the function will use the mkdirs to create cacheDirPath,
> and the parameters passed in include the path variable cacheDirPath and a
> permission 700. But we haven't confirmed whether the permission is correctly
> assigned to the file.
> The above question is raised because there are two mkdir functions of hadoop,
> {code:java}
> mkdirs(Path f, FsPermission permission)
> {code}
> and
> {code:java}
> mkdirs(FileSystem fs, Path dir, FsPermission permission)
> {code}
> and the first one is used here. The permissions of this function will be
> affected by the underlying umask. Although 700 here will hardly be affected
> by umask,
[jira] [Work logged] (HIVE-26887) Make sure dirPath has the correct permissions
[
https://issues.apache.org/jira/browse/HIVE-26887?focusedWorklogId=835576=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-835576
]
ASF GitHub Bot logged work on HIVE-26887:
-
Author: ASF GitHub Bot
Created on: 24/Dec/22 13:34
Start Date: 24/Dec/22 13:34
Worklog Time Spent: 10m
Work Description: skysiders opened a new pull request, #3894:
URL: https://github.com/apache/hive/pull/3894
In the QueryResultsCache function of class QueryResultsCache, there is the
following code segment
```java
private QueryResultsCache(HiveConf configuration) throws IOException {
..
FileSystem fs = cacheDirPath.getFileSystem(conf);
FsPermission fsPermission = new FsPermission("700");
fs.mkdirs(cacheDirPath, fsPermission);
..
}
```
It can be seen that the function will use the mkdirs to create cacheDirPath,
and the parameters passed in include the path variable cacheDirPath and a
permission 700. But we haven't confirmed whether the permission is correctly
assigned to the file.
The above question is raised because there are two mkdir functions of
hadoop,`mkdirs(Path f, FsPermission permission)` with `mkdirs(FileSystem fs,
Path dir, FsPermission permission)`and the first one is used here. The
permissions of this function will be affected by the underlying umask. Although
700 here will hardly be affected by umask, but I think from a rigorous point of
view, we should have one more permission check and permission grant here. So I
judge whether the file permission is correctly granted by detecting the
influence of umask on the permission, and if not, give it the correct
permission through setPermission.
```java
if
(!permission.equals(permission.applyUMask(FsPermission.getUMask(conf {
fs.setPermission(dir, permission);
}
```
And I find same issue in other three methods here.
In class Context
```java
private Path getScratchDir(String scheme, String authority,
boolean mkdir, String scratchDir) {
..
FileSystem fs = dirPath.getFileSystem(conf);
dirPath = new Path(fs.makeQualified(dirPath).toString());
FsPermission fsPermission = new FsPermission(scratchDirPermission);
if (!fs.mkdirs(dirPath, fsPermission)) {
throw new RuntimeException("Cannot make directory: "
+ dirPath.toString());
..
}
```
In class SessionState
```java
static void createPath(HiveConf conf, Path path, String permission,
boolean isLocal,
boolean isCleanUp) throws IOException {
FsPermission fsPermission = new FsPermission(permission);
FileSystem fs;
..
if (!fs.mkdirs(path, fsPermission)) {
throw new IOException("Failed to create directory " + path + " on fs "
+ fs.getUri());
}
..
}
```
and in class TezSessionState
```java
private Path createTezDir(String sessionId, String suffix) throws
IOException {
..
Path tezDir = new Path(hdfsScratchDir, TEZ_DIR);
FileSystem fs = tezDir.getFileSystem(conf);
FsPermission fsPermission = new FsPermission(HiveConf.getVar(conf,
HiveConf.ConfVars.SCRATCHDIRPERMISSION));
fs.mkdirs(tezDir, fsPermission);
..
}
```
Issue Time Tracking
---
Worklog Id: (was: 835576)
Remaining Estimate: 0h
Time Spent: 10m
> Make sure dirPath has the correct permissions
> -
>
> Key: HIVE-26887
> URL: https://issues.apache.org/jira/browse/HIVE-26887
> Project: Hive
> Issue Type: Improvement
>Reporter: Zhang Dongsheng
>Priority: Major
> Time Spent: 10m
> Remaining Estimate: 0h
>
> In the QueryResultsCache function of class QueryResultsCache, there is the
> following code segment
> {code:java}
> private QueryResultsCache(HiveConf configuration) throws IOException {
> ..
> FileSystem fs = cacheDirPath.getFileSystem(conf);
> FsPermission fsPermission = new FsPermission("700");
> fs.mkdirs(cacheDirPath, fsPermission);
> ..
> }
> {code}
> It can be seen that the function will use the mkdirs to create cacheDirPath,
> and the parameters passed in include the path variable cacheDirPath and a
> permission 700. But we haven't confirmed whether the permission is correctly
> assigned to the file.
> The above question is raised because there are two mkdir functions of hadoop,
> {code:java}
> mkdirs(Path f, FsPermission permission)
> {code}
> and
> {code:java}
> mkdirs(FileSystem fs, Path dir, FsPermission permission)
> {code}
> and the first one is used here. The permissions of this function will be
>
