Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe PR centralizes WinFsp discovery and DLL path selection by adding a new WinFspUtil class that locates and caches the WinFsp DLL path (selecting aarch64 vs x64), reads the install directory via the registry with a 3s timeout, and falls back to C:\Program Files (x86)\WinFsp\ if registry lookup fails. It removes the WinFspNotFoundException and adjusts existing callers (WinFspMountProvider, WinfspUtil) to use the new helper and simplified existence checks. Logging for registry success and fallback usage was added. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/org/cryptomator/frontend/fuse/mount/WinfspUtil.java (1)
32-37: Consider usingupdateAndGetfor atomic initialization.The current check-then-act pattern allows multiple threads to redundantly call
readWinFspInstallDirFromRegistry(). While functionally correct (idempotent), this can be simplified:♻️ Suggested atomic initialization pattern
static String getWinFspInstallDir() { - if (cache.get() == null) { - cache.set(readWinFspInstallDirFromRegistry()); - } - return cache.get(); + return cache.updateAndGet(current -> current != null ? current : readWinFspInstallDirFromRegistry()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/cryptomator/frontend/fuse/mount/WinfspUtil.java` around lines 32 - 37, Replace the current check-then-act in getWinFspInstallDir with an atomic update so multiple threads won't redundantly call readWinFspInstallDirFromRegistry; use cache.updateAndGet(...) (or cache.compareAndSet loop) to call readWinFspInstallDirFromRegistry only when cache is null, returning the stored value, and keep references to the existing symbols getWinFspInstallDir, cache, and readWinFspInstallDirFromRegistry to locate and modify the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/cryptomator/frontend/fuse/mount/WinfspUtil.java`:
- Around line 59-61: The current isWinFspInstalled() uses
Files.exists(Path.of(getWinFspInstallDir())) which can return true for leftover
directories; update isWinFspInstalled() to verify presence of required WinFsp
binaries (e.g., check for the expected DLLs such as "winfsp-x64.dll" and/or
"winfsp.dll" inside getWinFspInstallDir()/bin) instead of just the directory,
and make isSupported() rely on that stronger check so it only advertises WinFsp
when those binaries are present.
---
Nitpick comments:
In `@src/main/java/org/cryptomator/frontend/fuse/mount/WinfspUtil.java`:
- Around line 32-37: Replace the current check-then-act in getWinFspInstallDir
with an atomic update so multiple threads won't redundantly call
readWinFspInstallDirFromRegistry; use cache.updateAndGet(...) (or
cache.compareAndSet loop) to call readWinFspInstallDirFromRegistry only when
cache is null, returning the stored value, and keep references to the existing
symbols getWinFspInstallDir, cache, and readWinFspInstallDirFromRegistry to
locate and modify the logic.
* rename method getWinFspInstallDir ot getWinFspDLLPath * resolve dll in util * add doc
reducing risk by path manipulation
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/cryptomator/frontend/fuse/mount/WinFspUtil.java`:
- Around line 35-38: The DLL path is built by string concatenation which breaks
when getWinFspInstallDir() lacks a trailing separator; update WinFspUtil so
libPath is constructed using java.nio.file.Path joining (e.g., Path.of(...) or
Paths.get(...) and .resolve("bin").resolve(dllName)). Replace the current
installDir + "bin\\" + dllName expression with a Path-based join, then pass the
resulting toString() (or Path) into cache.set so the path is always correct
regardless of trailing separators.
- Around line 50-66: The reg invocation is brittle and the child Process `p`
isn't cleaned up on timeout; in WinFspUtil (the method that reads
REG_WINFSP_KEY/REG_WINFSP_VALUE and returns FALLBACK_PATH) change the
ProcessBuilder to call reg.exe by absolute path (e.g. System.getenv("windir") +
"\\\\System32\\\\reg.exe") instead of just "reg", keep `p` reference in a scope
visible to the catch handlers, and on TimeoutException (and other failure paths
where the process may still be running) call p.destroyForcibly() and wait for
termination before continuing; preserve the existing InterruptedException
handling (restore interrupt flag) and still return FALLBACK_PATH on failure.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/cryptomator/frontend/fuse/mount/WinFspMountProvider.javasrc/main/java/org/cryptomator/frontend/fuse/mount/WinFspUtil.java
src/main/java/org/cryptomator/frontend/fuse/mount/WinFspUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/frontend/fuse/mount/WinFspUtil.java
Outdated
Show resolved
Hide resolved
2 tasks
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/main/java/org/cryptomator/frontend/fuse/mount/WinFspUtil.java (2)
34-38:⚠️ Potential issue | 🟠 MajorBuild the DLL path with
Pathjoining to avoid malformed paths.Line 37 still depends on a trailing separator in
installDir; registry values without it produce invalid paths (e.g.,...WinFspbin\...).🔧 Proposed fix
static String getWinFspDLLPath() { if (cache.get() == null) { var installDir = getWinFspInstallDir(); var dllName = (System.getProperty("os.arch").toLowerCase().contains("aarch64") ? "winfsp-a64.dll" : "winfsp-x64.dll"); - var libPath = installDir + "bin\\" + dllName; + var libPath = Path.of(installDir, "bin", dllName).toString(); cache.set(libPath); } return cache.get(); }#!/bin/bash # Verify unresolved string-concatenated DLL path construction. rg -nP --type=java 'installDir\s*\+\s*"bin\\\\"\s*\+\s*dllName'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/cryptomator/frontend/fuse/mount/WinFspUtil.java` around lines 34 - 38, The current construction of libPath in WinFspUtil relies on string concatenation of installDir + "bin\\" + dllName and breaks if getWinFspInstallDir() does not include a trailing separator; change this to build the path using java.nio.file.Path/Paths (e.g., Paths.get(installDir, "bin", dllName).toString()) so the OS-specific separators are handled correctly, then store that resulting string into cache via cache.set(...), keeping the existing cache.get() / cache.set(libPath) logic and the dllName selection intact.
50-66:⚠️ Potential issue | 🟠 MajorHarden
reg.exelookup and ensure child process cleanup on failure paths.Line 51 hardcodes
C:\Windows\System32\reg.exe(breaks non-standard Windows roots), and timeout/error paths do not forcibly terminate a possibly running child process.🔧 Proposed fix
static String getWinFspInstallDir() { + Process p = null; try { - ProcessBuilder command = new ProcessBuilder("C:\\Windows\\System32\\reg.exe", "query", REG_WINFSP_KEY, "/v", REG_WINFSP_VALUE); - Process p = command.start(); + var systemRoot = System.getenv().getOrDefault("SystemRoot", "C:\\Windows"); + var regExe = Path.of(systemRoot, "System32", "reg.exe").toString(); + ProcessBuilder command = new ProcessBuilder(regExe, "query", REG_WINFSP_KEY, "/v", REG_WINFSP_VALUE); + p = command.start(); ProcessHelper.waitForSuccess(p, 3, "`reg query`"); try (var reader = p.inputReader(StandardCharsets.UTF_8)) { String result = reader.lines().filter(l -> l.contains(REG_WINFSP_VALUE)).findFirst().orElseThrow(); var installDir = result.substring(result.indexOf(REGSTR_TOKEN) + REGSTR_TOKEN.length()).trim(); LOG.debug("Successfully read WinFsp directory {} from registry.", installDir); return installDir; } } catch (TimeoutException | IOException | ProcessHelper.CommandFailedException | NoSuchElementException e) { //no-op } catch (InterruptedException e) { Thread.currentThread().interrupt(); + } finally { + if (p != null && p.isAlive()) { + p.destroyForcibly(); + } } LOG.debug("Failed to read WinFsp directory from registry. Using fallback path {}", FALLBACK_PATH); return FALLBACK_PATH; }#!/bin/bash # Verify hardcoded reg.exe path and missing process cleanup patterns. rg -nP --type=java 'new\s+ProcessBuilder\("C:\\\\Windows\\\\System32\\\\reg\.exe"' rg -nP --type=java 'destroyForcibly\s*\('🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/cryptomator/frontend/fuse/mount/WinFspUtil.java` around lines 50 - 66, Replace the hardcoded "C:\\Windows\\System32\\reg.exe" in the ProcessBuilder call with just "reg.exe" so the system path is respected (locate the ProcessBuilder instantiation in WinFspUtil); additionally, ensure the child Process referenced as p is always cleaned up on failure paths by capturing it in a variable visible to the catch blocks and calling p.destroyForcibly() (and optionally p.waitFor()) in the TimeoutException/IOException/CommandFailedException catch and after InterruptedException handling, making sure to re-set the interrupt for InterruptedException and to still return FALLBACK_PATH when cleanup completes; keep the existing use of ProcessHelper.waitForSuccess and the inputReader usage but guarantee the process is forcibly terminated on all non-success paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/org/cryptomator/frontend/fuse/mount/WinFspUtil.java`:
- Around line 34-38: The current construction of libPath in WinFspUtil relies on
string concatenation of installDir + "bin\\" + dllName and breaks if
getWinFspInstallDir() does not include a trailing separator; change this to
build the path using java.nio.file.Path/Paths (e.g., Paths.get(installDir,
"bin", dllName).toString()) so the OS-specific separators are handled correctly,
then store that resulting string into cache via cache.set(...), keeping the
existing cache.get() / cache.set(libPath) logic and the dllName selection
intact.
- Around line 50-66: Replace the hardcoded "C:\\Windows\\System32\\reg.exe" in
the ProcessBuilder call with just "reg.exe" so the system path is respected
(locate the ProcessBuilder instantiation in WinFspUtil); additionally, ensure
the child Process referenced as p is always cleaned up on failure paths by
capturing it in a variable visible to the catch blocks and calling
p.destroyForcibly() (and optionally p.waitFor()) in the
TimeoutException/IOException/CommandFailedException catch and after
InterruptedException handling, making sure to re-set the interrupt for
InterruptedException and to still return FALLBACK_PATH when cleanup completes;
keep the existing use of ProcessHelper.waitForSuccess and the inputReader usage
but guarantee the process is forcibly terminated on all non-success paths.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.