Return empty defaults when ImageFs metadata files exist but are empty#1565
Open
Smuger wants to merge 1 commit into
Open
Return empty defaults when ImageFs metadata files exist but are empty#1565Smuger wants to merge 1 commit into
Smuger wants to merge 1 commit into
Conversation
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 FileUtils.readFirstLine(File), refactors ImageFs and ContainerMigrator to use it with explicit null/blank handling, and adds unit tests for FileUtils plus expanded tests for ImageFs getters. ChangesFile Reading Utility Refactoring
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Smuger
force-pushed
the
fix/imagefs-empty-variant-crash
branch
from
95d09b5 to
0d8c4d3
Compare
![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: 2
🧹 Nitpick comments (1)
app/src/test/java/com/winlator/xenvironment/ImageFsTest.kt (1)
61-122: ⚡ Quick winAdd edge-case tests for whitespace and malformed version content.
Coverage is strong for missing/empty/populated files, but it should also assert behavior for:
- whitespace-only
.variant/.arch- non-numeric
.img_version(expected fallback0)That locks in the crash fix and defaulting contract against corrupted metadata formats.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/test/java/com/winlator/xenvironment/ImageFsTest.kt` around lines 61 - 122, Add tests to cover whitespace and malformed version edge cases: create tests that write whitespace-only content into the .variant and .arch files (using configDir() and File(...).writeText(" ") or via imageFs() helpers) and assert imageFs().variant and imageFs().arch return empty strings; also create a test that writes a non-numeric value into .img_version (e.g., "abc") and assert imageFs().version returns 0. Use the existing patterns in the file (imageFs(), configDir(), createVariantFile/createArchFile/createImgVersionFile where appropriate) and mirror the structure/naming of the other tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/com/winlator/xenvironment/ImageFs.java`:
- Line 96: getVariant() and getArch() currently return whitespace-only strings
as real values; change their normalization so that if the read line is null or
blank (use String.isBlank()) it is treated as missing and returns the empty
default. In both getVariant() and getArch(), replace the current null-only check
(return line == null ? "" : line) with a check that returns "" when line == null
|| line.isBlank(), otherwise return the original line (or trimmed value if
desired).
- Line 73: The current return in ImageFs that does "return line == null ||
line.isBlank() ? 0 : Integer.parseInt(line.trim());" can throw
NumberFormatException for corrupted/non-numeric .img_version content; wrap the
parsing in a safe parse path (e.g., try-catch around Integer.parseInt or use a
safe parse helper) so that any NumberFormatException results in returning 0
instead of crashing; update the method in ImageFs where this line appears to
trim the line, attempt Integer.parseInt inside a catch(NumberFormatException)
and return 0 on failure.
---
Nitpick comments:
In `@app/src/test/java/com/winlator/xenvironment/ImageFsTest.kt`:
- Around line 61-122: Add tests to cover whitespace and malformed version edge
cases: create tests that write whitespace-only content into the .variant and
.arch files (using configDir() and File(...).writeText(" ") or via imageFs()
helpers) and assert imageFs().variant and imageFs().arch return empty strings;
also create a test that writes a non-numeric value into .img_version (e.g.,
"abc") and assert imageFs().version returns 0. Use the existing patterns in the
file (imageFs(), configDir(),
createVariantFile/createArchFile/createImgVersionFile where appropriate) and
mirror the structure/naming of the other tests.
🪄 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: 04539604-0676-406b-ac2c-272774f5f542
📒 Files selected for processing (5)
app/src/main/java/app/gamenative/utils/ContainerMigrator.ktapp/src/main/java/com/winlator/core/FileUtils.javaapp/src/main/java/com/winlator/xenvironment/ImageFs.javaapp/src/test/java/com/winlator/core/FileUtilsTest.ktapp/src/test/java/com/winlator/xenvironment/ImageFsTest.kt
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/com/winlator/core/FileUtils.java`:
- Around line 259-260: The readFirstLine(File file) helper should early-return
null when its input is null to avoid a NullPointerException from new
FileInputStream(file); add a null check at the top of readFirstLine (and the
analogous code region around lines 263-266) so that if file == null the method
returns null immediately, preserving the safe-default behavior for unreadable
input and preventing the FileInputStream constructor from being invoked with a
null reference.
🪄 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: 6b906810-2673-43f9-9c41-1ab71aa855ca
📒 Files selected for processing (2)
app/src/main/java/com/winlator/core/FileUtils.javaapp/src/main/java/com/winlator/xenvironment/ImageFs.java
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/winlator/xenvironment/ImageFs.java
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Smuger
force-pushed
the
fix/imagefs-empty-variant-crash
branch
from
e058998 to
0a17b48
Compare
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/test/java/com/winlator/core/FileUtilsTest.kt (1)
15-44: ⚡ Quick winAdd a test for the explicit null-input contract.
FileUtils.readFirstLine(File)now guaranteesnullwhenfileisnull, but this branch is not covered yet (Lines 15-44). Please add one test to lock that behavior.Suggested test addition
class FileUtilsTest { @@ `@Test` fun `readFirstLine returns null when file is empty`() { val file = tmp.newFile("empty.txt") assertNull(FileUtils.readFirstLine(file)) } + + `@Test` + fun `readFirstLine returns null when file is null`() { + assertNull(FileUtils.readFirstLine(null)) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/test/java/com/winlator/core/FileUtilsTest.kt` around lines 15 - 44, Add a unit test in FileUtilsTest.kt that asserts FileUtils.readFirstLine(null) returns null to cover the explicit null-input contract; create a new `@Test` function (e.g., `readFirstLine_returns_null_when_input_is_null`) that calls FileUtils.readFirstLine with a null File reference and uses assertNull to verify the result, matching the style and naming of the existing tests and using the same test class and imports.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/com/winlator/xenvironment/ImageFs.java`:
- Around line 240-241: Several ImageFs methods (e.g., getInstalledWineDir)
construct File with an absolute child ("/...") which causes new File(rootDir,
child) to ignore rootDir; change each to use a relative child path (no leading
'/') or build paths via rootDir.toPath().resolve(...) so the result stays inside
the image root. Update all occurrences that call new File(rootDir, "/...")
(including getInstalledWineDir and the other similarly-implemented getters) to
remove the leading slash or use Path.resolve to compose children relative to
rootDir.
---
Nitpick comments:
In `@app/src/test/java/com/winlator/core/FileUtilsTest.kt`:
- Around line 15-44: Add a unit test in FileUtilsTest.kt that asserts
FileUtils.readFirstLine(null) returns null to cover the explicit null-input
contract; create a new `@Test` function (e.g.,
`readFirstLine_returns_null_when_input_is_null`) that calls
FileUtils.readFirstLine with a null File reference and uses assertNull to verify
the result, matching the style and naming of the existing tests and using the
same test class and imports.
🪄 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: 9e7f2922-4081-493d-984c-2c4dcea30951
📒 Files selected for processing (5)
app/src/main/java/app/gamenative/utils/ContainerMigrator.ktapp/src/main/java/com/winlator/core/FileUtils.javaapp/src/main/java/com/winlator/xenvironment/ImageFs.javaapp/src/test/java/com/winlator/core/FileUtilsTest.ktapp/src/test/java/com/winlator/xenvironment/ImageFsTest.kt
Comment on lines
240
to
241
| public File getInstalledWineDir() { | ||
| return new File(rootDir, "/opt/installed-wine"); |
Contributor
There was a problem hiding this comment.
Use relative child paths when composing imagefs subdirectories.
Line 241 (and the other listed methods) passes an absolute child path ("/...") to new File(rootDir, child), which ignores rootDir. These methods can resolve outside the imagefs root and violate their own “inside the image” contract.
Proposed fix
- public File getInstalledWineDir() {
- return new File(rootDir, "/opt/installed-wine");
- }
+ public File getInstalledWineDir() {
+ return new File(rootDir, "opt/installed-wine");
+ }
- public File getTmpDir() {
- return new File(rootDir, "/tmp");
- }
+ public File getTmpDir() {
+ return new File(rootDir, "tmp");
+ }
- public File getLibDir() { return new File(rootDir, "/usr/lib"); }
+ public File getLibDir() { return new File(rootDir, "usr/lib"); }
- public File getBinDir() { return new File(rootDir, "/usr/bin"); }
+ public File getBinDir() { return new File(rootDir, "usr/bin"); }
- public File getShareDir() {
- return new File(rootDir, "/usr/share");
- }
+ public File getShareDir() {
+ return new File(rootDir, "usr/share");
+ }
- public File getGlibc32Dir() {
- return new File(rootDir, "/usr/lib/arm-linux-gnueabihf");
- }
+ public File getGlibc32Dir() {
+ return new File(rootDir, "usr/lib/arm-linux-gnueabihf");
+ }
- public File getGlibc64Dir() {
- return new File(rootDir, "/usr/lib");
- }
+ public File getGlibc64Dir() {
+ return new File(rootDir, "usr/lib");
+ }
- public File getLib32Dir() {
- return new File(rootDir, "/usr/lib/arm-linux-gnueabihf");
- }
+ public File getLib32Dir() {
+ return new File(rootDir, "usr/lib/arm-linux-gnueabihf");
+ }
- public File getLib64Dir() {
- return new File(rootDir, "/usr/lib");
- }
+ public File getLib64Dir() {
+ return new File(rootDir, "usr/lib");
+ }
- public File getStorageDir() {
- return new File(rootDir, "/storage");
- }
+ public File getStorageDir() {
+ return new File(rootDir, "storage");
+ }Also applies to: 248-249, 256-257, 262-263, 268-269, 276-277, 284-285, 292-293, 300-301, 308-309
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/com/winlator/xenvironment/ImageFs.java` around lines 240 -
241, Several ImageFs methods (e.g., getInstalledWineDir) construct File with an
absolute child ("/...") which causes new File(rootDir, child) to ignore rootDir;
change each to use a relative child path (no leading '/') or build paths via
rootDir.toPath().resolve(...) so the result stays inside the image root. Update
all occurrences that call new File(rootDir, "/...") (including
getInstalledWineDir and the other similarly-implemented getters) to remove the
leading slash or use Path.resolve to compose children relative to rootDir.
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.
Description
ImageFs.getVariant(),getArch(), andgetVersion()called.get(0)on the result ofreadLines()after only checking file existence. A zero-byte.variantfile (caused by interrupted install) crashed the app on every game launch withIndexOutOfBoundsException. Empty files now return the same defaults as missing files, triggering a clean re-install of the base image.Recording
N/A — crash fix, no visual change.
Type of Change
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
Return safe defaults when
ImageFsmetadata files are empty, whitespace-only, or invalid to prevent crashes and trigger a clean re-install. Also hardens container migration version parsing, adds docs/tests, and makesFileUtils.readFirstLine()null-safe.Bug Fixes
getVariant(),getArch(), andgetVersion()now usereadFirstLine(), trim values, treat empty/whitespace as missing, and return0on non-numeric version; avoids crashes and bad parses.ContainerMigratorusesreadFirstLine(), trims, and defaults to0for empty/missing.container_migration_version; added tests forreadFirstLine()andImageFscovering missing, empty, whitespace-only, multi-line, and non-numeric cases.Refactors
ImageFsandFileUtilsto document behavior and shared directory helpers.Written for commit 0a17b48. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests