fix/feat: extract XAudio DLLs from DirectX redistributables#1184
Open
joshuatam wants to merge 13 commits intoutkarshdalal:masterfrom
Open
fix/feat: extract XAudio DLLs from DirectX redistributables#1184joshuatam wants to merge 13 commits intoutkarshdalal:masterfrom
joshuatam wants to merge 13 commits intoutkarshdalal:masterfrom
Conversation
Adds support for decompressing and installing DirectX audio components (XAudio, XACT, X3DAudio) from game cabinet files into the Wine prefix. This uses 7-Zip bindings to extract the necessary DLLs to system folders, helping resolve audio compatibility issues in games that rely on specific redistributable versions.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds SevenZip JBinding and JitPack repo; introduces XServerScreenUtils.replaceXAudioDllsFromRedistributable to extract XAudio-related DLLs from DirectX redistributables into Wine prefixes; invokes that replacement during XServerScreen's Wine setup sequence. Changes
Sequence DiagramsequenceDiagram
participant XS as XServerScreen
participant US as XServerScreenUtils
participant SZ as SevenZipJBinding
participant FS as FileSystem
XS->>US: replaceXAudioDllsFromRedistributable(context, appId)
US->>FS: resolve game dir, Wine prefix, DirectX redistributable (CommonRedist or DXSETUP)
FS-->>US: list of CAB files (xaudio/xact/x3daudio)
loop per CAB
US->>SZ: open CAB archive
SZ-->>US: enumerate entries
US->>FS: determine target (system32 or syswow64)
loop per .dll entry
US->>SZ: extract entry via IArchiveExtractCallback (stream)
SZ-->>FS: write DLL to target (overwrite)
end
US->>SZ: close archive
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt`:
- Around line 1626-1629: ContainerUtils.extractGameSourceFromContainerId
currently defaults unknown prefixes to STEAM and its failure can abort setup;
change the call site to treat extraction as best-effort by using a
non-defaulting check (or a safe variant) and only call
SteamUtils.replaceXAudioDllsFromRedistributable when the extracted value is
explicitly GameSource.STEAM; additionally wrap the call in a try/catch so any
replacement errors are swallowed/logged and do not abort the setup path (refer
to ContainerUtils.extractGameSourceFromContainerId, GameSource.STEAM,
SteamUtils.replaceXAudioDllsFromRedistributable and the appId variable).
In `@app/src/main/java/app/gamenative/utils/SteamUtils.kt`:
- Around line 1429-1433: Archive entry names from archive.getProperty(...) are
used directly to create File(targetDir, name.lowercase()), allowing path
traversal (e.g., "../") to escape targetDir; fix by normalizing and validating
the resolved path before writing: compute val candidate = File(targetDir,
name.lowercase()).canonicalFile and ensure
candidate.startsWith(targetDir.canonicalFile) (or compare paths) and only then
create parent dirs and write; skip or log any entries that fail this containment
check and also ensure directories are created with candidate.parentFile.mkdirs()
before writing.
- Around line 1437-1457: The extraction currently writes directly to outFile and
ignores the ExtractOperationResult; change the IArchiveExtractCallback
implementation used in archive.extract to write to a temporary file (e.g.,
tempOutFile), capture and store the extractOperationResult in
setOperationResult(), and after archive.extract completes verify the stored
result equals ExtractOperationResult.OK before atomically replacing outFile
(move/rename tempOutFile to outFile). On failure, delete tempOutFile and log a
warning via Timber.tag("replaceXAudioDllsFromRedistributable") with the actual
result; ensure getStream writes to tempOutFile and that setOperationResult,
setCompleted, and setTotal are implemented to record/check outcomes.
In `@settings.gradle.kts`:
- Around line 12-14: Remove the JitPack maven repository from the
pluginManagement block and instead add it under dependencyResolutionManagement
but only for the specific groups that need it; update the
dependencyResolutionManagement repositoriesMode/block to include JitPack for the
group filters com.github.omicronapps (sevenzipjbinding),
com.github.penfeizhou.android.animation (apng) and com.github.luben (zstd-jni)
so those artifacts can resolve while keeping pluginManagement clean (adjust the
repositories or componentMetadataRules/gradle module resolution for the groups
in dependencyResolutionManagement to limit JitPack exposure).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b43d7aea-ccbe-48a4-9ae7-9fe009716146
📒 Files selected for processing (5)
app/build.gradle.ktsapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/utils/SteamUtils.ktgradle/libs.versions.tomlsettings.gradle.kts
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
Outdated
Show resolved
Hide resolved
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
4 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="settings.gradle.kts">
<violation number="1" location="settings.gradle.kts:12">
P2: JitPack is unnecessary in `pluginManagement` — none of the project's plugins (`android.application`, `compose.compiler`) are hosted there. This adds an unnecessary repository that slows Gradle plugin resolution and increases supply-chain attack surface. Remove it from `pluginManagement`; the JitPack entry in `dependencyResolutionManagement` is sufficient for library dependencies.</violation>
</file>
<file name="app/src/main/java/app/gamenative/utils/SteamUtils.kt">
<violation number="1" location="app/src/main/java/app/gamenative/utils/SteamUtils.kt:1432">
P1: Archive entry path is used unsafely for extraction, enabling path traversal/out-of-directory overwrite during DLL extraction.</violation>
<violation number="2" location="app/src/main/java/app/gamenative/utils/SteamUtils.kt:1450">
P1: Extraction result is ignored, so DLL extraction may fail while success is still logged.</violation>
</file>
<file name="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt:1628">
P2: Optional Steam XAudio extraction is in the fatal setup path; extraction errors can abort game launch.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
Outdated
Show resolved
Hide resolved
add other gamesource detection
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt (2)
122-125: Empty callback stubs are valid interface requirements.Static analysis flags these as empty blocks, but they're required by
IArchiveExtractCallback. You can silence the lint by adding explicitUnitreturns or a brief comment.💡 Optional: Silence lint warnings
- override fun prepareOperation(extractAskMode: ExtractAskMode) {} - override fun setOperationResult(extractOperationResult: ExtractOperationResult) {} - override fun setCompleted(completeValue: Long) {} - override fun setTotal(total: Long) {} + override fun prepareOperation(extractAskMode: ExtractAskMode) = Unit + override fun setOperationResult(extractOperationResult: ExtractOperationResult) = Unit + override fun setCompleted(completeValue: Long) = Unit + override fun setTotal(total: Long) = Unit🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt` around lines 122 - 125, The empty implementations of IArchiveExtractCallback methods (prepareOperation, setOperationResult, setCompleted, setTotal) trigger lint for empty blocks; update each override to explicitly return Unit or include a brief comment explaining the no-op (e.g., "// no-op required by IArchiveExtractCallback") so static analysis recognizes they are intentional, keeping the method signatures intact.
53-59: Use.firstOrNull()instead of.mapfor side effects.
.mapis intended for transformations, not side effects. Additionally, if multipleDXSETUP.exefiles exist, the result depends on traversal order. Use.firstOrNull()for clarity and deterministic behavior.♻️ Proposed fix
if (!directXDir.exists()) { - FileUtils.findFilesRecursive( + val dxSetupFile = FileUtils.findFilesRecursive( rootPath = appDir.toPath(), pattern = "DXSETUP.exe", maxDepth = 5, - ).map { file -> - directXDir = file.parent.toFile() + ).firstOrNull() + if (dxSetupFile != null) { + directXDir = dxSetupFile.parent.toFile() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt` around lines 53 - 59, The code uses FileUtils.findFilesRecursive(...).map { file -> directXDir = file.parent.toFile() } which misuses map for side effects and may pick a non-deterministic match; change this to use firstOrNull() to pick the first matching file deterministically and assign its parent to directXDir (e.g., val file = FileUtils.findFilesRecursive(...).firstOrNull(); if (file != null) directXDir = file.parent.toFile()). Update the occurrence in XServerScreenUtils.kt where directXDir is set so it no longer maps over the sequence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt`:
- Line 38: The Epic branch currently calls
EpicService.getInstallPath(appId.toInt()) which can throw NumberFormatException
for non-numeric appId; update the GameSource.EPIC handling in XServerScreenUtils
(where EpicService.getInstallPath is invoked) to parse using toIntOrNull(),
check for null, and handle it early (return a safe default, log/error out, or
skip this branch) before calling EpicService.getInstallPath so only valid
integers are passed.
- Around line 91-97: The method extractDllsFromCab currently calls
SevenZip.initSevenZipFromPlatformJAR() per file and risks leaking raf and
inStream if SevenZip.openInArchive(...) throws; move the
SevenZip.initSevenZipFromPlatformJAR() call out of extractDllsFromCab
(initialize once at app startup) and refactor extractDllsFromCab to close
RandomAccessFile and RandomAccessFileInStream reliably (use try-with-resources /
Kotlin use{} or finally) around raf/inStream and any archive handling so
resources are always released even when SevenZip.openInArchive throws.
- Around line 116-119: The lambda returning ISequentialOutStream currently opens
a new FileOutputStream per data chunk (in XServerScreenUtils.kt), which is
inefficient and unsafe; instead, open a single FileOutputStream (preferably
wrapped in a BufferedOutputStream) once before calling the code that provides
the ISequentialOutStream, capture/close that stream after extraction completes
(use try/finally or try-with-resources), and have the ISequentialOutStream
implementation write to that single stream and return data.size; update the
enclosing function that creates the ISequentialOutStream to be responsible for
stream lifecycle (ensure parent dirs exist and use append mode only if
intended).
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt`:
- Around line 122-125: The empty implementations of IArchiveExtractCallback
methods (prepareOperation, setOperationResult, setCompleted, setTotal) trigger
lint for empty blocks; update each override to explicitly return Unit or include
a brief comment explaining the no-op (e.g., "// no-op required by
IArchiveExtractCallback") so static analysis recognizes they are intentional,
keeping the method signatures intact.
- Around line 53-59: The code uses FileUtils.findFilesRecursive(...).map { file
-> directXDir = file.parent.toFile() } which misuses map for side effects and
may pick a non-deterministic match; change this to use firstOrNull() to pick the
first matching file deterministically and assign its parent to directXDir (e.g.,
val file = FileUtils.findFilesRecursive(...).firstOrNull(); if (file != null)
directXDir = file.parent.toFile()). Update the occurrence in
XServerScreenUtils.kt where directXDir is set so it no longer maps over the
sequence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 411c6245-e34f-4e7c-911d-3de71c7a064d
📒 Files selected for processing (3)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.ktapp/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.ktapp/src/main/java/app/gamenative/utils/SteamUtils.kt
✅ Files skipped from review due to trivial changes (1)
- app/src/main/java/app/gamenative/utils/SteamUtils.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreen.kt
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt
Show resolved
Hide resolved
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt:117">
P1: Opening a new `FileOutputStream` for every data chunk callback is very inefficient and risks corrupted writes. Each invocation opens the file in append mode, writes, and closes — this can cause performance problems and buffering issues for multi-chunk files. Open the stream once before `archive.extract()` and close it in `setOperationResult()`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt (1)
91-97:⚠️ Potential issue | 🟠 MajorSevenZip initialization and stream lifecycle are still unsafe in this method.
On Line 93, SevenZip is initialized for every CAB. Also, on Lines 95-97, if
openInArchive(...)throws,raf/inStreamcan leak because they’re created before thetryblock.♻️ Proposed fix
+ `@Volatile` private var sevenZipInitialized = false + + private fun ensureSevenZipInitialized() { + if (!sevenZipInitialized) { + synchronized(this) { + if (!sevenZipInitialized) { + SevenZip.initSevenZipFromPlatformJAR() + sevenZipInitialized = true + } + } + } + } + private fun extractDllsFromCab(cabFile: File, targetDir: File) { - // Do this once at app startup or before the loop - SevenZip.initSevenZipFromPlatformJAR() - - val raf = RandomAccessFile(cabFile, "r") - val inStream = RandomAccessFileInStream(raf) - val archive: IInArchive = SevenZip.openInArchive(null, inStream) - - try { + ensureSevenZipInitialized() + RandomAccessFile(cabFile, "r").use { raf -> + val inStream = RandomAccessFileInStream(raf) + val archive = SevenZip.openInArchive(null, inStream) + try { val numberOfItems = archive.numberOfItems for (i in 0 until numberOfItems) { // existing extraction logic } - } finally { - archive.close() - inStream.close() - raf.close() + } finally { + archive.close() + inStream.close() + } } }For SevenZip-JBinding, should `SevenZip.initSevenZipFromPlatformJAR()` be called once per process, and what is the recommended resource-closing pattern around `openInArchive`?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt` around lines 91 - 97, The method extractDllsFromCab currently calls SevenZip.initSevenZipFromPlatformJAR() for each CAB and constructs RandomAccessFile/RandomAccessFileInStream before opening the archive, risking repeated init and resource leaks if openInArchive throws; move SevenZip.initSevenZipFromPlatformJAR() out of extractDllsFromCab so it runs once per process (e.g., in a companion object or application startup), and ensure RandomAccessFile, RandomAccessFileInStream and the resulting IInArchive from SevenZip.openInArchive are properly closed using Kotlin's use{} or try/finally so resources are released even on exceptions (reference extractDllsFromCab, SevenZip.initSevenZipFromPlatformJAR, RandomAccessFile, RandomAccessFileInStream, SevenZip.openInArchive, IInArchive).
🧹 Nitpick comments (2)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt (2)
53-59: UsefirstOrNull/forEachinstead ofmapfor side effects.On Lines 57-59,
mapis being used only to mutatedirectXDir. This is harder to read and does unnecessary list work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt` around lines 53 - 59, The code uses FileUtils.findFilesRecursive(...).map { file -> directXDir = file.parent.toFile() } purely for side effects; replace the map with either firstOrNull() to pick the first matching file and assign directXDir = it.parent.toFile() if non-null, or use forEach { file -> directXDir = file.parent.toFile() } if you intend to set directXDir from every match. Update the call sites around FileUtils.findFilesRecursive, preserving rootPath/pattern/maxDepth, and ensure directXDir is only mutated once when using firstOrNull to avoid unnecessary list allocation.
63-66: UseimageFs.wineprefixproperty instead of concatenating withImageFs.WINEPREFIXconstant for clarity.On line 65, while
File(rootDir, ImageFs.WINEPREFIX + "/drive_c/windows")does technically resolve to the correct path due to Java's File concatenation behavior with absolute children, this pattern is unintuitive and fragile. Use thewineprefixproperty directly:Cleaner approach
- val imageFs = ImageFs.find(context) - val rootDir = imageFs.rootDir - val windowsDir = File(rootDir, ImageFs.WINEPREFIX + "/drive_c/windows") + val imageFs = ImageFs.find(context) + val windowsDir = File(imageFs.wineprefix, "drive_c/windows")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt` around lines 63 - 66, The current code builds the Windows path by concatenating ImageFs.WINEPREFIX with "/drive_c/windows" which is fragile; instead use the instance property on the ImageFs returned by ImageFs.find(context). Update the construction of windowsDir to reference imageFs.wineprefix (and imageFs.rootDir) rather than ImageFs.WINEPREFIX concatenation so the path is formed from the instance's wineprefix property (keep using ImageFs.find(context) and the windowsDir variable).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt`:
- Around line 102-106: The CAB entry path from archive.getProperty(...,
PropID.PATH) is used directly to build File(targetDir, name) which allows path
traversal; sanitize and validate the entry before writing: obtain the raw name,
reject empty or absolute paths, normalize/remove any .. segments (e.g. via
File(name).normalize()/canonical resolution), resolve the resulting path against
targetDir and verify the resolved/canonical path starts with
targetDir.canonicalPath, and only then create parents and write the file (apply
this logic where archive.getProperty, PropID.PATH, targetDir, and outFile are
used in XServerScreenUtils.kt).
---
Duplicate comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt`:
- Around line 91-97: The method extractDllsFromCab currently calls
SevenZip.initSevenZipFromPlatformJAR() for each CAB and constructs
RandomAccessFile/RandomAccessFileInStream before opening the archive, risking
repeated init and resource leaks if openInArchive throws; move
SevenZip.initSevenZipFromPlatformJAR() out of extractDllsFromCab so it runs once
per process (e.g., in a companion object or application startup), and ensure
RandomAccessFile, RandomAccessFileInStream and the resulting IInArchive from
SevenZip.openInArchive are properly closed using Kotlin's use{} or try/finally
so resources are released even on exceptions (reference extractDllsFromCab,
SevenZip.initSevenZipFromPlatformJAR, RandomAccessFile,
RandomAccessFileInStream, SevenZip.openInArchive, IInArchive).
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt`:
- Around line 53-59: The code uses FileUtils.findFilesRecursive(...).map { file
-> directXDir = file.parent.toFile() } purely for side effects; replace the map
with either firstOrNull() to pick the first matching file and assign directXDir
= it.parent.toFile() if non-null, or use forEach { file -> directXDir =
file.parent.toFile() } if you intend to set directXDir from every match. Update
the call sites around FileUtils.findFilesRecursive, preserving
rootPath/pattern/maxDepth, and ensure directXDir is only mutated once when using
firstOrNull to avoid unnecessary list allocation.
- Around line 63-66: The current code builds the Windows path by concatenating
ImageFs.WINEPREFIX with "/drive_c/windows" which is fragile; instead use the
instance property on the ImageFs returned by ImageFs.find(context). Update the
construction of windowsDir to reference imageFs.wineprefix (and imageFs.rootDir)
rather than ImageFs.WINEPREFIX concatenation so the path is formed from the
instance's wineprefix property (keep using ImageFs.find(context) and the
windowsDir variable).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a13b918-e423-4b3a-aee8-d0577685ddcf
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt (1)
98-143:⚠️ Potential issue | 🔴 CriticalHarden CAB extraction: sanitize paths + fix stream/resource lifecycle.
PropID.PATHis used directly for output path construction, and stream/archive setup occurs before protected lifecycle handling. This allows path traversal risk and can leak resources on early exceptions; per-chunkFileOutputStreamcreation is also expensive and brittle.🛡️ Proposed fix
+ `@Volatile` + private var sevenZipInitialized = false + + private val sevenZipInitLock = Any() + + private fun ensureSevenZipInitialized() { + if (!sevenZipInitialized) { + synchronized(sevenZipInitLock) { + if (!sevenZipInitialized) { + SevenZip.initSevenZipFromPlatformJAR() + sevenZipInitialized = true + } + } + } + } + private fun extractDllsFromCab(cabFile: File, targetDir: File) { - // Do this once at app startup or before the loop - SevenZip.initSevenZipFromPlatformJAR() - - val raf = RandomAccessFile(cabFile, "r") - val inStream = RandomAccessFileInStream(raf) - val archive: IInArchive = SevenZip.openInArchive(null, inStream) - - try { + ensureSevenZipInitialized() + targetDir.mkdirs() + + RandomAccessFile(cabFile, "r").use { raf -> + val inStream = RandomAccessFileInStream(raf) + try { + val archive: IInArchive = SevenZip.openInArchive(null, inStream) + try { val numberOfItems = archive.numberOfItems for (i in 0 until numberOfItems) { - val name = archive.getProperty(i, net.sf.sevenzipjbinding.PropID.PATH) as String + val rawName = archive.getProperty(i, net.sf.sevenzipjbinding.PropID.PATH) as? String ?: continue - if (name.endsWith(".dll", ignoreCase = true)) { - val outFile = File(targetDir, name.lowercase()) + if (rawName.endsWith(".dll", ignoreCase = true)) { + val safeName = File(rawName).name.lowercase() + if (safeName.isBlank()) continue + val outFile = File(targetDir, safeName) + val canonicalTarget = targetDir.canonicalFile + val canonicalOut = outFile.canonicalFile + if (!canonicalOut.path.startsWith(canonicalTarget.path + File.separator)) { + Timber.w("Skipping suspicious CAB entry path: %s", rawName) + continue + } if (outFile.exists()) { outFile.delete() } - archive.extract( - intArrayOf(i), false, - object : IArchiveExtractCallback { - override fun getStream(index: Int, extractAskMode: ExtractAskMode): ISequentialOutStream? { - if (extractAskMode != ExtractAskMode.EXTRACT) return null - - return ISequentialOutStream { data -> - FileOutputStream(outFile, true).use { it.write(data) } - data.size // Return the number of bytes written - } - } - - override fun prepareOperation(extractAskMode: ExtractAskMode) {} - override fun setOperationResult(extractOperationResult: ExtractOperationResult) {} - override fun setCompleted(completeValue: Long) {} - override fun setTotal(total: Long) {} - } - ) + FileOutputStream(outFile, false).use { fos -> + var opResult: ExtractOperationResult? = null + archive.extract( + intArrayOf(i), false, + object : IArchiveExtractCallback { + override fun getStream(index: Int, extractAskMode: ExtractAskMode): ISequentialOutStream? { + if (extractAskMode != ExtractAskMode.EXTRACT) return null + return ISequentialOutStream { data -> + fos.write(data) + data.size + } + } + override fun prepareOperation(extractAskMode: ExtractAskMode) {} + override fun setOperationResult(extractOperationResult: ExtractOperationResult) { + opResult = extractOperationResult + } + override fun setCompleted(completeValue: Long) {} + override fun setTotal(total: Long) {} + } + ) + if (opResult != ExtractOperationResult.OK) { + outFile.delete() + Timber.w("Extraction failed for %s with result=%s", safeName, opResult) + continue + } + } Timber.tag("replaceXAudioDllsFromRedistributable").d("Extracted: ${outFile.name}") } } - } finally { - archive.close() - inStream.close() - raf.close() + } finally { + archive.close() + } + } finally { + inStream.close() + } } }#!/bin/bash # Read-only verification: confirm whether extraction still uses unsanitized CAB paths # and per-chunk stream allocation patterns. rg -n -C3 'PropID\.PATH|FileOutputStream\(outFile, true\)|SevenZip\.openInArchive|setOperationResult\(' app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt` around lines 98 - 143, The extractDllsFromCab function currently trusts archive.getProperty(i, PropID.PATH) and opens per-chunk FileOutputStream in ISequentialOutStream, risking path traversal and resource leaks; fix by canonicalizing and sanitizing the entry path (reject paths with ".." or absolute roots and normalize via File(targetDir, entryName).canonicalPath startsWith targetDir.canonicalPath), create parent directories before extraction, open a single FileOutputStream for each outFile (not inside the per-chunk lambda) and return an ISequentialOutStream that writes to that preopened stream, and wrap RandomAccessFile, RandomAccessFileInStream and archive in Kotlin use{} (or try-with-resources) so they are closed on exceptions; keep checks like name.endsWith(".dll", ignoreCase=true) but apply them after safe normalization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt`:
- Around line 46-53: The catch block in XServerScreenUtils that currently
swallows Exception and yields null should log the failure with context; change
the catch to capture the exception (e.g., catch (e: Exception)) and call the
existing logger (or add one) to emit a warning that includes the appId and
source along with the exception, and likewise when appDirPath == null log a
warning before returning; reference the appDirPath variable and the appId/source
context so the log shows which app failed to resolve.
---
Duplicate comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt`:
- Around line 98-143: The extractDllsFromCab function currently trusts
archive.getProperty(i, PropID.PATH) and opens per-chunk FileOutputStream in
ISequentialOutStream, risking path traversal and resource leaks; fix by
canonicalizing and sanitizing the entry path (reject paths with ".." or absolute
roots and normalize via File(targetDir, entryName).canonicalPath startsWith
targetDir.canonicalPath), create parent directories before extraction, open a
single FileOutputStream for each outFile (not inside the per-chunk lambda) and
return an ISequentialOutStream that writes to that preopened stream, and wrap
RandomAccessFile, RandomAccessFileInStream and archive in Kotlin use{} (or
try-with-resources) so they are closed on exceptions; keep checks like
name.endsWith(".dll", ignoreCase=true) but apply them after safe normalization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e468b378-f133-4cf3-b9fc-ecc48599b03b
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt (1)
60-66: UsefirstOrNull()instead ofmapfor single-result lookup.The
.map {}iterates all matches and overwritesdirectXDirfor each one. If multipleDXSETUP.exefiles exist, the last match wins non-deterministically. Use.firstOrNull()to explicitly select the first match.♻️ Proposed fix
- FileUtils.findFilesRecursive( - rootPath = appDir.toPath(), - pattern = "DXSETUP.exe", - maxDepth = 5, - ).map { file -> - directXDir = file.parent.toFile() - } + FileUtils.findFilesRecursive( + rootPath = appDir.toPath(), + pattern = "DXSETUP.exe", + maxDepth = 5, + ).firstOrNull()?.let { file -> + directXDir = file.parent.toFile() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt` around lines 60 - 66, The code currently uses FileUtils.findFilesRecursive(...).map { file -> directXDir = file.parent.toFile() } which iterates all matches and overwrites directXDir; change this to take the first match only by calling firstOrNull() on the result, then set directXDir = firstMatch?.parent?.toFile() only when non-null (or handle null case), using the same FileUtils.findFilesRecursive call with rootPath/appDir and pattern "DXSETUP.exe" to locate the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt`:
- Around line 117-141: The FileOutputStream (fos) is being closed in
setCompleted (a progress callback) which can run multiple times and risks
truncation or leaks; move cleanup to setOperationResult and ensure fos is closed
in all paths: wrap creation of FileOutputStream and the call to
archive.extract(...) in a try/finally (or try/catch/finally) so fos is closed if
archive.extract throws, and remove fos.close() from setCompleted; implement
fos.close() inside setOperationResult(ExtractOperationResult) in the
IArchiveExtractCallback implementation and null-check the stream before closing
to avoid double-close errors (refer to FileOutputStream, archive.extract(...),
getStream, setCompleted, and setOperationResult).
---
Nitpick comments:
In `@app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt`:
- Around line 60-66: The code currently uses
FileUtils.findFilesRecursive(...).map { file -> directXDir =
file.parent.toFile() } which iterates all matches and overwrites directXDir;
change this to take the first match only by calling firstOrNull() on the result,
then set directXDir = firstMatch?.parent?.toFile() only when non-null (or handle
null case), using the same FileUtils.findFilesRecursive call with
rootPath/appDir and pattern "DXSETUP.exe" to locate the file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a8886d91-9d7c-480b-bfd1-08d6beac0a41
📒 Files selected for processing (1)
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt
Outdated
Show resolved
Hide resolved
joshuatam
changed the title
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt">
<violation number="1" location="app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt:68">
P2: Fallback DirectX directory discovery uses `Stream.map` without a terminal operation, so the lambda (and `directXDir` assignment) never executes; `return@map` also does not short-circuit iteration.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
app/src/main/java/app/gamenative/ui/screen/xserver/XServerScreenUtils.kt
Outdated
Show resolved
Hide resolved
Contributor
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
Contributor
Author
|
@cubic-dev-ai review |
Contributor
@joshuatam I have started the AI code review. It will take a few minutes to complete. |
| onExtractFileListener, | ||
| ) | ||
|
|
||
| if (!container.wineVersion.lowercase().contains("proton-9.0")) { |
Owner
There was a problem hiding this comment.
maybe we should check for proton-10 instead of checking for !proton-9.0?
Contributor
Author
There was a problem hiding this comment.
Yes it is clearer, updated
Owner
There was a problem hiding this comment.
should we add this as a preinstallstep like we do with the other common redists?
Contributor
Author
There was a problem hiding this comment.
I think the dlls have to be always replaced just before game start as changing proton will override the xaudio, which the preinstallstep may only run once.
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
2 participants
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.
Description
Adds support for decompressing and installing DirectX audio components (XAudio, XACT, X3DAudio) from game cabinet files into the Wine prefix. This uses 7-Zip bindings to extract the necessary DLLs to system folders, helping resolve audio compatibility issues in games that rely on specific redistributable versions.
Note
This extraction can only be applied to proton 10, if using on proton 9 will make the game crash immediately.
Games Tested that fixed Audio Issue
Recording
https://discord.com/channels/1378308569287622737/1491981133628440737/1492575279691075614
Checklist
#code-changes, I have discussed this change there and it has been green-lighted. If I do not have access, I have still provided clear context in this PR. If I skip both, I accept that this change may face delays in review, may not be reviewed at all, or may be closed.CONTRIBUTING.md.Summary by cubic
Extracts XAudio, XACT, and X3DAudio DLLs from DirectX CABs and installs them into the Wine prefix to fix game audio issues. Enabled only on Proton 10; gated to avoid Proton 9.0 crashes.
New Features
XServerScreenUtils.replaceXAudioDllsFromRedistributable(...)usingsevenzipjbinding; invoked fromXServerScreen._CommonRedist/DirectXorDXSETUP.exe, and extracts.dllfrom.cabto Winesystem32(x64) orsyswow64(x86).sevenzipjbindingandjitpack.io.Bug Fixes
container.wineVersioncontaining "proton-10"; skip others to prevent crashes.DXSETUP.exeand using its parent when_CommonRedist/DirectXis missing.SevenZipbefore processing and useFileOutputStreamfor stable extraction.Written for commit e482b57. Summary will update on new commits.
Summary by CodeRabbit
New Features
Chores