You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/05/02 11:37:22 UTC
[GitHub] [pulsar] skysiders opened a new pull request, #15412: [fix][Broker] NarUnpacker create directory and file without permission check
skysiders opened a new pull request, #15412:
URL: https://github.com/apache/pulsar/pull/15412
*(If this PR fixes a github issue, please add `Fixes #<xyz>`.)*
Fixes #15411
### Motivation
While NarUnpacker create directory and files , it forget to check if the directory or file have write or others permission they need.So I create new method to setPermission for them.
### Modifications
Add permission check for directory and file create in NapUnpacker.
### Documentation
Need to update docs?
- [x] `no-need-doc`
(Please explain why)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on pull request #15412: [fix][Broker] NarUnpacker create directory and file without permission check
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #15412:
URL: https://github.com/apache/pulsar/pull/15412#issuecomment-1345278398
After another look, I believe this patch should be improve especially to handle IO exception and syscall consistently.
If we tolerate all failure fs operations, we should not throw IOException.
Close as stale. Please rebase and resubmit the patch if it's still relevant - with comments addressed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #15412: [fix][Broker] NarUnpacker create directory and file without permission check
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15412:
URL: https://github.com/apache/pulsar/pull/15412#issuecomment-1229102313
The pr had no activity for 30 days, mark with Stale label.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] skysiders commented on a diff in pull request #15412: [fix][Broker] NarUnpacker create directory and file without permission check
Posted by GitBox <gi...@apache.org>.
skysiders commented on code in PR #15412:
URL: https://github.com/apache/pulsar/pull/15412#discussion_r863896269
##########
pulsar-common/src/main/java/org/apache/pulsar/common/nar/NarUnpacker.java:
##########
@@ -87,6 +92,7 @@ static File doUnpackNar(final File nar, final File baseWorkingDirectory, Runnabl
FileLock lock = channel.lock()) {
File narWorkingDirectory = new File(parentDirectory, md5Sum);
if (narWorkingDirectory.mkdir()) {
+ setPosixFilePermissions(narWorkingDirectory,"rwxr-x---");
Review Comment:
Thank for your review. Not only mkdirs/mkdir can create new directory which need set permission, there are some other method can create new file such as FileOutputStream if file do not exist first.So I think we need to create file then set some special permission such as w permission for file before FileOutputStream.I also think like you that a new method should be created to create the file or directory. I will try to solve it.Thanks again!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] skysiders commented on pull request #15412: [fix][Broker] NarUnpacker create directory and file without permission check
Posted by GitBox <gi...@apache.org>.
skysiders commented on PR #15412:
URL: https://github.com/apache/pulsar/pull/15412#issuecomment-1114753097
Hi @Jason918 , I found some permission question in NarUnpacker , can you please take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] skysiders commented on pull request #15412: [fix][Broker] NarUnpacker create directory and file without permission check
Posted by GitBox <gi...@apache.org>.
skysiders commented on PR #15412:
URL: https://github.com/apache/pulsar/pull/15412#issuecomment-1118271575
Hi @Jason918 , I add some methods to create directory and file with special permission to ensure it's permission.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] skysiders commented on a diff in pull request #15412: [fix][Broker] NarUnpacker create directory and file without permission check
Posted by GitBox <gi...@apache.org>.
skysiders commented on code in PR #15412:
URL: https://github.com/apache/pulsar/pull/15412#discussion_r886905189
##########
pulsar-common/src/test/java/org/apache/pulsar/common/nar/NarUnpackerTest.java:
##########
@@ -157,4 +160,17 @@ File findJavaExe() {
File javaExe = new File(javaHome, "bin/java" + (SystemUtils.IS_OS_WINDOWS ? ".exe" : ""));
return javaExe;
}
-}
\ No newline at end of file
+ @Test
+ public void testForPermissionSet() throws IOException{
+ File file = File.createTempFile("TestFilePerm", "test");
+ file.delete();
Review Comment:
Thanks @Jason918 ,I fix this problem in this commit. Could you please have a look at this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on a diff in pull request #15412: [fix][Broker] NarUnpacker create directory and file without permission check
Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #15412:
URL: https://github.com/apache/pulsar/pull/15412#discussion_r863791836
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/RocksdbMetadataStore.java:
##########
@@ -220,6 +223,8 @@ private RocksdbMetadataStore(String metadataURL, MetadataStoreConfig metadataSto
Path dataPath = FileSystems.getDefault().getPath(dataDir);
try {
Files.createDirectories(dataPath);
+ Set<PosixFilePermission> perms = PosixFilePermissions.fromString("rwx------");
Review Comment:
Not in the scope of this PR.
##########
pulsar-common/src/main/java/org/apache/pulsar/common/nar/NarUnpacker.java:
##########
@@ -175,4 +180,51 @@ private static byte[] calculateMd5sum(final File file) throws IOException {
throw new IllegalArgumentException(nsae);
}
}
+
+ private static boolean setPosixFilePermissions(File file, String perm) throws IOException {
+ Set<PosixFilePermission> perms = PosixFilePermissions.fromString(perm);
+ try {
+ Files.setPosixFilePermissions(file.toPath(), perms);
+ log.info("Set permissions on " + file.getAbsolutePath() + " with permission " + perm);
+ return true;
+ } catch (IOException e) {
+ log.warn("Failed to set file permissions on " + file.getAbsolutePath());
+ return false;
+ }
+ }
+
+ private static boolean createDirWithSpecialPermission(File file, String permission, boolean createParent)
+ throws IOException {
+ if (file.exists()) {
+ log.warn("Directory " + file.toString() + " exist.");
+ return false;
+ }
+ try {
+ if (createParent) {
+ File parent = file.getParentFile();
+ if (parent == null) {
+ log.warn("Parent File is null");
+ return false;
+ }
+ parent.mkdirs();
+ }
+ file.mkdir();
Review Comment:
```suggestion
if (createParent) {
file.mkdirs();
} else {
file.mkdir();
}
```
This is simpler?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #15412: [fix][Broker] NarUnpacker create directory and file without permission check
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15412:
URL: https://github.com/apache/pulsar/pull/15412#issuecomment-1172997128
The pr had no activity for 30 days, mark with Stale label.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] skysiders commented on pull request #15412: [fix][Broker] NarUnpacker create directory and file without permission check
Posted by GitBox <gi...@apache.org>.
skysiders commented on PR #15412:
URL: https://github.com/apache/pulsar/pull/15412#issuecomment-1123951058
>
Yes it's better to add UT here.But the method is private it is not easy to write UT here.Could I change new method to public?Because it seem has no impact here if new method is public.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun closed pull request #15412: [fix][Broker] NarUnpacker create directory and file without permission check
Posted by GitBox <gi...@apache.org>.
tisonkun closed pull request #15412: [fix][Broker] NarUnpacker create directory and file without permission check
URL: https://github.com/apache/pulsar/pull/15412
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] skysiders commented on pull request #15412: [fix][Broker] NarUnpacker create directory and file without permission check
Posted by GitBox <gi...@apache.org>.
skysiders commented on PR #15412:
URL: https://github.com/apache/pulsar/pull/15412#issuecomment-1195451707
Hi @Technoboy- , could you please take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on a diff in pull request #15412: [fix][Broker] NarUnpacker create directory and file without permission check
Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #15412:
URL: https://github.com/apache/pulsar/pull/15412#discussion_r874291849
##########
pulsar-common/src/test/java/org/apache/pulsar/common/nar/NarUnpackerTest.java:
##########
@@ -157,4 +160,17 @@ File findJavaExe() {
File javaExe = new File(javaHome, "bin/java" + (SystemUtils.IS_OS_WINDOWS ? ".exe" : ""));
return javaExe;
}
-}
\ No newline at end of file
+ @Test
+ public void testForPermissionSet() throws IOException{
+ File file = File.createTempFile("TestFilePerm", "test");
+ file.delete();
Review Comment:
please use space instead of tab.
```suggestion
file.delete();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on pull request #15412: [fix][Broker] NarUnpacker create directory and file without permission check
Posted by GitBox <gi...@apache.org>.
Jason918 commented on PR #15412:
URL: https://github.com/apache/pulsar/pull/15412#issuecomment-1125846302
> >
>
> Yes it's better to add UT here.But the method is private it is not easy to write UT here.Could I change new method to public?Because it seem has no impact here if new method is public.
You can checkout `@VisibleForTesting` for reference.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] skysiders commented on pull request #15412: [fix][Broker] NarUnpacker create directory and file without permission check
Posted by GitBox <gi...@apache.org>.
skysiders commented on PR #15412:
URL: https://github.com/apache/pulsar/pull/15412#issuecomment-1126904510
> > Yes it's better to add UT here.But the method is private it is not easy to write UT here.Could I change new method to public?Because it seem has no impact here if new method is public.
>
> You can checkout `@VisibleForTesting` for reference.
Hi @Jason918 ,Thanks for your review. I try to use`@VisibleForTesting` and add UT to test new method in this patch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on a diff in pull request #15412: [fix][Broker] NarUnpacker create directory and file without permission check
Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #15412:
URL: https://github.com/apache/pulsar/pull/15412#discussion_r863791834
##########
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/RocksdbMetadataStore.java:
##########
@@ -220,6 +223,8 @@ private RocksdbMetadataStore(String metadataURL, MetadataStoreConfig metadataSto
Path dataPath = FileSystems.getDefault().getPath(dataDir);
try {
Files.createDirectories(dataPath);
+ Set<PosixFilePermission> perms = PosixFilePermissions.fromString("rwx------");
Review Comment:
Not in the scope of this PR.
##########
pulsar-common/src/main/java/org/apache/pulsar/common/nar/NarUnpacker.java:
##########
@@ -75,6 +79,7 @@ static File doUnpackNar(final File nar, final File baseWorkingDirectory, Runnabl
throw new IOException("Cannot create " + parentDirectory);
}
}
+ setPosixFilePermissions(parentDirectory,"rwxr-x---");
Review Comment:
This only need to set after `mkdirs`.
It's better to create a new method like `makeDirs(File)` and set permission there.
##########
pulsar-common/src/main/java/org/apache/pulsar/common/nar/NarUnpacker.java:
##########
@@ -75,6 +79,7 @@ static File doUnpackNar(final File nar, final File baseWorkingDirectory, Runnabl
throw new IOException("Cannot create " + parentDirectory);
}
}
+ setPosixFilePermissions(parentDirectory,"rwxr-x---");
Review Comment:
This only need to set after `mkdirs`.
It's better to create a new method like `makeDirs(File)` and set permission there.
##########
pulsar-common/src/main/java/org/apache/pulsar/common/nar/NarUnpacker.java:
##########
@@ -87,6 +92,7 @@ static File doUnpackNar(final File nar, final File baseWorkingDirectory, Runnabl
FileLock lock = channel.lock()) {
File narWorkingDirectory = new File(parentDirectory, md5Sum);
if (narWorkingDirectory.mkdir()) {
+ setPosixFilePermissions(narWorkingDirectory,"rwxr-x---");
Review Comment:
And please check code format.
##########
pulsar-common/src/main/java/org/apache/pulsar/common/nar/NarUnpacker.java:
##########
@@ -87,6 +92,7 @@ static File doUnpackNar(final File nar, final File baseWorkingDirectory, Runnabl
FileLock lock = channel.lock()) {
File narWorkingDirectory = new File(parentDirectory, md5Sum);
if (narWorkingDirectory.mkdir()) {
+ setPosixFilePermissions(narWorkingDirectory,"rwxr-x---");
Review Comment:
And please check code format.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on a diff in pull request #15412: [fix][Broker] NarUnpacker create directory and file without permission check
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #15412:
URL: https://github.com/apache/pulsar/pull/15412#discussion_r1045091920
##########
pulsar-common/src/main/java/org/apache/pulsar/common/nar/NarUnpacker.java:
##########
@@ -69,7 +73,7 @@ static File doUnpackNar(final File nar, final File baseWorkingDirectory, Runnabl
throws IOException {
File parentDirectory = new File(baseWorkingDirectory, nar.getName() + "-unpacked");
if (!parentDirectory.exists()) {
- if (parentDirectory.mkdirs()) {
+ if (createDirWithSpecialPermission(parentDirectory, "rwxr-x---", true)) {
Review Comment:
I'd prefer to use constants for these perm settings. But it should not be a blocker.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org