Ethanlm commented on a change in pull request #3362:
URL: https://github.com/apache/storm/pull/3362#discussion_r543498772
##########
File path:
storm-server/src/test/java/org/apache/storm/utils/ServerUtilsTest.java
##########
@@ -106,7 +116,15 @@ public void
testExtractZipFileDisallowsPathTraversalWhenUsingPrefix() throws Exc
continue;
}
try {
- pids.add(Long.parseLong(line.split("\\s")[0]));
+ String pidStr = line.split("\\s")[0];
Review comment:
indention seems off
##########
File path:
storm-server/src/test/java/org/apache/storm/utils/ServerUtilsTest.java
##########
@@ -272,18 +290,28 @@ public void
testIsAnyPosixProcessPidDirAliveMockingFileOwnerUid() throws Excepti
LOG.info("Test testIsAnyPosixProcessPidDirAlive is designed to run
on systems with /proc directory only, marking as success");
return;
}
- Collection<Long> pids = getRunningProcessIds();
- assertFalse(pids.isEmpty());
+ Collection<Long> allPids = getRunningProcessIds(null);
+ Collection<Long> rootPids = getRunningProcessIds("root");
+ assertFalse(allPids.isEmpty());
+ assertFalse(rootPids.isEmpty());
- for (int i = 0 ; i < 2 ; i++) {
- boolean mockFileOwnerToUid = (i % 2 == 0);
+ String currentUser = System.getProperty("user.name");
+
+ for (boolean mockFileOwnerToUid: Arrays.asList(true, false)) {
// at least one pid will be owned by the current user (doing the
testing)
- String currentUser = System.getProperty("user.name");
- boolean status = ServerUtils.isAnyPosixProcessPidDirAlive(pids,
currentUser, mockFileOwnerToUid);
+ boolean status = ServerUtils.isAnyPosixProcessPidDirAlive(allPids,
currentUser, mockFileOwnerToUid);
String err = String.format("(mockFileOwnerToUid=%s) Expecting user
%s to own at least one process",
mockFileOwnerToUid, currentUser);
assertTrue(err, status);
}
+
+ // simulate reassignment of all process id to a different user (root)
+ for (boolean mockFileOwnerToUid: Arrays.asList(true, false)) {
Review comment:
Thanks for adding the unit test
##########
File path:
storm-server/src/test/java/org/apache/storm/utils/ServerUtilsTest.java
##########
@@ -272,18 +290,28 @@ public void
testIsAnyPosixProcessPidDirAliveMockingFileOwnerUid() throws Excepti
LOG.info("Test testIsAnyPosixProcessPidDirAlive is designed to run
on systems with /proc directory only, marking as success");
return;
}
- Collection<Long> pids = getRunningProcessIds();
- assertFalse(pids.isEmpty());
+ Collection<Long> allPids = getRunningProcessIds(null);
Review comment:
It doesn't seem very necessary to get all pids. We just need the pids
from currentUser (`System.getProperty("user.name")`)
##########
File path: storm-server/src/main/java/org/apache/storm/utils/ServerUtils.java
##########
@@ -1309,8 +1309,6 @@ public static boolean
isAnyPosixProcessPidDirAlive(Collection<Long> pids, String
+ "likely pid {} was reused for a new process for
expectedUser {}",
pidDir, actualUser, expectedUser, pid, actualUser);
Review comment:
Looks like the log should be `a new process for actualUser {}"`
And is it possible to print out the cmdline of this processId so we know
what the process really is? It seems unlikely that a processId is reused by
another user (or root) so quickly. I hope to get more details when this
happens. Thanks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
