GEODE-10531: Remove SecurityManager Usage from OSProcess.java - Java 21 Blocker - #7971
sboorlagadda wants to merge 7 commits into
Conversation
- Phase 0 (Critical Java 21 Blockers) is now complete - Updated warning baseline from 41 to 39 total warnings - Marked SecurityManager API removal as resolved - Added completion details and lessons learned - Updated project status to focus on Phase 1 (Spring Framework blockers)
There was a problem hiding this comment.
Pull request overview
This PR addresses GEODE-10531, removing deprecated SecurityManager APIs from OSProcess.java that were removed in Java 21, thereby unblocking the future migration to Java 21. The change is part of a larger initiative (GEODE-10479) to remove all deprecation warnings from the Apache Geode codebase following the Java 17 migration.
Key Changes:
- Removed SecurityManager references and checks from
OSProcess.bgexec()method - Updated JavaDoc to remove references to
SecurityManagerandSecurityManager#checkExec() - Security functionality now relies on OS/JVM security models rather than the deprecated SecurityManager API
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| proposals/GEODE-10479/todo.md | Tracks completion of Phase 0 critical Java 21 blocker (SecurityManager removal), marking task as complete with detailed implementation notes |
| proposals/GEODE-10479/spec.md | Technical specification document for the overall Java 17 deprecation warning removal project (new file) |
| proposals/GEODE-10479/plan.md | Comprehensive implementation plan for the deprecation removal project including prompts for LLM code generation (new file) |
| proposals/GEODE-10479/issue.md | JIRA-formatted issue description for the overall deprecation removal effort (new file) |
| proposals/GEODE-10479/GEODE-10534.md | Documentation for fixing 13 deprecation warnings across 4 support modules (new file) |
| proposals/GEODE-10479/GEODE-10533.md | Documentation for fixing 23 warnings in geode-gfsh module (new file) |
| proposals/GEODE-10479/GEODE-10532.md | Documentation for high-priority Spring Framework getRawStatusCode() removal warnings (new file) |
| proposals/GEODE-10479/GEODE-10531.md | Issue documentation for this specific SecurityManager removal task (new file) |
| geode-logging/src/main/java/org/apache/geode/logging/internal/OSProcess.java | Removed 6 lines containing SecurityManager API usage and JavaDoc references that block Java 21 compilation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
JinwooHwang
left a comment
JinwooHwang
left a comment
There was a problem hiding this comment.
Hi @sboorlagadda. Thank you for addressing this Java 21 blocker - removing the deprecated SecurityManager usage is definitely necessary for forward compatibility.
While I understand that SecurityManager removal is mandatory for Java 21 compilation, I'm concerned about completely removing the subprocess execution security check without providing any replacement mechanism. Since this check previously acted as a safeguard against unauthorized command invocation, could you clarify how equivalent protections will be maintained? If no replacement is planned, could you please help me understand the reasoning for why this execution path no longer requires any guardrail.
There was a problem hiding this comment.
🟡 Changes recommended
Align the documented compatibility exception with the PR criteria and make the integration test reliable for paths containing spaces.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
geode-logging/src/integrationTest/java/org/apache/geode/logging/internal/OSProcessSecurityIntegrationTest.java:50
TemporaryFoldercan produce a path containing spaces, whileOSProcess.bgexecappendslogfile.getPath()unquoted to the shell redirection (OSProcess.java:283-285). On a valid Windows or Unix temp directory with spaces, this probe will redirect to the wrong path and fail to observe the child output, making the new cross-platform test flaky/failing. Use a whitespace-free test path or fix the redirection quoting inbgexec.
File output = temporaryFolder.newFile("probe-output.txt");
List<String> command = new ArrayList<>();
proposals/GEODE-10479/README.md:47
- This implementation deliberately retains the deprecated SecurityManager policy, so the revised acceptance criteria require an explicit agreement before merge. The PR description still presents the original “Remove SecurityManager Usage” scope and generic checklist, without recording this exception or the revised criteria; please update the Jira/PR acceptance criteria and obtain that agreement before merging.
- Record the agreed policy in the issue and PR description before merge.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
| // GEODE-10531: ProcessBuilder checks the shell, not the requested executable. Retain | ||
| // this check until support for the legacy SecurityManager policy is explicitly retired. | ||
| @SuppressWarnings("removal") | ||
| SecurityManager security = System.getSecurityManager(); |
f28e917 to
0469007
Compare
For all changes, please confirm:
develop)?gradlew buildrun cleanly?