fix(android): resolve black screen, modernize build with vcpkg and LuaJIT#1673
Open
Kizuno18 wants to merge 1 commit intoopentibiabr:mainfrom
Open
fix(android): resolve black screen, modernize build with vcpkg and LuaJIT#1673Kizuno18 wants to merge 1 commit intoopentibiabr:mainfrom
Kizuno18 wants to merge 1 commit intoopentibiabr:mainfrom
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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (23)
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 end-to-end Android build support: new Docker Android builder, SDK/NDK/CMake/vcpkg provisioning, LuaJIT cross-compilation and helper scripts, Gradle/CMake/manifest adjustments, a vendored minizip unzip stack, Android runtime fixes, and build documentation. Changes
Sequence DiagramsequenceDiagram
actor Developer
participant DockerCLI as "Docker CLI"
participant Builder as "Builder Stage"
participant SDKNDK as "Android SDK/NDK/CMake"
participant VCPKG as "vcpkg"
participant Gradle as "Gradle"
participant APK as "APK Artifact"
Developer->>DockerCLI: docker build -f Dockerfile.android
DockerCLI->>Builder: create builder container
Builder->>SDKNDK: sdkmanager install SDK/NDK/CMake
Builder->>VCPKG: clone & bootstrap vcpkg
Builder->>VCPKG: vcpkg install triplets
Builder->>SDKNDK: cross-compile LuaJIT per ABI (build_luajit_android.sh)
Builder->>Builder: assemble assets -> android/app/src/main/assets/data.zip
Builder->>Gradle: ./gradlew assembleRelease
Gradle->>SDKNDK: externalNativeBuild (CMake uses vcpkg & prebuilt LuaJIT)
Gradle->>APK: package and sign release APK
APK->>Developer: artifact available from builder stage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
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 |
7 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/framework/platform/androidmanager.cpp (1)
113-120:⚠️ Potential issue | 🔴 CriticalUndefined behavior:
delete[]used onmalloc-allocated memory.Line 113 allocates with
malloc()but line 120 deallocates withdelete[]. Mixing C and C++ allocation/deallocation functions is undefined behavior and can cause crashes or memory corruption.🐛 Proposed fix
auto dataFileLength = AAsset_getLength(dataAsset); char* dataContent = (char *) malloc(dataFileLength + 1); + if (!dataContent) { + AAsset_close(dataAsset); + g_logger.fatal("Failed to allocate memory for data.zip extraction"); + return; + } AAsset_read(dataAsset, dataContent, dataFileLength); dataContent[dataFileLength] = '\0'; unzipper::extract(dataContent, dataFileLength, destFolder); AAsset_close(dataAsset); - delete [] dataContent; + free(dataContent); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/platform/androidmanager.cpp` around lines 113 - 120, The code allocates dataContent with malloc but deallocates it with delete[], causing undefined behavior; replace the deallocation with free(dataContent) (or alternatively allocate with new char[...] and keep delete[]) and ensure <cstdlib> is included if free isn't already available; verify the buffer passed to unzipper::extract still uses dataContent and AAsset_close remains after free.src/framework/core/unzipper.cpp (2)
47-48:⚠️ Potential issue | 🟠 MajorMemory leak:
unzmem.baseis never freed.The buffer allocated with
mallocat line 47 is never freed afterunzClose(). Add cleanup before returning.🛠️ Proposed fix
unzClose(zipfile); + free(unzmem.base); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/core/unzipper.cpp` around lines 47 - 48, The malloc'd buffer assigned to unzmem.base is never freed; ensure you free(unzmem.base) after you're done with the unzipper (e.g., immediately after calling unzClose()) and also on all early-return/error paths in the function to avoid leaks—update the code paths around unzmem.base allocation and the unzClose() call so that unzmem.base is freed exactly once when the in-memory unz structure is no longer needed.
120-120:⚠️ Potential issue | 🔴 CriticalInvalid error message construction will cause undefined behavior.
The expression
&"error: " [ error]performs pointer arithmetic on a string literal using the error code as an index, which produces garbage output or crashes. This should format the error code properly.🐛 Proposed fix
- g_logger.fatal( &"error: " [ error] ); + g_logger.fatal(stdext::format("unzip read error: %d", error));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/core/unzipper.cpp` at line 120, The call to g_logger.fatal currently uses the invalid expression &"error: " [ error], which performs unintended pointer arithmetic; fix by building a proper formatted message that includes the numeric/enum error value and passing that to g_logger.fatal (e.g. use the logger's printf-style overload or construct a std::string via std::to_string/error-to-string conversion or an ostringstream and pass that), replacing the faulty expression where g_logger.fatal(...) is called and using the local variable error as the value to include.src/framework/platform/androidwindow.cpp (1)
270-280:⚠️ Potential issue | 🟡 MinorInconsistent clock sources for long-press detection.
Line 271 records
m_lastPressusingg_clock.millis(), but line 274 compares againststdext::millis(). If these clock sources have different bases or drift, long-press detection could behave unexpectedly. Consider using the same clock source for both.Proposed fix
- if (!m_isDragging && stdext::millis() > m_lastPress + 500) { + if (!m_isDragging && g_clock.millis() > m_lastPress + 500) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/platform/androidwindow.cpp` around lines 270 - 280, The long-press detection mixes clock sources: m_lastPress is set from g_clock.millis() but later compared to stdext::millis(); update the comparison to use the same clock (e.g., replace stdext::millis() with g_clock.millis() or vice versa) so the check in the TOUCH_UP branch (the if that uses m_lastPress + 500) uses the same clock that set m_lastPress; adjust references around m_lastPress, g_clock.millis(), stdext::millis() in the TOUCH_UP handling (including the block that sets mouseButton to Fw::MouseRightButton and the subsequent g_dispatcher.addEvent that modifies m_mouseButtonStates) so timing is consistent.
🧹 Nitpick comments (9)
build_luajit_android.sh (2)
43-43: Quote$(nproc)to prevent word splitting.Shellcheck (SC2046) flags the unquoted command substitution. While unlikely to cause issues with
nproc, quoting is the safe practice.🔧 Proposed fix
- make -j$(nproc) amalg \ + make -j"$(nproc)" amalg \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build_luajit_android.sh` at line 43, The make invocation uses an unquoted command substitution in the parallel flag (make -j$(nproc) ...), which can trigger word-splitting warnings; update the make command to quote the substitution so it reads make -j"$(nproc)" (i.e., replace $(nproc) with "$(nproc)" in the make -j usage) to satisfy ShellCheck SC2046 and avoid potential splitting issues.
24-27:apt-getassumes Debian/Ubuntu and may fail silently on other distributions.The automatic installation of
gcc-multilibonly works on Debian-based systems. Consider adding a more informative error message for non-Debian environments or documenting this requirement.🔧 Proposed improvement
# Verify gcc -m32 works (needed for 32-bit targets) if ! gcc -m32 -x c -c /dev/null -o /dev/null 2>/dev/null; then - echo "Installing gcc-multilib for 32-bit cross-compilation..." - sudo apt-get install -y gcc-multilib g++-multilib + echo "32-bit compilation support (gcc -m32) not available." + if command -v apt-get &>/dev/null; then + echo "Installing gcc-multilib for 32-bit cross-compilation..." + sudo apt-get install -y gcc-multilib g++-multilib + else + echo "ERROR: Please install 32-bit GCC support manually (e.g., gcc-multilib on Debian/Ubuntu)" + exit 1 + fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build_luajit_android.sh` around lines 24 - 27, The check that installs gcc-multilib assumes a Debian-based system; update the if-block that tests "gcc -m32 ..." to first detect the package manager (e.g., check for apt-get) and only attempt "sudo apt-get install -y gcc-multilib g++-multilib" when apt-get is present, otherwise print a clear, actionable error message describing the requirement and supported package managers (or instruct the user to install equivalent packages for their distro) and exit non-zero; reference the installation block that runs "sudo apt-get install -y gcc-multilib g++-multilib" so you modify that branch to perform package-manager detection and user-friendly messaging.android/app/build.gradle.kts (1)
36-43: Misleading signing configuration name.The
"release"signing config uses the debug keystore, which will produce APKs that cannot be uploaded to Play Store. Consider renaming to"debugRelease"or adding a comment clarifying this is for testing only.📝 Proposed clarification
signingConfigs { - create("release") { + // Debug-signed release for testing; replace with production keystore for Play Store + create("release") { storeFile = file(System.getProperty("user.home") + "/.android/debug.keystore")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/build.gradle.kts` around lines 36 - 43, The signingConfigs block defines create("release") but points to the debug keystore, which is misleading; update the config name from "release" to something explicit like "debugRelease" (or add a clear inline comment) and keep the existing values (storeFile = file(System.getProperty("user.home") + "/.android/debug.keystore"), storePassword/keyAlias/keyPassword) so callers don’t mistake this for a real Play Store release signing config; ensure any references to the signing config elsewhere (e.g., in buildTypes or signingConfig usage) are updated to the new name.Dockerfile.android (2)
4-4: Container runs as root user.Static analysis flags that the last USER is root (DS-0002). This is acceptable for a build-only container that outputs artifacts to a scratch stage, but consider documenting this decision or adding a non-root user if the image may be used interactively.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile.android` at line 4, The Dockerfile ends with USER root which triggers a static-analysis warning; either explicitly document that this is a build-only stage and safe to run as root (add a comment above the USER root line explaining artifacts are copied to a scratch/producer stage), or change to a non-root runtime user by adding commands to create and set a less-privileged user (e.g., RUN groupadd -r app && useradd -r -g app app && chown -R app:app /output && then replace USER root with USER app) so the image does not end running as root.
13-14: Pin vcpkg to a specific commit for reproducible builds.The unpinned vcpkg clone may pull incompatible changes. Use the same commit as
builtin-baselineinvcpkg.json.♻️ Proposed fix
-RUN git clone https://github.com/microsoft/vcpkg.git \ - && cd vcpkg && ./bootstrap-vcpkg.sh +RUN git clone https://github.com/microsoft/vcpkg.git \ + && cd vcpkg && git checkout b322364f06308bdd24823f9d8f03fe0cc86fd46f \ + && ./bootstrap-vcpkg.sh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile.android` around lines 13 - 14, The Dockerfile.android currently clones vcpkg without pinning, causing non-reproducible builds; update the RUN that performs "git clone ... vcpkg" / "cd vcpkg" / "./bootstrap-vcpkg.sh" to checkout the specific commit referenced by the vcpkg.json builtin-baseline before running bootstrap; find the commit SHA in vcpkg.json and ensure the Dockerfile checks out that SHA (e.g., after cloning and before bootstrapping) so builds are reproducible.setup_android_deps.sh (2)
103-106: Usingsudoin build script may be unexpected.Auto-installing packages with
sudo apt-getcan surprise users who expect build scripts to be non-privileged. Consider adding a prompt or documenting this behavior.📝 Proposed improvement
if ! gcc -m32 -x c -c /dev/null -o /dev/null 2>/dev/null; then - echo "Installing gcc-multilib for 32-bit cross-compilation..." - sudo apt-get install -y gcc-multilib g++-multilib + echo "WARNING: gcc-multilib required for 32-bit builds but not found." + echo "Installing with: sudo apt-get install -y gcc-multilib g++-multilib" + read -p "Continue? [y/N] " -n 1 -r + echo + if [[ $REPLY =~ ^[Yy]$ ]]; then + sudo apt-get install -y gcc-multilib g++-multilib + else + echo "Skipping 32-bit ABIs (armeabi-v7a, x86)" + # Filter out 32-bit ABIs from ABI_MAP + unset 'ABI_MAP[armeabi-v7a]' 'ABI_MAP[x86]' + fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup_android_deps.sh` around lines 103 - 106, The block that auto-installs multilib using "sudo apt-get install -y gcc-multilib g++-multilib" should not run without explicit user consent; replace the automatic sudo install in the if-check (the snippet starting with "if ! gcc -m32 -x c -c /dev/null -o /dev/null") with a non-privileged prompt or clear message: detect missing 32-bit compiler as now, then print instructions for the user to run the install command themselves (or ask for confirmation before calling sudo), so the script no longer surprises users by invoking sudo without explicit approval.
55-55: Quote$(nproc)to satisfy shellcheck.While safe in this context, quoting prevents potential issues if
nprocreturns unexpected output.♻️ Proposed fix
- make -j$(nproc) \ + make -j"$(nproc)" \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup_android_deps.sh` at line 55, Quote the subshell expansion used for make's job count to satisfy shellcheck: in the line containing the make invocation with the -j$(nproc) argument, surround the $(nproc) substitution with double quotes so the -j value is provided as "-$(nproc)" (i.e., change -j$(nproc) to use a quoted subshell expansion) to avoid potential issues if nproc outputs unexpected whitespace or characters.setup_android_wsl.sh (1)
4-6: Hardcoded paths reduce script portability.The paths
/home/dev/...assume a specific environment. Consider using environment variables with defaults or deriving paths from the script location.♻️ Proposed fix
-ANDROID_HOME=/home/dev/android-sdk -VCPKG_ROOT=/home/dev/vcpkg -PROJECT_DIR=/home/dev/kizu-otc +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +ANDROID_HOME="${ANDROID_HOME:-$HOME/android-sdk}" +VCPKG_ROOT="${VCPKG_ROOT:-$HOME/vcpkg}" +PROJECT_DIR="${PROJECT_DIR:-$(dirname "$SCRIPT_DIR")}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup_android_wsl.sh` around lines 4 - 6, Replace the hardcoded paths set in ANDROID_HOME, VCPKG_ROOT, and PROJECT_DIR with values derived from environment variables and sensible defaults or from the script location; for example, read existing env vars (ANDROID_HOME, VCPKG_ROOT, PROJECT_DIR) and fall back to "$HOME/android-sdk" or a path relative to the script directory (use dirname "$0") if not set, so the variables in the script are assigned via environment-aware defaults rather than fixed /home/dev/... literals.src/framework/core/minizip/ioapi.c (1)
99-110: Memory leak and potential null pointer dereference infile_build_ioposix.This vendored code has potential issues:
- Line 104-108: No null checks after
malloc()calls - could dereference null on allocation failure- Line 109: If
mallocforioposix->filename(line 107) fails, the previously allocatedioposixis leakedSince this is vendored code from the established minizip library, these are pre-existing issues. Consider whether to patch locally or accept as-is given the low likelihood of malloc failure in practice.
Optional defensive fix
static voidpf file_build_ioposix(FILE *file, const char *filename) { FILE_IOPOSIX *ioposix = NULL; if (file == NULL) return NULL; ioposix = (FILE_IOPOSIX*)malloc(sizeof(FILE_IOPOSIX)); + if (ioposix == NULL) + return NULL; ioposix->file = file; ioposix->filenameLength = (int)strlen(filename) + 1; ioposix->filename = (char*)malloc(ioposix->filenameLength * sizeof(char)); + if (ioposix->filename == NULL) { + free(ioposix); + return NULL; + } strncpy((char*)ioposix->filename, filename, ioposix->filenameLength); return (voidpf)ioposix; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/core/minizip/ioapi.c` around lines 99 - 110, In file_build_ioposix, add defensive null checks after each malloc: check the allocation of ioposix (from malloc(sizeof(FILE_IOPOSIX))) and return NULL on failure; then allocate ioposix->filename and if that malloc fails free(ioposix) and return NULL to avoid leaking the FILE_IOPOSIX struct; only assign ioposix->filenameLength and copy the filename after successful allocation (and handle a NULL filename parameter if needed) so you never dereference null pointers or leak memory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/BUILDING.md`:
- Around line 99-102: The docs show running the otclient-android image to
extract the APK but the Dockerfile.android uses a scratch final stage so docker
run can't produce files; update the instructions to build the image
(Dockerfile.android -> tag otclient-android) and then extract the artifact by
creating a container from otclient-android and using docker cp to copy
/app-release.apk (or the correct artifact path) to the host, then remove the
temporary container, or alternatively recommend using docker build --output to
directly emit the apk; reference the otclient-android image name and the scratch
final stage when making the change.
In `@Dockerfile.android`:
- Line 2: Update the Dockerfile's base image FROM instruction (the "FROM
thyrlian/android-sdk:latest AS builder" line) to pin a specific, stable tag or
digest instead of :latest; replace ":latest" with a chosen version string or
immutable digest (e.g., a semantic version or SHA256 digest) so builds are
reproducible and won't break when the upstream image changes.
In `@setup_android_deps.sh`:
- Around line 72-74: The headers are being installed to
"$INSTALL_DIR/include/luajit-2.1" but src/CMakeLists.txt expects
"include/luajit/", so change the install path in this script to match
CMakeLists: update the mkdir target and the cp destination from
include/luajit-2.1 to include/luajit (i.e., replace mkdir -p
"$INSTALL_DIR/include/luajit-2.1" and the cp target
"$INSTALL_DIR/include/luajit-2.1/" so headers land in
"$INSTALL_DIR/include/luajit/"); alternatively, if you prefer to change CMake,
update the include directory reference in src/CMakeLists.txt to
"include/luajit-2.1"—pick one consistent path and apply it in both the mkdir/cp
lines and the CMakeLists.txt include reference.
In `@src/CMakeLists.txt`:
- Around line 187-200: The LuaJIT include directory constant LUAJIT_ANDROID_INC
points to "include/luajit" but the setup script installs headers to
"include/luajit-2.1", causing include path mismatch; update the CMake variable
LUAJIT_ANDROID_INC (and any uses: LUAJIT_INCLUDE_DIR, include_directories) to
"${LUAJIT_ANDROID_LIBS}/include/luajit-2.1" (or alternatively change the
setup_android_deps.sh install path to "include/luajit") so the include path used
by set(LUAJIT_ANDROID_INC ...), set(LUAJIT_INCLUDE_DIR ...), and
include_directories(...) matches the actual installed headers.
In `@src/framework/core/minizip/crypt.h`:
- Around line 19-28: The header is missing the <stdint.h> include required for
fixed-width types used in this file (uint8_t, uint32_t, int32_t); add a single
`#include` <stdint.h> near the top of the file (alongside existing includes and
before any function declarations) so symbols used in prototypes (e.g., uses of
uint8_t/uint32_t/int32_t in the minizip crypt API) are defined; keep it inside
the include-guard (_MINICRYPT_H) and after any C++ extern "C" guard setup as
appropriate.
In `@src/framework/core/minizip/ioapi_mem.c`:
- Around line 91-100: The code grows the in-memory buffer by allocating newbase
and immediately uses memcpy without checking malloc result; add a NULL check
after newbase = (char *)malloc(newmemsize) in the function that manipulates mem
(uses mem->base, mem->size and IOMEM_BUFFERSIZE) and handle allocation failure
by leaving mem unchanged and returning an error/false (or appropriate failure
code for the surrounding API) instead of calling memcpy/free on NULL; only
proceed to memcpy(mem->base, mem->size) and update mem->base/mem->size when
newbase != NULL.
In `@src/framework/core/minizip/unzip.c`:
- Around line 1440-1447: The calculation of total_out_before/total_out_after in
unzip.c uses (((uint32_t)s->pfile_in_zip_read->bstream.total_out_hi32) << 32)
which performs a 32-bit shift on a 32-bit type (undefined behavior); cast
total_out_hi32 to a 64-bit type before shifting (e.g.,
(uint64_t)s->pfile_in_zip_read->bstream.total_out_hi32 << 32) and ensure the sum
is computed in 64-bit (uint64_t) to produce correct 64-bit totals around the
BZ2_bzDecompress call and its surrounding uses (refer to total_out_before,
total_out_after, pfile_in_zip_read->bstream.total_out_hi32 and
BZ2_bzDecompress).
In `@src/framework/platform/androidwindow.cpp`:
- Around line 338-344: The AndroidWindow methods call
g_androidManager.getClipboardText() and g_androidManager.setClipboardText(...),
but AndroidManager lacks those declarations; add corresponding public member
function declarations and implementations to AndroidManager (e.g., std::string
getClipboardText() and void setClipboardText(const std::string&)) that forward
to the platform clipboard logic used elsewhere, and ensure their signatures
match how AndroidWindow invokes them (returning std::string and accepting a
const std::string& or std::string parameter); update AndroidManager header and
implementation files accordingly so g_androidManager.getClipboardText() and
g_androidManager.setClipboardText(...) compile.
---
Outside diff comments:
In `@src/framework/core/unzipper.cpp`:
- Around line 47-48: The malloc'd buffer assigned to unzmem.base is never freed;
ensure you free(unzmem.base) after you're done with the unzipper (e.g.,
immediately after calling unzClose()) and also on all early-return/error paths
in the function to avoid leaks—update the code paths around unzmem.base
allocation and the unzClose() call so that unzmem.base is freed exactly once
when the in-memory unz structure is no longer needed.
- Line 120: The call to g_logger.fatal currently uses the invalid expression
&"error: " [ error], which performs unintended pointer arithmetic; fix by
building a proper formatted message that includes the numeric/enum error value
and passing that to g_logger.fatal (e.g. use the logger's printf-style overload
or construct a std::string via std::to_string/error-to-string conversion or an
ostringstream and pass that), replacing the faulty expression where
g_logger.fatal(...) is called and using the local variable error as the value to
include.
In `@src/framework/platform/androidmanager.cpp`:
- Around line 113-120: The code allocates dataContent with malloc but
deallocates it with delete[], causing undefined behavior; replace the
deallocation with free(dataContent) (or alternatively allocate with new
char[...] and keep delete[]) and ensure <cstdlib> is included if free isn't
already available; verify the buffer passed to unzipper::extract still uses
dataContent and AAsset_close remains after free.
In `@src/framework/platform/androidwindow.cpp`:
- Around line 270-280: The long-press detection mixes clock sources: m_lastPress
is set from g_clock.millis() but later compared to stdext::millis(); update the
comparison to use the same clock (e.g., replace stdext::millis() with
g_clock.millis() or vice versa) so the check in the TOUCH_UP branch (the if that
uses m_lastPress + 500) uses the same clock that set m_lastPress; adjust
references around m_lastPress, g_clock.millis(), stdext::millis() in the
TOUCH_UP handling (including the block that sets mouseButton to
Fw::MouseRightButton and the subsequent g_dispatcher.addEvent that modifies
m_mouseButtonStates) so timing is consistent.
---
Nitpick comments:
In `@android/app/build.gradle.kts`:
- Around line 36-43: The signingConfigs block defines create("release") but
points to the debug keystore, which is misleading; update the config name from
"release" to something explicit like "debugRelease" (or add a clear inline
comment) and keep the existing values (storeFile =
file(System.getProperty("user.home") + "/.android/debug.keystore"),
storePassword/keyAlias/keyPassword) so callers don’t mistake this for a real
Play Store release signing config; ensure any references to the signing config
elsewhere (e.g., in buildTypes or signingConfig usage) are updated to the new
name.
In `@build_luajit_android.sh`:
- Line 43: The make invocation uses an unquoted command substitution in the
parallel flag (make -j$(nproc) ...), which can trigger word-splitting warnings;
update the make command to quote the substitution so it reads make -j"$(nproc)"
(i.e., replace $(nproc) with "$(nproc)" in the make -j usage) to satisfy
ShellCheck SC2046 and avoid potential splitting issues.
- Around line 24-27: The check that installs gcc-multilib assumes a Debian-based
system; update the if-block that tests "gcc -m32 ..." to first detect the
package manager (e.g., check for apt-get) and only attempt "sudo apt-get install
-y gcc-multilib g++-multilib" when apt-get is present, otherwise print a clear,
actionable error message describing the requirement and supported package
managers (or instruct the user to install equivalent packages for their distro)
and exit non-zero; reference the installation block that runs "sudo apt-get
install -y gcc-multilib g++-multilib" so you modify that branch to perform
package-manager detection and user-friendly messaging.
In `@Dockerfile.android`:
- Line 4: The Dockerfile ends with USER root which triggers a static-analysis
warning; either explicitly document that this is a build-only stage and safe to
run as root (add a comment above the USER root line explaining artifacts are
copied to a scratch/producer stage), or change to a non-root runtime user by
adding commands to create and set a less-privileged user (e.g., RUN groupadd -r
app && useradd -r -g app app && chown -R app:app /output && then replace USER
root with USER app) so the image does not end running as root.
- Around line 13-14: The Dockerfile.android currently clones vcpkg without
pinning, causing non-reproducible builds; update the RUN that performs "git
clone ... vcpkg" / "cd vcpkg" / "./bootstrap-vcpkg.sh" to checkout the specific
commit referenced by the vcpkg.json builtin-baseline before running bootstrap;
find the commit SHA in vcpkg.json and ensure the Dockerfile checks out that SHA
(e.g., after cloning and before bootstrapping) so builds are reproducible.
In `@setup_android_deps.sh`:
- Around line 103-106: The block that auto-installs multilib using "sudo apt-get
install -y gcc-multilib g++-multilib" should not run without explicit user
consent; replace the automatic sudo install in the if-check (the snippet
starting with "if ! gcc -m32 -x c -c /dev/null -o /dev/null") with a
non-privileged prompt or clear message: detect missing 32-bit compiler as now,
then print instructions for the user to run the install command themselves (or
ask for confirmation before calling sudo), so the script no longer surprises
users by invoking sudo without explicit approval.
- Line 55: Quote the subshell expansion used for make's job count to satisfy
shellcheck: in the line containing the make invocation with the -j$(nproc)
argument, surround the $(nproc) substitution with double quotes so the -j value
is provided as "-$(nproc)" (i.e., change -j$(nproc) to use a quoted subshell
expansion) to avoid potential issues if nproc outputs unexpected whitespace or
characters.
In `@setup_android_wsl.sh`:
- Around line 4-6: Replace the hardcoded paths set in ANDROID_HOME, VCPKG_ROOT,
and PROJECT_DIR with values derived from environment variables and sensible
defaults or from the script location; for example, read existing env vars
(ANDROID_HOME, VCPKG_ROOT, PROJECT_DIR) and fall back to "$HOME/android-sdk" or
a path relative to the script directory (use dirname "$0") if not set, so the
variables in the script are assigned via environment-aware defaults rather than
fixed /home/dev/... literals.
In `@src/framework/core/minizip/ioapi.c`:
- Around line 99-110: In file_build_ioposix, add defensive null checks after
each malloc: check the allocation of ioposix (from malloc(sizeof(FILE_IOPOSIX)))
and return NULL on failure; then allocate ioposix->filename and if that malloc
fails free(ioposix) and return NULL to avoid leaking the FILE_IOPOSIX struct;
only assign ioposix->filenameLength and copy the filename after successful
allocation (and handle a NULL filename parameter if needed) so you never
dereference null pointers or leak memory.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 52360faf-768b-4c9c-abde-41ee37b1eb23
📒 Files selected for processing (23)
.dockerignore.gitignoreDockerfile.androidandroid/BUILDING.mdandroid/app/build.gradle.ktsandroid/app/src/main/AndroidManifest.xmlbuild_luajit_android.shsetup_android_deps.shsetup_android_wsl.shsrc/CMakeLists.txtsrc/framework/core/minizip/crypt.csrc/framework/core/minizip/crypt.hsrc/framework/core/minizip/ioapi.csrc/framework/core/minizip/ioapi.hsrc/framework/core/minizip/ioapi_mem.csrc/framework/core/minizip/ioapi_mem.hsrc/framework/core/minizip/unzip.csrc/framework/core/minizip/unzip.hsrc/framework/core/unzipper.cppsrc/framework/platform/androidmanager.cppsrc/framework/platform/androidwindow.cppsrc/framework/ui/uitextedit.cppsrc/main.cpp
Kizuno18
force-pushed
the
fix/android-black-screen
branch
from
9d5dfa4 to
fb15559
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/framework/core/unzipper.cpp (1)
86-103:⚠️ Potential issue | 🟠 MajorBlock zip-slip paths before writing extracted files.
filenameis still appended todestinationPathverbatim. With the newcreate_directories()calls, entries like../...or absolute paths can now escape the extraction root instead of failing implicitly.🔒 Suggested guard
+ const auto entryPath = std::filesystem::path(filename).lexically_normal(); + if (entryPath.is_absolute() || entryPath.string().starts_with("..")) { + unzClose(zipfile); + g_logger.fatal("zip entry escapes destination folder"); + } + const auto outputPath = destinationFolder / entryPath; + if (filename[ filename_length-1 ] == dir_delimiter ) { - std::filesystem::create_directories({ destinationPath + filename }); + std::filesystem::create_directories(outputPath); } else { // Entry is a file, so extract it. if ( unzOpenCurrentFile( zipfile ) != UNZ_OK ) @@ - std::string destFilePath = destinationPath + filename; + const std::string destFilePath = outputPath.string(); // Ensure parent directory exists (zip may not have explicit directory entries) - std::filesystem::path parentDir = std::filesystem::path(destFilePath).parent_path(); + const std::filesystem::path parentDir = outputPath.parent_path();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/core/unzipper.cpp` around lines 86 - 103, Reject or sanitize zip entries that try to escape the intended extraction root: build the extraction target using std::filesystem::path dest = std::filesystem::path(destinationPath) / filename and normalize it (e.g., dest = dest.lexically_normal()), compute the normalized extraction root std::filesystem::path root = std::filesystem::path(destinationPath).lexically_normal(), then verify dest is not absolute and that dest.string() begins with root.string() (or use std::mismatch/compare on path components) — if the check fails, log an error and skip the entry instead of creating directories or writing the file; apply this check before using destFilePath/destFilePath.parent_path(), before calling create_directories(), and before opening the file after unzOpenCurrentFile in unzipper.cpp.
🧹 Nitpick comments (3)
src/CMakeLists.txt (1)
241-244: Scope the vendored unzip sources to Android only.
framework/core/unzipper.cppis already Android-only, so compiling the four MiniZip C files into every desktop/WASM build just adds dead code and extra portability surface.♻️ Suggested scoping
- framework/core/minizip/ioapi.c - framework/core/minizip/ioapi_mem.c - framework/core/minizip/unzip.c - framework/core/minizip/crypt.cif(ANDROID) list(APPEND SOURCE_FILES framework/core/minizip/ioapi.c framework/core/minizip/ioapi_mem.c framework/core/minizip/unzip.c framework/core/minizip/crypt.c ) endif()Move the matching
set_source_files_properties(...)block under the sameif(ANDROID)guard as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CMakeLists.txt` around lines 241 - 244, The four MiniZip C sources (framework/core/minizip/ioapi.c, ioapi_mem.c, unzip.c, crypt.c) are being added unconditionally; wrap the list(APPEND SOURCE_FILES ... ) that includes those filenames in an if(ANDROID) ... endif() guard and move the matching set_source_files_properties(...) block under the same if(ANDROID) guard so both the file additions and their property settings are only applied for Android builds.src/framework/core/minizip/ioapi.c (1)
99-110: Memory safety issues in vendored minizip code.The
file_build_ioposixfunction has potential NULL pointer dereferences ifmallocfails:
- Line 104: No NULL check after
malloc(sizeof(FILE_IOPOSIX))- Line 107: No NULL check after
malloc(...)for filenameIf memory allocation fails, subsequent dereferences (lines 105-108) will crash. Similar patterns exist in
fopendisk64_file_funcandfopendisk_file_func.Since this is vendored third-party code (minizip v1.2), these are known upstream issues. Consider either:
- Tracking upstream for fixes
- Applying a minimal defensive patch if Android builds run in memory-constrained environments
🛡️ Defensive patch for file_build_ioposix
static voidpf file_build_ioposix(FILE *file, const char *filename) { FILE_IOPOSIX *ioposix = NULL; if (file == NULL) return NULL; ioposix = (FILE_IOPOSIX*)malloc(sizeof(FILE_IOPOSIX)); + if (ioposix == NULL) + return NULL; ioposix->file = file; ioposix->filenameLength = (int)strlen(filename) + 1; ioposix->filename = (char*)malloc(ioposix->filenameLength * sizeof(char)); + if (ioposix->filename == NULL) { + free(ioposix); + return NULL; + } strncpy((char*)ioposix->filename, filename, ioposix->filenameLength); return (voidpf)ioposix; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/core/minizip/ioapi.c` around lines 99 - 110, file_build_ioposix (and similarly fopendisk64_file_func / fopendisk_file_func) can dereference NULL if malloc fails; add defensive NULL checks: after allocating FILE_IOPOSIX (ioposix = malloc(...)) verify ioposix != NULL and return NULL on failure, then after allocating ioposix->filename verify it != NULL and if it is free(ioposix) and return NULL; ensure you only compute/use ioposix->filenameLength and strncpy when both allocations succeeded and preserve behavior by returning (voidpf)ioposix on success.src/framework/platform/androidwindow.cpp (1)
264-280: Potential logic issue in right-click detection.The right-button assignment on long press (lines 274-277) occurs inside the
TOUCH_UPbranch, butmouseButtonis used earlier on line 268 to setm_inputEvent.mouseButton. When a long press is detected,mouseButtonis reassigned toFw::MouseRightButtonbut this happens after the input event type was already set withFw::MouseLeftButton. The subsequent re-assignment on line 276 fixesm_inputEvent.mouseButton, but this logic flow is confusing and error-prone.Consider restructuring to determine the button type before setting any input event fields.
♻️ Suggested refactor
void AndroidWindow::processFingerDownAndUp() { bool isTouchdown = m_currentEvent.type == TOUCH_DOWN; - Fw::MouseButton mouseButton = Fw::MouseLeftButton; - - m_inputEvent.reset(); - m_inputEvent.type = (isTouchdown) ? Fw::MousePressInputEvent : Fw::MouseReleaseInputEvent; - m_inputEvent.mouseButton = mouseButton; + Fw::MouseButton mouseButton = Fw::MouseLeftButton; if (isTouchdown) { m_lastPress = g_clock.millis(); - m_mouseButtonStates |= 1 << mouseButton; } else if (m_currentEvent.type == TOUCH_UP) { if (!m_isDragging && stdext::millis() > m_lastPress + 500) { mouseButton = Fw::MouseRightButton; - m_inputEvent.mouseButton = mouseButton; } m_isDragging = false; + } + + m_inputEvent.reset(); + m_inputEvent.type = (isTouchdown) ? Fw::MousePressInputEvent : Fw::MouseReleaseInputEvent; + m_inputEvent.mouseButton = mouseButton; + + if (isTouchdown) { + m_mouseButtonStates |= 1 << mouseButton; + } else { g_dispatcher.addEvent([this, mouseButton] { m_mouseButtonStates &= ~(1 << mouseButton); }); } handleInputEvent(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/platform/androidwindow.cpp` around lines 264 - 280, Determine the correct mouse button before writing any fields to m_inputEvent: compute mouseButton based on isTouchdown (default MouseLeftButton) and, if !isTouchdown and m_currentEvent.type == TOUCH_UP and stdext::millis() > m_lastPress + 500 and !m_isDragging, set mouseButton = Fw::MouseRightButton; then set m_inputEvent.type and m_inputEvent.mouseButton once using that computed mouseButton. Keep the existing behaviors for updating m_lastPress, toggling m_isDragging, updating m_mouseButtonStates (use |= when pressing and schedule &= ~(1 << mouseButton) via g_dispatcher.addEvent on release), but move those updates so they use the precomputed mouseButton and avoid reassigning m_inputEvent.mouseButton later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build_luajit_android.sh`:
- Line 9: The script's use of "set -e" doesn't catch failures from piped
commands like the "make ... | tail -3" invocation, so a failed LuaJIT build can
still proceed to header installation and print a success banner; update
build_luajit_android.sh to either enable pipefail (e.g., set -euo pipefail) at
the top or explicitly check the exit status of the make pipeline (e.g., inspect
PIPESTATUS[0] or capture make's exit code via tee) after the "make ... | tail
-3" call and abort (non-zero exit) on failure before proceeding to the header
install and success message.
In `@Dockerfile.android`:
- Around line 24-36: The Dockerfile proceeds to run the Android Gradle build
(./gradlew_unix assembleRelease) without producing the required LuaJIT static
libraries (libluajit-5.1.a), which causes CMakeLists.txt to emit a FATAL_ERROR;
fix by adding a pre-build step in the Dockerfile to generate the LuaJIT
artifacts (invoke ./build_luajit_android.sh or the project's
./setup_android_deps.sh) after copying the repo and before the Gradle build so
that android/app/libs/lib/<ABI>/ contains libluajit-5.1.a for each ABI expected
by CMake.
In `@setup_android_deps.sh`:
- Around line 122-129: The script currently prints "Now run: cd android &&
./gradlew assembleRelease" but never creates the expected
android/app/src/main/assets/data.zip, causing AndroidManager::unZipAssetData()
to fail; update the end of the setup_android_deps.sh flow to mirror
Dockerfile.android by ensuring the assets directory exists and producing a
data.zip (e.g., cd into the data source directory and run zip -r into
android/app/src/main/assets/data.zip or invoke the same packaging step used in
Dockerfile.android) so the APK/assembleRelease step includes the asset that
AndroidManager::unZipAssetData() expects.
- Around line 55-65: Add the missing amalg build target and the TARGET_CFLAGS
with -DLUAJIT_UNWIND_EXTERNAL to the make invocation in setup_android_deps.sh so
it matches build_luajit_android.sh: update the make command that currently sets
HOST_CC, CROSS, STATIC_CC, DYNAMIC_CC, TARGET_LD, TARGET_AR, TARGET_STRIP,
TARGET_SYS, and BUILDMODE to also include TARGET_CFLAGS="${NDKBIN}/${CROSS}clang
-fPIC ? or the appropriate CFLAGS variable used in build_luajit_android.sh plus
-DLUAJIT_UNWIND_EXTERNAL" (use the same variable name TARGET_CFLAGS as in
build_luajit_android.sh) and append the amalg target to the make call so the
build uses the single-file amalgamated target for Android NDK C++ interop.
- Line 5: The script uses "set -e" but pipes the LuaJIT build (the "make ... |
tail -3" pipeline) and the "vcpkg install ... | tail -3" pipeline so failures
are masked by tail's success; enable pipe failure detection by adding "set -o
pipefail" near "set -e" or explicitly check the producer's exit status (e.g.,
inspect PIPESTATUS[0] after the "make ... | tail -3" and after the "vcpkg
install ... | tail -3") and exit if non‑zero; alternatively wrap those pipelines
in an if/then (if ! make ... | tail -3; then exit 1; fi) to ensure LuaJIT build
and vcpkg install failures stop the script.
In `@src/framework/core/minizip/ioapi.c`:
- Around line 55-64: The function call_ztell64 has the wrong NULL check: it
tests pfilefunc->zfile_func64.zseek64_file but then calls ztell64_file, risking
a NULL-deref; update the condition to check pfilefunc->zfile_func64.ztell64_file
before calling it, and keep the fallback to pfilefunc->ztell32_file unchanged
(optionally ensure ztell32_file is non-NULL if desired); modify the check in
call_ztell64 so it verifies ztell64_file is not NULL and then invokes
ztell64_file(opaque, filestream).
In `@src/framework/core/minizip/unzip.c`:
- Around line 1487-1495: The variable flags is used uninitialized in the Apple
compression path; initialize it before use so it has a defined value when
s->pfile_in_zip_read->stream.avail_in != 0. For example, set flags to 0 (no
flags) by default, or explicitly set flags = 0 in the else branch, then call
compression_stream_process(&s->pfile_in_zip_read->astream, flags); this ensures
flags is defined for the call.
---
Outside diff comments:
In `@src/framework/core/unzipper.cpp`:
- Around line 86-103: Reject or sanitize zip entries that try to escape the
intended extraction root: build the extraction target using
std::filesystem::path dest = std::filesystem::path(destinationPath) / filename
and normalize it (e.g., dest = dest.lexically_normal()), compute the normalized
extraction root std::filesystem::path root =
std::filesystem::path(destinationPath).lexically_normal(), then verify dest is
not absolute and that dest.string() begins with root.string() (or use
std::mismatch/compare on path components) — if the check fails, log an error and
skip the entry instead of creating directories or writing the file; apply this
check before using destFilePath/destFilePath.parent_path(), before calling
create_directories(), and before opening the file after unzOpenCurrentFile in
unzipper.cpp.
---
Nitpick comments:
In `@src/CMakeLists.txt`:
- Around line 241-244: The four MiniZip C sources
(framework/core/minizip/ioapi.c, ioapi_mem.c, unzip.c, crypt.c) are being added
unconditionally; wrap the list(APPEND SOURCE_FILES ... ) that includes those
filenames in an if(ANDROID) ... endif() guard and move the matching
set_source_files_properties(...) block under the same if(ANDROID) guard so both
the file additions and their property settings are only applied for Android
builds.
In `@src/framework/core/minizip/ioapi.c`:
- Around line 99-110: file_build_ioposix (and similarly fopendisk64_file_func /
fopendisk_file_func) can dereference NULL if malloc fails; add defensive NULL
checks: after allocating FILE_IOPOSIX (ioposix = malloc(...)) verify ioposix !=
NULL and return NULL on failure, then after allocating ioposix->filename verify
it != NULL and if it is free(ioposix) and return NULL; ensure you only
compute/use ioposix->filenameLength and strncpy when both allocations succeeded
and preserve behavior by returning (voidpf)ioposix on success.
In `@src/framework/platform/androidwindow.cpp`:
- Around line 264-280: Determine the correct mouse button before writing any
fields to m_inputEvent: compute mouseButton based on isTouchdown (default
MouseLeftButton) and, if !isTouchdown and m_currentEvent.type == TOUCH_UP and
stdext::millis() > m_lastPress + 500 and !m_isDragging, set mouseButton =
Fw::MouseRightButton; then set m_inputEvent.type and m_inputEvent.mouseButton
once using that computed mouseButton. Keep the existing behaviors for updating
m_lastPress, toggling m_isDragging, updating m_mouseButtonStates (use |= when
pressing and schedule &= ~(1 << mouseButton) via g_dispatcher.addEvent on
release), but move those updates so they use the precomputed mouseButton and
avoid reassigning m_inputEvent.mouseButton later.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fbbac8d8-6a94-4e4e-b70d-4b940663c9f5
📒 Files selected for processing (23)
.dockerignore.gitignoreDockerfile.androidandroid/BUILDING.mdandroid/app/build.gradle.ktsandroid/app/src/main/AndroidManifest.xmlbuild_luajit_android.shsetup_android_deps.shsetup_android_wsl.shsrc/CMakeLists.txtsrc/framework/core/minizip/crypt.csrc/framework/core/minizip/crypt.hsrc/framework/core/minizip/ioapi.csrc/framework/core/minizip/ioapi.hsrc/framework/core/minizip/ioapi_mem.csrc/framework/core/minizip/ioapi_mem.hsrc/framework/core/minizip/unzip.csrc/framework/core/minizip/unzip.hsrc/framework/core/unzipper.cppsrc/framework/platform/androidmanager.cppsrc/framework/platform/androidwindow.cppsrc/framework/ui/uitextedit.cppsrc/main.cpp
✅ Files skipped from review due to trivial changes (5)
- .gitignore
- android/app/src/main/AndroidManifest.xml
- .dockerignore
- android/app/build.gradle.kts
- src/framework/core/minizip/crypt.h
🚧 Files skipped from review as they are similar to previous changes (5)
- src/framework/ui/uitextedit.cpp
- src/main.cpp
- src/framework/platform/androidmanager.cpp
- setup_android_wsl.sh
- android/BUILDING.md
Kizuno18
force-pushed
the
fix/android-black-screen
branch
2 times, most recently
from
fdb92cd to
c7ec6fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/framework/platform/androidwindow.cpp (1)
264-279:⚠️ Potential issue | 🟠 MajorLong-press release leaves the left button stuck down.
Line 272 always marks
Fw::MouseLeftButtonas pressed, but Lines 274-279 switch only the release event to right-button and clear the right-bit. After a long press,m_mouseButtonStatesstill contains left.Suggested patch
if (isTouchdown) { m_lastPress = g_clock.millis(); - m_mouseButtonStates |= 1 << mouseButton; + m_mouseButtonStates |= 1 << Fw::MouseLeftButton; } else if (m_currentEvent.type == TOUCH_UP) { if (!m_isDragging && stdext::millis() > m_lastPress + 500) { mouseButton = Fw::MouseRightButton; m_inputEvent.mouseButton = mouseButton; } m_isDragging = false; - g_dispatcher.addEvent([this, mouseButton] { m_mouseButtonStates &= ~(1 << mouseButton); }); + g_dispatcher.addEvent([this] { m_mouseButtonStates &= ~(1 << Fw::MouseLeftButton); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/platform/androidwindow.cpp` around lines 264 - 279, The release path incorrectly always sets the initial mouseButton to Fw::MouseLeftButton and later may reassign it to right for the input event but then clears the bit for the reassigned button, leaving the left-bit stuck; fix by preserving the actual pressed button (capture the pressedButton before you mutate mouseButton) or compute the bit to clear from the originally set pressed button (m_mouseButtonStates) rather than the possibly reassigned mouseButton, then use that preserved value when calling m_mouseButtonStates &= ~(1 << ... ) inside the g_dispatcher.addEvent block so the correct bit (left or right) is cleared after release.
♻️ Duplicate comments (2)
setup_android_deps.sh (1)
55-65:⚠️ Potential issue | 🟠 MajorKeep the LuaJIT Android build flags aligned with the dedicated builder.
This path still omits the
amalgtarget andTARGET_CFLAGS="-fPIC -DLUAJIT_UNWIND_EXTERNAL -fno-stack-protector", sosetup_android_deps.shcan emit differentlibluajit-5.1.aartifacts frombuild_luajit_android.shfor the same ABI.Suggested patch
- make -j$(nproc) \ + make -j"$(nproc)" amalg \ HOST_CC="$HOST_CC" \ CROSS="${NDKBIN}/${CROSS}" \ STATIC_CC="${NDKBIN}/${CROSS}clang -fPIC" \ DYNAMIC_CC="${NDKBIN}/${CROSS}clang -fPIC" \ TARGET_LD="${NDKBIN}/${CROSS}clang" \ TARGET_AR="$NDKBIN/llvm-ar rcus" \ TARGET_STRIP="$NDKBIN/llvm-strip" \ + TARGET_CFLAGS="-fPIC -DLUAJIT_UNWIND_EXTERNAL -fno-stack-protector" \ TARGET_SYS=Linux \ BUILDMODE=static \ 2>&1 | tail -3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup_android_deps.sh` around lines 55 - 65, The make invocation in setup_android_deps.sh omits the LuaJIT-specific build settings causing different artifacts; update the make command (the block invoking make -j$(nproc) with HOST_CC/CROSS/STATIC_CC/etc.) to include the amalg target and set TARGET_CFLAGS to "-fPIC -DLUAJIT_UNWIND_EXTERNAL -fno-stack-protector" so it matches build_luajit_android.sh; ensure the final make arguments append "amalg" and include TARGET_CFLAGS="...".src/framework/core/minizip/unzip.c (1)
1487-1495:⚠️ Potential issue | 🟠 MajorInitialize
flagsbefore calling libcompression.When
avail_in != 0,flagsis never assigned beforecompression_stream_process().Suggested patch
compression_status status; - compression_stream_flags flags; + compression_stream_flags flags = 0; if (s->pfile_in_zip_read->stream.avail_in == 0) { flags = COMPRESSION_STREAM_FINALIZE; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/core/minizip/unzip.c` around lines 1487 - 1495, The variable flags (type compression_stream_flags) may be read uninitialized when s->pfile_in_zip_read->stream.avail_in != 0 before calling compression_stream_process(&s->pfile_in_zip_read->astream, flags); initialize flags to a safe default (e.g., 0 or COMPRESSION_STREAM_NO_FLAGS) before the if, and keep the existing assignment to COMPRESSION_STREAM_FINALIZE inside the if so flags is always defined when passed to compression_stream_process; update the block around compression_status, compression_stream_flags, the avail_in check and compression_stream_process call accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@setup_android_deps.sh`:
- Around line 127-145: The zip branch should mirror the Python branch by
creating a fresh archive of data/mods/modules and only adding init.lua and
otclientrc.lua if they exist; replace the single zip -r
android/app/src/main/assets/data.zip data mods modules init.lua otclientrc.lua
invocation with a sequence that (inside $SCRIPT_DIR) creates/overwrites
android/app/src/main/assets/data.zip from directories data, mods, modules and
then conditionally adds init.lua and otclientrc.lua only when they exist (using
zip -r for the directories and zip -u or additional zip calls guarded by [ -e
"$f" ] for each optional file) so the behavior matches the python block and the
script does not abort when optional files are missing.
In `@src/CMakeLists.txt`:
- Around line 204-212: The Android minizip sources are being lost because a
later set(SOURCE_FILES ...) overrides the earlier list append; create a separate
variable (e.g., ANDROID_MINIZIP_SOURCES) to hold the minizip file list instead
of directly appending to SOURCE_FILES inside the Android conditional, and after
the main set(SOURCE_FILES ...) block but before add_library(otclient_core STATIC
${SOURCE_FILES}) conditionally list(APPEND SOURCE_FILES
${ANDROID_MINIZIP_SOURCES}) when ANDROID is true so the minizip files are
preserved for the final SOURCE_FILES used by add_library.
In `@src/framework/core/minizip/ioapi.c`:
- Around line 158-180: In fopendisk64_file_func (and the similar
fopendisk_file_func at 183-205) guard the diskFilename allocation and string
ops: check malloc returned non-NULL before calling strncpy/snprintf and return
NULL (or handle error) if allocation fails; ensure you allocate space for a
terminating NUL (use filenameLength+1) and explicitly NUL-terminate diskFilename
after copying; only call snprintf if diskFilename is non-NULL and the remaining
buffer size (filenameLength - i) is correct; free diskFilename on all paths and
call fopen64_file_func/fopen_file_func only when diskFilename is valid.
In `@src/framework/core/minizip/unzip.c`:
- Around line 1590-1598: The code in unzGetLocalExtrafield (using
s->pfile_in_zip_read, ZSEEK64 and ZREAD64) fails to account for
byte_before_the_zipfile when seeking and does not advance pos_local_extrafield
after a partial read, causing misaligned or repeated reads for archives with
prepended data and chunked callers. Fix by adding byte_before_the_zipfile to the
seek offset (i.e. include s->pfile_in_zip_read->byte_before_the_zipfile when
computing the ZSEEK64 target) and after a successful ZREAD64 increment
s->pfile_in_zip_read->pos_local_extrafield by the number of bytes read (read_now
or the actual bytes returned) so subsequent calls continue from the right
position; ensure the function returns the actual bytes read and handles partial
reads correctly.
- Around line 511-519: The bridge struct zlib_filefunc64_32_def_fill in
unzOpen2_64 is not fully initialized, leaving zopen32_file and zopendisk32_file
indeterminate; zero-initialize the entire zlib_filefunc64_32_def_fill (e.g.,
memset or aggregate initialization) before setting zfile_func64 and the 32-bit
callbacks, and explicitly set zopen32_file and zopendisk32_file to NULL so
unzOpenInternal sees defined fallbacks.
- Around line 1752-1776: The restore block after a failed locate restores
num_file, pos_in_central_dir, cur_file_info and cur_file_info_internal but omits
s->current_file_ok; update the restore logic to save the original
s->current_file_ok into a local variable (e.g., current_file_ok_saved) before
scanning and then set s->current_file_ok = current_file_ok_saved when restoring,
ensuring functions that call unzGoToFirstFile2/unzGoToNextFile2 can rely on the
previous current file validity.
- Around line 1915-1970: unzSeek64 rebuilds the cursor incorrectly: the reset
path sets pos_in_zipfile to offset_local_extrafield + position (pointing into
the local extra field) and the fast buffered path uses unsigned arithmetic so
backward seeks wrap. Fix by computing a signed delta = (int64_t)position -
(int64_t)s->pfile_in_zip_read->total_out_64 and only take the buffered fast path
when delta >= 0 and delta fits within stream.avail_in/stream window; otherwise
clear the buffer and set pos_in_zipfile to the actual start-of-file-data plus
position (use the field that represents the file data start rather than
offset_local_extrafield, e.g., a stored start-of-data offset or compute from
local header), set rest_read_compressed = cur_file_info.compressed_size -
position, and update rest_read_uncompressed and total_out fields using the
signed delta.
In `@src/framework/core/unzipper.cpp`:
- Around line 85-93: The unsafe-path branch in unzipper.cpp calls
unzCloseCurrentFile() before unzOpenCurrentFile() (violating MiniZip's
lifecycle) and ignores errors from unzGoToNextFile(); remove the
unzCloseCurrentFile() call from the unsafe-path branch, call unzGoToNextFile()
and check its return value (like the normal iteration path), and on failure log
an error via g_logger and abort/return an error status (or break the loop) to
avoid silent failures; reference the unsafe-path branch surrounding entryPath
checks, unzOpenCurrentFile, unzCloseCurrentFile, and unzGoToNextFile to locate
the changes.
---
Outside diff comments:
In `@src/framework/platform/androidwindow.cpp`:
- Around line 264-279: The release path incorrectly always sets the initial
mouseButton to Fw::MouseLeftButton and later may reassign it to right for the
input event but then clears the bit for the reassigned button, leaving the
left-bit stuck; fix by preserving the actual pressed button (capture the
pressedButton before you mutate mouseButton) or compute the bit to clear from
the originally set pressed button (m_mouseButtonStates) rather than the possibly
reassigned mouseButton, then use that preserved value when calling
m_mouseButtonStates &= ~(1 << ... ) inside the g_dispatcher.addEvent block so
the correct bit (left or right) is cleared after release.
---
Duplicate comments:
In `@setup_android_deps.sh`:
- Around line 55-65: The make invocation in setup_android_deps.sh omits the
LuaJIT-specific build settings causing different artifacts; update the make
command (the block invoking make -j$(nproc) with HOST_CC/CROSS/STATIC_CC/etc.)
to include the amalg target and set TARGET_CFLAGS to "-fPIC
-DLUAJIT_UNWIND_EXTERNAL -fno-stack-protector" so it matches
build_luajit_android.sh; ensure the final make arguments append "amalg" and
include TARGET_CFLAGS="...".
In `@src/framework/core/minizip/unzip.c`:
- Around line 1487-1495: The variable flags (type compression_stream_flags) may
be read uninitialized when s->pfile_in_zip_read->stream.avail_in != 0 before
calling compression_stream_process(&s->pfile_in_zip_read->astream, flags);
initialize flags to a safe default (e.g., 0 or COMPRESSION_STREAM_NO_FLAGS)
before the if, and keep the existing assignment to COMPRESSION_STREAM_FINALIZE
inside the if so flags is always defined when passed to
compression_stream_process; update the block around compression_status,
compression_stream_flags, the avail_in check and compression_stream_process call
accordingly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 727ce52a-6ea0-45fa-b18a-bbb8fe0adac7
📒 Files selected for processing (23)
.dockerignore.gitignoreDockerfile.androidandroid/BUILDING.mdandroid/app/build.gradle.ktsandroid/app/src/main/AndroidManifest.xmlbuild_luajit_android.shsetup_android_deps.shsetup_android_wsl.shsrc/CMakeLists.txtsrc/framework/core/minizip/crypt.csrc/framework/core/minizip/crypt.hsrc/framework/core/minizip/ioapi.csrc/framework/core/minizip/ioapi.hsrc/framework/core/minizip/ioapi_mem.csrc/framework/core/minizip/ioapi_mem.hsrc/framework/core/minizip/unzip.csrc/framework/core/minizip/unzip.hsrc/framework/core/unzipper.cppsrc/framework/platform/androidmanager.cppsrc/framework/platform/androidwindow.cppsrc/framework/ui/uitextedit.cppsrc/main.cpp
✅ Files skipped from review due to trivial changes (7)
- src/framework/platform/androidmanager.cpp
- .gitignore
- src/framework/ui/uitextedit.cpp
- .dockerignore
- android/app/src/main/AndroidManifest.xml
- android/BUILDING.md
- src/framework/core/minizip/crypt.h
🚧 Files skipped from review as they are similar to previous changes (2)
- android/app/build.gradle.kts
- setup_android_wsl.sh
Kizuno18
force-pushed
the
fix/android-black-screen
branch
from
c7ec6fb to
dc7c9fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (9)
src/framework/core/minizip/ioapi.c (1)
158-180:⚠️ Potential issue | 🟡 MinorGuard
diskFilenamebefore the first copy.Both spanning helpers still dereference
diskFilenameimmediately aftermalloc(). On allocation failure, that becomes a null-pointer crash.🛠️ Suggested fix
ioposix = (FILE_IOPOSIX*)stream; diskFilename = (char*)malloc(ioposix->filenameLength * sizeof(char)); + if (diskFilename == NULL) + return NULL; strncpy(diskFilename, (const char*)ioposix->filename, ioposix->filenameLength);ioposix = (FILE_IOPOSIX*)stream; diskFilename = (char*)malloc(ioposix->filenameLength * sizeof(char)); + if (diskFilename == NULL) + return NULL; strncpy(diskFilename, (const char*)ioposix->filename, ioposix->filenameLength);Also applies to: 183-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/core/minizip/ioapi.c` around lines 158 - 180, The code in fopendisk64_file_func (and the similar fopendisk_file_func) uses diskFilename immediately after malloc without checking for allocation failure, which can dereference NULL; fix by testing the return of malloc for NULL before calling strncpy/strncpy/snprintf and if allocation fails return NULL (or appropriate error) and avoid calling fopen64_file_func/fopen_file_func; ensure you only free diskFilename when non-NULL and propagate the error path consistently in both functions.src/framework/core/unzipper.cpp (1)
85-92:⚠️ Potential issue | 🟠 MajorDon't close a MiniZip entry that was never opened.
This branch runs before Line 103 calls
unzOpenCurrentFile(), sounzCloseCurrentFile()here is invalid. The uncheckedunzGoToNextFile()also makes a bad iteration state harder to diagnose.🛠️ Suggested fix
if (entryPath.is_absolute() || entryPath.string().starts_with("..")) { g_logger.warning("skipping zip entry with unsafe path: {}", filename); - unzCloseCurrentFile( zipfile ); - if ( ( i+1 ) < global_info.number_entry ) - unzGoToNextFile( zipfile ); + if ( ( i+1 ) < global_info.number_entry && + unzGoToNextFile( zipfile ) != UNZ_OK ) + { + unzClose( zipfile ); + g_logger.fatal("could not read next file"); + } continue; }MiniZip classic API: is `unzCloseCurrentFile()` valid before `unzOpenCurrentFile()`, and should `unzGoToNextFile()` failures be checked when skipping an entry?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/core/unzipper.cpp` around lines 85 - 92, The branch that rejects unsafe entry paths must not call unzCloseCurrentFile() because unzOpenCurrentFile() hasn't been called yet; remove the unzCloseCurrentFile(zipfile) call (or only call it when a file was actually opened), and when advancing with unzGoToNextFile(zipfile) check its return value and log or handle errors (use zipfile, i, global_info.number_entry) instead of ignoring failures; keep the existing g_logger.warning(...) and use entryPath/filename to report context.setup_android_deps.sh (2)
55-65:⚠️ Potential issue | 🟠 MajorMatch this LuaJIT build with the documented Android builder.
Line 55 still omits
amalgandTARGET_CFLAGS=-DLUAJIT_UNWIND_EXTERNAL, sosetup_android_deps.shcan produce a differentlibluajit-5.1.athanbuild_luajit_android.sh.🛠️ Suggested patch
- make -j$(nproc) \ + make -j"$(nproc)" amalg \ HOST_CC="$HOST_CC" \ CROSS="${NDKBIN}/${CROSS}" \ STATIC_CC="${NDKBIN}/${CROSS}clang -fPIC" \ DYNAMIC_CC="${NDKBIN}/${CROSS}clang -fPIC" \ TARGET_LD="${NDKBIN}/${CROSS}clang" \ TARGET_AR="$NDKBIN/llvm-ar rcus" \ TARGET_STRIP="$NDKBIN/llvm-strip" \ + TARGET_CFLAGS="-fPIC -DLUAJIT_UNWIND_EXTERNAL -fno-stack-protector" \ TARGET_SYS=Linux \ BUILDMODE=static \ 2>&1 | tail -3#!/bin/bash # Compare the LuaJIT make invocations used by the two Android build paths. echo "=== setup_android_deps.sh ===" sed -n '52,66p' setup_android_deps.sh echo echo "=== build_luajit_android.sh ===" fd '^build_luajit_android\.sh$' -x sed -n '1,220p' {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup_android_deps.sh` around lines 55 - 65, The LuaJIT make invocation in setup_android_deps.sh is missing the amalg build flag and TARGET_CFLAGS=-DLUAJIT_UNWIND_EXTERNAL so it can produce a different libluajit-5.1.a than build_luajit_android.sh; update the make call (the multi-line make block that sets HOST_CC, CROSS, STATIC_CC, DYNAMIC_CC, TARGET_LD, TARGET_AR, TARGET_STRIP, TARGET_SYS, BUILDMODE) to include amalg and TARGET_CFLAGS=-DLUAJIT_UNWIND_EXTERNAL so the build flags match the documented Android builder.
127-142:⚠️ Potential issue | 🟠 MajorRecreate
data.zipand only add optional root files when they exist.Line 130 updates any existing archive in place and fails the whole script if
init.luaorotclientrc.luais missing. The Python fallback already treats both as optional, so thezippath is currently less reliable than the fallback.🛠️ Suggested patch
if command -v zip &>/dev/null; then - zip -r android/app/src/main/assets/data.zip data mods modules init.lua otclientrc.lua + rm -f android/app/src/main/assets/data.zip + inputs=(data mods modules) + [[ -f init.lua ]] && inputs+=(init.lua) + [[ -f otclientrc.lua ]] && inputs+=(otclientrc.lua) + zip -r android/app/src/main/assets/data.zip "${inputs[@]}" elif command -v python3 &>/dev/null; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup_android_deps.sh` around lines 127 - 142, The zip invocation currently updates an existing archive and fails if optional root files are missing; change the shell branch that runs zip to recreate the archive and only add init.lua and otclientrc.lua if they exist (mirror the Python fallback). Concretely, before calling zip in the shell branch remove any existing android/app/src/main/assets/data.zip, then run zip -r to add the directories data, mods, modules and append init.lua and otclientrc.lua only when test -f returns true (e.g. use $(test -f init.lua && echo init.lua) and similar for otclientrc.lua). Update the block containing the zip command (the shell branch that parallels the Python fallback) so behavior matches the Python fallback.src/framework/core/minizip/unzip.c (5)
511-519:⚠️ Potential issue | 🟠 MajorZero-initialize the 64/32 callback bridge.
Line 515 leaves
zopen32_fileandzopendisk32_fileindeterminate. If a caller provides a partialzlib_filefunc64_def, the fallback path can jump through garbage.🛠️ Suggested patch
if (pzlib_filefunc_def != NULL) { - zlib_filefunc64_32_def zlib_filefunc64_32_def_fill; + zlib_filefunc64_32_def zlib_filefunc64_32_def_fill = {0}; zlib_filefunc64_32_def_fill.zfile_func64 = *pzlib_filefunc_def; - zlib_filefunc64_32_def_fill.ztell32_file = NULL; - zlib_filefunc64_32_def_fill.zseek32_file = NULL; return unzOpenInternal(path, &zlib_filefunc64_32_def_fill); }#!/bin/bash # Inspect the bridge initialization and the fallback dispatch in ioapi.c. echo "=== unzOpen2_64 bridge setup ===" sed -n '511,519p' src/framework/core/minizip/unzip.c echo echo "=== call_zopen64 / call_zopendisk64 fallback logic ===" fd '^ioapi\.c$' -x rg -n -C3 'call_zopen64|call_zopendisk64|zopen32_file|zopendisk32_file' {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/core/minizip/unzip.c` around lines 511 - 519, unzOpen2_64 currently leaves members of the bridge struct zlib_filefunc64_32_def_fill (notably zopen32_file and zopendisk32_file) uninitialized; zero-initialize the whole zlib_filefunc64_32_def_fill before assigning zfile_func64 and the other members so any missing 64-bit callbacks safely fall back to NULL, e.g. clear the struct (memset or initializer) and then set zfile_func64 = *pzlib_filefunc_def and ztell32_file/zseek32_file = NULL to ensure call_zopen64/call_zopendisk64 won't execute garbage.
1487-1495:⚠️ Potential issue | 🟠 MajorInitialize
flagsbeforecompression_stream_process.Line 1488 leaves
flagsundefined wheneveravail_in != 0, which makes the Apple decompression path undefined behavior.🛠️ Suggested patch
compression_status status; - compression_stream_flags flags; + compression_stream_flags flags = 0; if (s->pfile_in_zip_read->stream.avail_in == 0) { flags = COMPRESSION_STREAM_FINALIZE; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/core/minizip/unzip.c` around lines 1487 - 1495, The variable `flags` (type compression_stream_flags) is uninitialized when `s->pfile_in_zip_read->stream.avail_in != 0`; ensure `flags` is given a defined value before calling `compression_stream_process` (e.g. initialize it to 0 or explicitly set it in an else branch) so that the call to `compression_stream_process(&s->pfile_in_zip_read->astream, flags)` always receives a defined flag value; update the code around the `if (s->pfile_in_zip_read->stream.avail_in == 0)` check (referencing `flags`, `compression_stream_process`, `compression_stream_flags`, and `s->pfile_in_zip_read->stream.avail_in`) to set a default/else value.
1590-1598:⚠️ Potential issue | 🟠 MajorAdvance the local-extra-field cursor and include the prepended-data offset.
Lines 1591-1598 reread the wrong bytes for self-extracting archives and for chunked callers because
byte_before_the_zipfileis skipped andpos_local_extrafieldnever advances.🛠️ Suggested patch
if (ZSEEK64(s->pfile_in_zip_read->z_filefunc, s->pfile_in_zip_read->filestream, - s->pfile_in_zip_read->offset_local_extrafield + s->pfile_in_zip_read->pos_local_extrafield, + s->pfile_in_zip_read->offset_local_extrafield + + s->pfile_in_zip_read->byte_before_the_zipfile + + s->pfile_in_zip_read->pos_local_extrafield, ZLIB_FILEFUNC_SEEK_SET) != 0) return UNZ_ERRNO; if (ZREAD64(s->pfile_in_zip_read->z_filefunc, s->pfile_in_zip_read->filestream, buf, read_now) != read_now) return UNZ_ERRNO; + s->pfile_in_zip_read->pos_local_extrafield += read_now; return (int)read_now;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/core/minizip/unzip.c` around lines 1590 - 1598, The code in unzip.c (inside the read-from-local-extra-field sequence that uses ZSEEK64/ZREAD64) fails to include the archive's byte_before_the_zipfile and does not advance pos_local_extrafield, causing wrong bytes to be read for self-extracting archives and chunked callers; fix by seeking to filestream position (byte_before_the_zipfile + offset_local_extrafield + pos_local_extrafield) before reading and after a successful ZREAD64 increment s->pfile_in_zip_read->pos_local_extrafield by read_now (or the actual bytes read) so subsequent reads continue from the correct local-extra-field cursor, ensuring UNZ_ERRNO is still returned on seek/read failures and the function returns (int)read_now on success.
1752-1776:⚠️ Potential issue | 🟠 MajorRestore
current_file_okafter a failed locate.Line 1771 restores the cursor state but not the validity flag. After one miss, the previously current entry becomes unusable until the caller repositions manually.
🛠️ Suggested patch
uint64_t num_file_saved = 0; uint64_t pos_in_central_dir_saved = 0; + uint64_t current_file_ok_saved = 0; char current_filename[UNZ_MAXFILENAMEINZIP+1]; int err = UNZ_OK; @@ num_file_saved = s->num_file; pos_in_central_dir_saved = s->pos_in_central_dir; cur_file_info_saved = s->cur_file_info; cur_file_info_internal_saved = s->cur_file_info_internal; + current_file_ok_saved = s->current_file_ok; @@ s->num_file = num_file_saved; s->pos_in_central_dir = pos_in_central_dir_saved; s->cur_file_info = cur_file_info_saved; s->cur_file_info_internal = cur_file_info_internal_saved; + s->current_file_ok = current_file_ok_saved; return err;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/core/minizip/unzip.c` around lines 1752 - 1776, The code that searches for a filename saves and restores s->num_file, s->pos_in_central_dir, s->cur_file_info and s->cur_file_info_internal but misses s->current_file_ok, so after a failed locate the validity flag remains clobbered; update the locate routine (the block that calls unzGoToFirstFile2/unzGoToNextFile2) to save the current value of s->current_file_ok into a local (e.g., current_file_ok_saved) before the search and restore s->current_file_ok to that saved value in the same restore section alongside num_file_saved, pos_in_central_dir_saved, cur_file_info_saved and cur_file_info_internal_saved so the previous "current" entry remains usable after a failed locate.
1912-1970:⚠️ Potential issue | 🟠 MajorReset stored-file seeks to the data start, not the local extra field.
Line 1964 rebuilds
pos_in_zipfilefromoffset_local_extrafield, so non-trivial seeks restart inside the local extra field. Line 1968 also subtracts an unsigned delta, which underflows on backward seeks.🛠️ Safe short-term fix
- uint64_t stream_pos_begin = 0; - uint64_t stream_pos_end = 0; uint64_t position = 0; - int is_within_buffer = 0; @@ - stream_pos_end = s->pfile_in_zip_read->pos_in_zipfile; - stream_pos_begin = stream_pos_end; - - if (stream_pos_begin > UNZ_BUFSIZE) - stream_pos_begin -= UNZ_BUFSIZE; - else - stream_pos_begin = 0; - - is_within_buffer = - (s->pfile_in_zip_read->stream.avail_in != 0) && - (s->pfile_in_zip_read->rest_read_compressed != 0 || s->cur_file_info.compressed_size < UNZ_BUFSIZE) && - (position >= stream_pos_begin && position < stream_pos_end); - - if (is_within_buffer) - { - s->pfile_in_zip_read->stream.next_in += position - s->pfile_in_zip_read->total_out_64; - s->pfile_in_zip_read->stream.avail_in = (uInt)(stream_pos_end - position); - } - else - { - s->pfile_in_zip_read->stream.avail_in = 0; - s->pfile_in_zip_read->stream.next_in = 0; - - s->pfile_in_zip_read->pos_in_zipfile = s->pfile_in_zip_read->offset_local_extrafield + position; - s->pfile_in_zip_read->rest_read_compressed = s->cur_file_info.compressed_size - position; - } - - s->pfile_in_zip_read->rest_read_uncompressed -= (position - s->pfile_in_zip_read->total_out_64); + s->pfile_in_zip_read->stream.avail_in = 0; + s->pfile_in_zip_read->stream.next_in = 0; + s->pfile_in_zip_read->pos_in_zipfile = + s->pfile_in_zip_read->offset_local_extrafield + + s->pfile_in_zip_read->size_local_extrafield + + position; + s->pfile_in_zip_read->rest_read_compressed = s->cur_file_info.compressed_size - position; + s->pfile_in_zip_read->rest_read_uncompressed = s->cur_file_info.uncompressed_size - position; s->pfile_in_zip_read->stream.total_out = (uint32_t)position; s->pfile_in_zip_read->total_out_64 = position;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/core/minizip/unzip.c` around lines 1912 - 1970, The code resets pos_in_zipfile to offset_local_extrafield which positions seeks inside the local extra field and also subtracts an unsigned delta causing underflow on backward seeks; change the seek base to the actual compressed-data start (compute from the local header start + local header size / filename+extra lengths instead of offset_local_extrafield) when setting s->pfile_in_zip_read->pos_in_zipfile, and compute the moved amount as a signed difference (int64_t delta = (int64_t)position - (int64_t)s->pfile_in_zip_read->total_out_64) then adjust s->pfile_in_zip_read->rest_read_uncompressed using that signed delta to avoid underflow; update references in unzSeek64 to use these corrected values (pos_in_zipfile and rest_read_uncompressed adjustments).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build_luajit_android.sh`:
- Around line 22-26: The current probe uses a compile-only check with "gcc -m32
-x c -c /dev/null -o /dev/null" which can pass even if the 32-bit linker or
libraries are missing; replace it with a full link test by invoking gcc -m32 to
compile-and-link a trivial C program (the same check that will be needed by
HOST_CC when building buildvm) and, if that link fails, install
gcc-multilib/g++-multilib; update the probe that references "gcc -m32 -x c -c
/dev/null -o /dev/null" and ensure it mirrors the actual invocation used for
HOST_CC so failures are detected early.
In `@Dockerfile.android`:
- Around line 11-14: Replace the open-ended git clone of the vcpkg repo with a
pinned checkout so the vcpkg tool is reproducible: after cloning
https://github.com/microsoft/vcpkg.git in the WORKDIR /opt step, perform a git
checkout of a specific commit hash (chosen to align with the vcpkg.json
builtin-baseline) before running ./bootstrap-vcpkg.sh; update the commit hash
when intentionally upgrading the tool. Reference: the RUN step that currently
does "git clone ... vcpkg.git && cd vcpkg && ./bootstrap-vcpkg.sh".
In `@src/framework/core/minizip/ioapi_mem.c`:
- Around line 68-79: fread_mem_func currently clamps reads against mem->size
(allocated capacity) which can return unwritten bytes after a grow/write cycle;
change the clamp to use the written-data boundary mem->limit instead (i.e.,
compute allowed = mem->limit - mem->cur_offset and reduce size = min(size,
allowed)), ensure you handle cases where cur_offset >= mem->limit by returning
0, and update mem->cur_offset only by the actual bytes copied.
- Around line 44-59: In fopen_mem_func, fail fast when the initial grow-buffer
allocation fails: after attempting to allocate mem->base (inside the mem->grow
branch) check whether malloc returned NULL, and if so free any allocated mem
state and return NULL instead of continuing; only set mem->size, mem->limit (and
later mem->cur_offset) and return mem when mem->base is non-NULL. Ensure the
NULL-check references mem->base and adjusts control flow so subsequent memcpy
calls cannot run on a NULL buffer.
In `@src/framework/core/minizip/ioapi.c`:
- Around line 100-117: file_build_ioposix currently returns NULL on allocation
failures but leaves the incoming FILE* open; change the error paths so the FILE*
is closed before returning. Specifically, in file_build_ioposix: if
malloc(sizeof(FILE_IOPOSIX)) returns NULL, call fclose(file) then return NULL;
if malloc for ioposix->filename fails, free(ioposix), call fclose(file), then
return NULL. Apply the same pattern to the other open-helper(s) that allocate
FILE_IOPOSIX and ioposix->filename so no FILE* is leaked.
---
Duplicate comments:
In `@setup_android_deps.sh`:
- Around line 55-65: The LuaJIT make invocation in setup_android_deps.sh is
missing the amalg build flag and TARGET_CFLAGS=-DLUAJIT_UNWIND_EXTERNAL so it
can produce a different libluajit-5.1.a than build_luajit_android.sh; update the
make call (the multi-line make block that sets HOST_CC, CROSS, STATIC_CC,
DYNAMIC_CC, TARGET_LD, TARGET_AR, TARGET_STRIP, TARGET_SYS, BUILDMODE) to
include amalg and TARGET_CFLAGS=-DLUAJIT_UNWIND_EXTERNAL so the build flags
match the documented Android builder.
- Around line 127-142: The zip invocation currently updates an existing archive
and fails if optional root files are missing; change the shell branch that runs
zip to recreate the archive and only add init.lua and otclientrc.lua if they
exist (mirror the Python fallback). Concretely, before calling zip in the shell
branch remove any existing android/app/src/main/assets/data.zip, then run zip -r
to add the directories data, mods, modules and append init.lua and
otclientrc.lua only when test -f returns true (e.g. use $(test -f init.lua &&
echo init.lua) and similar for otclientrc.lua). Update the block containing the
zip command (the shell branch that parallels the Python fallback) so behavior
matches the Python fallback.
In `@src/framework/core/minizip/ioapi.c`:
- Around line 158-180: The code in fopendisk64_file_func (and the similar
fopendisk_file_func) uses diskFilename immediately after malloc without checking
for allocation failure, which can dereference NULL; fix by testing the return of
malloc for NULL before calling strncpy/strncpy/snprintf and if allocation fails
return NULL (or appropriate error) and avoid calling
fopen64_file_func/fopen_file_func; ensure you only free diskFilename when
non-NULL and propagate the error path consistently in both functions.
In `@src/framework/core/minizip/unzip.c`:
- Around line 511-519: unzOpen2_64 currently leaves members of the bridge struct
zlib_filefunc64_32_def_fill (notably zopen32_file and zopendisk32_file)
uninitialized; zero-initialize the whole zlib_filefunc64_32_def_fill before
assigning zfile_func64 and the other members so any missing 64-bit callbacks
safely fall back to NULL, e.g. clear the struct (memset or initializer) and then
set zfile_func64 = *pzlib_filefunc_def and ztell32_file/zseek32_file = NULL to
ensure call_zopen64/call_zopendisk64 won't execute garbage.
- Around line 1487-1495: The variable `flags` (type compression_stream_flags) is
uninitialized when `s->pfile_in_zip_read->stream.avail_in != 0`; ensure `flags`
is given a defined value before calling `compression_stream_process` (e.g.
initialize it to 0 or explicitly set it in an else branch) so that the call to
`compression_stream_process(&s->pfile_in_zip_read->astream, flags)` always
receives a defined flag value; update the code around the `if
(s->pfile_in_zip_read->stream.avail_in == 0)` check (referencing `flags`,
`compression_stream_process`, `compression_stream_flags`, and
`s->pfile_in_zip_read->stream.avail_in`) to set a default/else value.
- Around line 1590-1598: The code in unzip.c (inside the
read-from-local-extra-field sequence that uses ZSEEK64/ZREAD64) fails to include
the archive's byte_before_the_zipfile and does not advance pos_local_extrafield,
causing wrong bytes to be read for self-extracting archives and chunked callers;
fix by seeking to filestream position (byte_before_the_zipfile +
offset_local_extrafield + pos_local_extrafield) before reading and after a
successful ZREAD64 increment s->pfile_in_zip_read->pos_local_extrafield by
read_now (or the actual bytes read) so subsequent reads continue from the
correct local-extra-field cursor, ensuring UNZ_ERRNO is still returned on
seek/read failures and the function returns (int)read_now on success.
- Around line 1752-1776: The code that searches for a filename saves and
restores s->num_file, s->pos_in_central_dir, s->cur_file_info and
s->cur_file_info_internal but misses s->current_file_ok, so after a failed
locate the validity flag remains clobbered; update the locate routine (the block
that calls unzGoToFirstFile2/unzGoToNextFile2) to save the current value of
s->current_file_ok into a local (e.g., current_file_ok_saved) before the search
and restore s->current_file_ok to that saved value in the same restore section
alongside num_file_saved, pos_in_central_dir_saved, cur_file_info_saved and
cur_file_info_internal_saved so the previous "current" entry remains usable
after a failed locate.
- Around line 1912-1970: The code resets pos_in_zipfile to
offset_local_extrafield which positions seeks inside the local extra field and
also subtracts an unsigned delta causing underflow on backward seeks; change the
seek base to the actual compressed-data start (compute from the local header
start + local header size / filename+extra lengths instead of
offset_local_extrafield) when setting s->pfile_in_zip_read->pos_in_zipfile, and
compute the moved amount as a signed difference (int64_t delta =
(int64_t)position - (int64_t)s->pfile_in_zip_read->total_out_64) then adjust
s->pfile_in_zip_read->rest_read_uncompressed using that signed delta to avoid
underflow; update references in unzSeek64 to use these corrected values
(pos_in_zipfile and rest_read_uncompressed adjustments).
In `@src/framework/core/unzipper.cpp`:
- Around line 85-92: The branch that rejects unsafe entry paths must not call
unzCloseCurrentFile() because unzOpenCurrentFile() hasn't been called yet;
remove the unzCloseCurrentFile(zipfile) call (or only call it when a file was
actually opened), and when advancing with unzGoToNextFile(zipfile) check its
return value and log or handle errors (use zipfile, i, global_info.number_entry)
instead of ignoring failures; keep the existing g_logger.warning(...) and use
entryPath/filename to report context.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 59d7e979-c734-4c87-a098-89b9d9ab626e
📒 Files selected for processing (23)
.dockerignore.gitignoreDockerfile.androidandroid/BUILDING.mdandroid/app/build.gradle.ktsandroid/app/src/main/AndroidManifest.xmlbuild_luajit_android.shsetup_android_deps.shsetup_android_wsl.shsrc/CMakeLists.txtsrc/framework/core/minizip/crypt.csrc/framework/core/minizip/crypt.hsrc/framework/core/minizip/ioapi.csrc/framework/core/minizip/ioapi.hsrc/framework/core/minizip/ioapi_mem.csrc/framework/core/minizip/ioapi_mem.hsrc/framework/core/minizip/unzip.csrc/framework/core/minizip/unzip.hsrc/framework/core/unzipper.cppsrc/framework/platform/androidmanager.cppsrc/framework/platform/androidwindow.cppsrc/framework/ui/uitextedit.cppsrc/main.cpp
✅ Files skipped from review due to trivial changes (6)
- src/framework/ui/uitextedit.cpp
- android/app/src/main/AndroidManifest.xml
- android/app/build.gradle.kts
- android/BUILDING.md
- setup_android_wsl.sh
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (4)
- src/framework/platform/androidmanager.cpp
- .dockerignore
- src/main.cpp
- src/framework/platform/androidwindow.cpp
Kizuno18
force-pushed
the
fix/android-black-screen
branch
2 times, most recently
from
0b4287e to
6c174e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/framework/platform/androidwindow.cpp (1)
264-279:⚠️ Potential issue | 🟠 MajorLong-press currently leaves the left button pressed.
Line 272 always marks
Fw::MouseLeftButtonas pressed, but Lines 274-279 can rewrite the release toFw::MouseRightButtonand clear that bit instead. After a long-press there is no matching left-button release, and there is still no matching right-button press event. Clear the button that was actually pressed, or synthesize a full right-click pair instead of remapping only the release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/platform/androidwindow.cpp` around lines 264 - 279, The code always sets mouseButton = Fw::MouseLeftButton and updates m_mouseButtonStates as if left was pressed, then later may remap the release to Fw::MouseRightButton leaving the left bit stuck; fix by recording the actually-pressed button at touch-down (e.g. store an originalPressed variable or set an initialPressed value before potential remap) and use that stored value when clearing m_mouseButtonStates in the g_dispatcher callback (or alternatively, when you remap a release to Fw::MouseRightButton synthesize a matching right-button press event before scheduling the release), updating m_inputEvent.mouseButton only for the delivered event while always clearing the bit for the button that was originally set (references: Fw::MouseLeftButton, Fw::MouseRightButton, m_inputEvent, m_mouseButtonStates, m_lastPress, TOUCH_UP, m_isDragging, g_dispatcher).
♻️ Duplicate comments (10)
build_luajit_android.sh (1)
22-26:⚠️ Potential issue | 🟠 MajorUse a link test for the
gcc -m32probe.This check only compiles.
HOST_CC="gcc -m32"later has to linkbuildvm, so a host missing 32-bit linker/libs can pass here and still fail insidemake.Suggested fix
-# Verify gcc -m32 works (needed for 32-bit targets) -if ! gcc -m32 -x c -c /dev/null -o /dev/null 2>/dev/null; then +# Verify gcc -m32 can compile and link host tools (needed for buildvm) +tmp_bin="$(mktemp)" +if ! printf 'int main(void){return 0;}\n' | gcc -m32 -x c - -o "$tmp_bin" 2>/dev/null; then echo "Installing gcc-multilib for 32-bit cross-compilation..." sudo apt-get install -y gcc-multilib g++-multilib fi +rm -f "$tmp_bin"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build_luajit_android.sh` around lines 22 - 26, Current probe only compiles with gcc -m32 and can miss missing 32-bit linker/libs; replace the probe with a full link test (compile+link a trivial C program) to mirror how HOST_CC="gcc -m32" will be used when building buildvm so failures are caught early; update the check that currently runs `gcc -m32 -x c -c /dev/null` to instead attempt compiling and linking a tiny main() and only install gcc-multilib/g++-multilib if that link step fails.src/framework/core/unzipper.cpp (1)
87-91:⚠️ Potential issue | 🟡 MinorHandle
unzGoToNextFile()failures in the unsafe-entry branch.If the cursor advance on Line 89 fails, the loop still
continues as if the archive state moved, so extraction can silently proceed on stale MiniZip state. Check the return code here the same way you do in the normal iteration path.Suggested fix
if (entryPath.is_absolute() || entryPath.string().starts_with("..")) { g_logger.warning("skipping zip entry with unsafe path: {}", filename); - if ( ( i+1 ) < global_info.number_entry ) - unzGoToNextFile( zipfile ); + if ((i + 1) < global_info.number_entry && + unzGoToNextFile(zipfile) != UNZ_OK) { + unzClose(zipfile); + g_logger.fatal("could not read next file"); + } continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/core/unzipper.cpp` around lines 87 - 91, The unsafe-entry branch currently calls unzGoToNextFile(zipfile) but ignores its return value, which can leave MiniZip in a stale state; update the block around entryPath.is_absolute() / entryPath.string().starts_with("..") to capture the return code from unzGoToNextFile(zipfile), check for an error (non-UNZ_OK/negative), log an error via g_logger.error with context (filename and return code) and break/return appropriately (matching the error handling used in the normal iteration path that checks return values against global_info.number_entry), so the loop does not continue on failure and the archive state stays consistent.src/framework/core/minizip/unzip.c (5)
513-519:⚠️ Potential issue | 🟠 MajorZero-initialize the bridge struct in
unzOpen2_64().
zopen32_fileandzopendisk32_filestay indeterminate here. If a caller omits one of the 64-bit open callbacks, the fallback path jumps through garbage.🛠️ Suggested fix
if (pzlib_filefunc_def != NULL) { - zlib_filefunc64_32_def zlib_filefunc64_32_def_fill; + zlib_filefunc64_32_def zlib_filefunc64_32_def_fill = {0}; zlib_filefunc64_32_def_fill.zfile_func64 = *pzlib_filefunc_def; - zlib_filefunc64_32_def_fill.ztell32_file = NULL; - zlib_filefunc64_32_def_fill.zseek32_file = NULL; return unzOpenInternal(path, &zlib_filefunc64_32_def_fill); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/core/minizip/unzip.c` around lines 513 - 519, The bridge struct zlib_filefunc64_32_def_fill used in unzOpen2_64() is not zero-initialized so fields like zopen32_file and zopendisk32_file can be indeterminate; fix by zero-initializing zlib_filefunc64_32_def_fill (or explicitly set zopen32_file and zopendisk32_file to NULL) before assigning zfile_func64, ztell32_file and zseek32_file, ensuring any omitted 64-bit callbacks safely fall back instead of using garbage.
1487-1495:⚠️ Potential issue | 🟠 MajorInitialize
flagsbeforecompression_stream_process().When
avail_in != 0, Line 1495 usesflagsuninitialized. That is UB on Apple builds.🛠️ Suggested fix
compression_status status; - compression_stream_flags flags; + compression_stream_flags flags = 0; if (s->pfile_in_zip_read->stream.avail_in == 0) { flags = COMPRESSION_STREAM_FINALIZE; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/core/minizip/unzip.c` around lines 1487 - 1495, The variable flags used in the call to compression_stream_process(&s->pfile_in_zip_read->astream, flags) can be uninitialized when s->pfile_in_zip_read->stream.avail_in != 0; set a sensible default (e.g., COMPRESSION_STREAM_FINALIZE or COMPRESSION_STREAM_NOT_FINALIZED depending on intended behavior) when declaring compression_stream_flags flags before the if, or explicitly assign flags = COMPRESSION_STREAM_NOT_FINALIZED in the else branch so flags is always initialized prior to calling compression_stream_process; update the code near the use of compression_status and compression_stream_flags in unzip.c to ensure flags is always assigned.
1590-1598:⚠️ Potential issue | 🟠 MajorAdvance the local extra-field cursor after each read.
The seek ignores
byte_before_the_zipfile, andpos_local_extrafieldnever advances, so chunked callers on prepended/SFX archives reread the wrong bytes.🛠️ Suggested fix
if (ZSEEK64(s->pfile_in_zip_read->z_filefunc, s->pfile_in_zip_read->filestream, - s->pfile_in_zip_read->offset_local_extrafield + s->pfile_in_zip_read->pos_local_extrafield, + s->pfile_in_zip_read->offset_local_extrafield + + s->pfile_in_zip_read->byte_before_the_zipfile + + s->pfile_in_zip_read->pos_local_extrafield, ZLIB_FILEFUNC_SEEK_SET) != 0) return UNZ_ERRNO; if (ZREAD64(s->pfile_in_zip_read->z_filefunc, s->pfile_in_zip_read->filestream, buf, read_now) != read_now) return UNZ_ERRNO; + s->pfile_in_zip_read->pos_local_extrafield += read_now; return (int)read_now;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/core/minizip/unzip.c` around lines 1590 - 1598, The seek/read block for reading local extra-field must include the archive prepend offset and advance the local extra-field cursor: when calling ZSEEK64 use s->pfile_in_zip_read->byte_before_the_zipfile + s->pfile_in_zip_read->offset_local_extrafield + s->pfile_in_zip_read->pos_local_extrafield (not just offset+pos), and after a successful ZREAD64 increment s->pfile_in_zip_read->pos_local_extrafield by read_now so subsequent chunked reads continue from the correct position; adjust the code around ZSEEK64, ZREAD64 and s->pfile_in_zip_read->pos_local_extrafield accordingly.
1737-1776:⚠️ Potential issue | 🟠 MajorRestore
current_file_okafter an unsuccessful locate.A miss restores the cursor fields but leaves
current_file_okcleared, so the previously current entry becomes unusable.🛠️ Suggested fix
unz_file_info64 cur_file_info_saved; unz_file_info64_internal cur_file_info_internal_saved; uint64_t num_file_saved = 0; uint64_t pos_in_central_dir_saved = 0; + uint64_t current_file_ok_saved = 0; char current_filename[UNZ_MAXFILENAMEINZIP+1]; int err = UNZ_OK; @@ num_file_saved = s->num_file; pos_in_central_dir_saved = s->pos_in_central_dir; cur_file_info_saved = s->cur_file_info; cur_file_info_internal_saved = s->cur_file_info_internal; + current_file_ok_saved = s->current_file_ok; @@ s->num_file = num_file_saved; s->pos_in_central_dir = pos_in_central_dir_saved; s->cur_file_info = cur_file_info_saved; s->cur_file_info_internal = cur_file_info_internal_saved; + s->current_file_ok = current_file_ok_saved; return err;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/core/minizip/unzip.c` around lines 1737 - 1776, Save the current_file_ok state at the start (e.g. bool current_file_ok_saved = s->current_file_ok) alongside num_file_saved/pos_in_central_dir_saved/cur_file_info_saved/cur_file_info_internal_saved, and after the failed search restore it (s->current_file_ok = current_file_ok_saved) before returning err so that the prior current entry remains usable; locate this change around the unzGoToFirstFile2/unzGoToNextFile2 loop in unzip.c where the saved state is restored.
1915-1970:⚠️ Potential issue | 🟠 MajorRebuild stored-entry seeks from the file-data start.
The reset path derives
pos_in_zipfilefromoffset_local_extrafield, which points before the file data, and therest_read_uncompressed -= (position - total_out_64)math wraps on backward seeks.🛠️ Safe short-term fix
- uint64_t stream_pos_begin = 0; - uint64_t stream_pos_end = 0; uint64_t position = 0; - int is_within_buffer = 0; @@ - stream_pos_end = s->pfile_in_zip_read->pos_in_zipfile; - stream_pos_begin = stream_pos_end; - - if (stream_pos_begin > UNZ_BUFSIZE) - stream_pos_begin -= UNZ_BUFSIZE; - else - stream_pos_begin = 0; - - is_within_buffer = - (s->pfile_in_zip_read->stream.avail_in != 0) && - (s->pfile_in_zip_read->rest_read_compressed != 0 || s->cur_file_info.compressed_size < UNZ_BUFSIZE) && - (position >= stream_pos_begin && position < stream_pos_end); - - if (is_within_buffer) - { - s->pfile_in_zip_read->stream.next_in += position - s->pfile_in_zip_read->total_out_64; - s->pfile_in_zip_read->stream.avail_in = (uInt)(stream_pos_end - position); - } - else - { - s->pfile_in_zip_read->stream.avail_in = 0; - s->pfile_in_zip_read->stream.next_in = 0; - - s->pfile_in_zip_read->pos_in_zipfile = s->pfile_in_zip_read->offset_local_extrafield + position; - s->pfile_in_zip_read->rest_read_compressed = s->cur_file_info.compressed_size - position; - } - - s->pfile_in_zip_read->rest_read_uncompressed -= (position - s->pfile_in_zip_read->total_out_64); + s->pfile_in_zip_read->stream.avail_in = 0; + s->pfile_in_zip_read->stream.next_in = 0; + s->pfile_in_zip_read->pos_in_zipfile = + s->pfile_in_zip_read->offset_local_extrafield + + s->pfile_in_zip_read->size_local_extrafield + + position; + s->pfile_in_zip_read->rest_read_compressed = s->cur_file_info.compressed_size - position; + s->pfile_in_zip_read->rest_read_uncompressed = s->cur_file_info.uncompressed_size - position;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/core/minizip/unzip.c` around lines 1915 - 1970, The seek reset currently uses offset_local_extrafield directly (which points before the actual file data) and subtracts unsigned values causing wrap on backward seeks; compute the actual file-data start first (e.g. file_data_start = s->pfile_in_zip_read->offset_local_extrafield + size of the local header/filename/extra fields — use the existing fields that record filename length and extra-field length such as size_filename / size_extra_field on pfile_in_zip_read), set s->pfile_in_zip_read->pos_in_zipfile = file_data_start + position, and update rest_read_uncompressed using a guarded subtraction (only subtract when position >= s->pfile_in_zip_read->total_out_64 or compute the delta as a signed value) so total_out_64, stream.total_out and rest_read_uncompressed remain consistent for forward and backward seeks.src/framework/core/minizip/ioapi.c (2)
158-180:⚠️ Potential issue | 🟠 MajorGuard
diskFilenamebefore copying into it.Both spanning helpers dereference
diskFilenameimmediately aftermalloc(). If allocation fails, they crash instead of returning an open error.🛠️ Suggested fix
ioposix = (FILE_IOPOSIX*)stream; diskFilename = (char*)malloc(ioposix->filenameLength * sizeof(char)); + if (diskFilename == NULL) + return NULL; strncpy(diskFilename, (const char*)ioposix->filename, ioposix->filenameLength);Apply the same guard in both
fopendisk64_file_func()andfopendisk_file_func().Also applies to: 183-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/core/minizip/ioapi.c` around lines 158 - 180, The function fopendisk64_file_func currently mallocs diskFilename then immediately uses it; add a NULL check after the malloc to guard against allocation failure: if diskFilename is NULL, do not dereference it—return NULL (or the appropriate open error) immediately. Apply the same fix pattern to fopendisk_file_func; keep the rest of the logic (strncpy from FILE_IOPOSIX->filename, the filename tweak loop, and the call to fopen64_file_func) unchanged and ensure free(diskFilename) only runs when allocation succeeded.
100-117:⚠️ Potential issue | 🟡 MinorClose the
FILE*whenfile_build_ioposix()fails.
fopen_file_func()/fopen64_file_func()have already opened the file before calling this helper, so either allocation failure here leaks the descriptor.🛠️ Suggested fix
static voidpf file_build_ioposix(FILE *file, const char *filename) { FILE_IOPOSIX *ioposix = NULL; if (file == NULL) return NULL; /* Fix: legacy minizip missing null checks after malloc (upstream archived) */ ioposix = (FILE_IOPOSIX*)malloc(sizeof(FILE_IOPOSIX)); - if (ioposix == NULL) + if (ioposix == NULL) { + fclose(file); return NULL; + } ioposix->file = file; ioposix->filenameLength = (int)strlen(filename) + 1; ioposix->filename = (char*)malloc(ioposix->filenameLength * sizeof(char)); if (ioposix->filename == NULL) { + fclose(file); free(ioposix); return NULL; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/core/minizip/ioapi.c` around lines 100 - 117, When file_build_ioposix fails after fopen_file_func/fopen64_file_func has already opened the FILE*, you must close that FILE to avoid leaking the descriptor: call fclose(file) before returning NULL on allocation failures (after malloc(sizeof(FILE_IOPOSIX)) fails and after ioposix->filename allocation fails). Update file_build_ioposix to fclose(file) in those failure paths (keep freeing ioposix where present) and ensure no further use of file after it is closed; leave the early return when file == NULL unchanged.setup_android_deps.sh (1)
137-150:⚠️ Potential issue | 🟠 MajorMake the
zipbranch mirror the Python fallback.
zip -r ... init.lua otclientrc.luaaborts when either optional file is absent, and because it updates an existing archive in place it can keep stale entries from previous runs.🛠️ Suggested fix
if command -v zip &>/dev/null; then - zip -r android/app/src/main/assets/data.zip data mods modules init.lua otclientrc.lua + rm -f android/app/src/main/assets/data.zip + inputs=(data mods modules) + [[ -f init.lua ]] && inputs+=(init.lua) + [[ -f otclientrc.lua ]] && inputs+=(otclientrc.lua) + zip -r android/app/src/main/assets/data.zip "${inputs[@]}" elif command -v python3 &>/dev/null; thenThis reproduces the current
zipbehavior; expect a non-zero exit status when the optional files are absent.#!/bin/bash set -euo pipefail if ! command -v zip >/dev/null; then echo "zip is not installed in the sandbox; verify this on a machine with zip available." exit 0 fi tmpdir="$(mktemp -d)" trap 'rm -rf "$tmpdir"' EXIT mkdir -p "$tmpdir"/data "$tmpdir"/mods "$tmpdir"/modules ( cd "$tmpdir" set +e zip -r data.zip data mods modules init.lua otclientrc.lua >/tmp/setup_android_deps_zip.log 2>&1 status=$? set -e printf 'zip_exit_status=%s\n' "$status" cat /tmp/setup_android_deps_zip.log )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup_android_deps.sh` around lines 137 - 150, The zip branch currently runs zip -r android/app/src/main/assets/data.zip data mods modules init.lua otclientrc.lua which fails if the optional files are absent and updates the archive in-place leaving stale entries; change it to build a fresh archive in a temporary file or use zip -FS and only pass optional files if they exist. Concretely, create a tmp file (mktemp), run zip -r "$tmpzip" data mods modules and then conditionally run zip -u or include init.lua and otclientrc.lua only if [ -f init.lua ] / [ -f otclientrc.lua ], then move the tmp archive to android/app/src/main/assets/data.zip (or use zip -FS to remove stale entries) and clean up the temp file so the behavior mirrors the python fallback and never errors when optional files are missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/app/build.gradle.kts`:
- Around line 36-42: The release signing config currently created by
signingConfigs.create("release") points at the debug keystore
(~/.android/debug.keystore) and should be removed or replaced; update the
signingConfigs and buildTypes.release to either (a) load a real release keystore
via Gradle properties or environment variables (e.g., keystoreFile,
keystorePassword, keyAlias, keyPassword) and reference those properties in the
create("release") block, or (b) remove the custom release signing config and use
the existing debug signingConfig for development builds; ensure
buildTypes.release no longer assigns the debug keystore values and instead
references the secure properties or omits the assignment so CI/Docker builds
won’t expect the debug keystore.
In `@src/framework/core/minizip/unzip.c`:
- Around line 831-835: The code checks file_info.disk_num_start against
UINT32_MAX before reading the Zip64 extra-field, but the central-directory field
was read as 16 bits earlier so the Zip64 sentinel is 0xFFFF; change the
conditional to test against UINT16_MAX (0xFFFF) instead of UINT32_MAX so that
unzReadUInt32(&s->z_filefunc, s->filestream_with_CD, &file_info.disk_num_start)
is invoked when the 16-bit placeholder indicates a Zip64 disk start number; keep
the existing error handling (compare return to UNZ_OK and set err = UNZ_ERRNO on
failure).
---
Outside diff comments:
In `@src/framework/platform/androidwindow.cpp`:
- Around line 264-279: The code always sets mouseButton = Fw::MouseLeftButton
and updates m_mouseButtonStates as if left was pressed, then later may remap the
release to Fw::MouseRightButton leaving the left bit stuck; fix by recording the
actually-pressed button at touch-down (e.g. store an originalPressed variable or
set an initialPressed value before potential remap) and use that stored value
when clearing m_mouseButtonStates in the g_dispatcher callback (or
alternatively, when you remap a release to Fw::MouseRightButton synthesize a
matching right-button press event before scheduling the release), updating
m_inputEvent.mouseButton only for the delivered event while always clearing the
bit for the button that was originally set (references: Fw::MouseLeftButton,
Fw::MouseRightButton, m_inputEvent, m_mouseButtonStates, m_lastPress, TOUCH_UP,
m_isDragging, g_dispatcher).
---
Duplicate comments:
In `@build_luajit_android.sh`:
- Around line 22-26: Current probe only compiles with gcc -m32 and can miss
missing 32-bit linker/libs; replace the probe with a full link test
(compile+link a trivial C program) to mirror how HOST_CC="gcc -m32" will be used
when building buildvm so failures are caught early; update the check that
currently runs `gcc -m32 -x c -c /dev/null` to instead attempt compiling and
linking a tiny main() and only install gcc-multilib/g++-multilib if that link
step fails.
In `@setup_android_deps.sh`:
- Around line 137-150: The zip branch currently runs zip -r
android/app/src/main/assets/data.zip data mods modules init.lua otclientrc.lua
which fails if the optional files are absent and updates the archive in-place
leaving stale entries; change it to build a fresh archive in a temporary file or
use zip -FS and only pass optional files if they exist. Concretely, create a tmp
file (mktemp), run zip -r "$tmpzip" data mods modules and then conditionally run
zip -u or include init.lua and otclientrc.lua only if [ -f init.lua ] / [ -f
otclientrc.lua ], then move the tmp archive to
android/app/src/main/assets/data.zip (or use zip -FS to remove stale entries)
and clean up the temp file so the behavior mirrors the python fallback and never
errors when optional files are missing.
In `@src/framework/core/minizip/ioapi.c`:
- Around line 158-180: The function fopendisk64_file_func currently mallocs
diskFilename then immediately uses it; add a NULL check after the malloc to
guard against allocation failure: if diskFilename is NULL, do not dereference
it—return NULL (or the appropriate open error) immediately. Apply the same fix
pattern to fopendisk_file_func; keep the rest of the logic (strncpy from
FILE_IOPOSIX->filename, the filename tweak loop, and the call to
fopen64_file_func) unchanged and ensure free(diskFilename) only runs when
allocation succeeded.
- Around line 100-117: When file_build_ioposix fails after
fopen_file_func/fopen64_file_func has already opened the FILE*, you must close
that FILE to avoid leaking the descriptor: call fclose(file) before returning
NULL on allocation failures (after malloc(sizeof(FILE_IOPOSIX)) fails and after
ioposix->filename allocation fails). Update file_build_ioposix to fclose(file)
in those failure paths (keep freeing ioposix where present) and ensure no
further use of file after it is closed; leave the early return when file == NULL
unchanged.
In `@src/framework/core/minizip/unzip.c`:
- Around line 513-519: The bridge struct zlib_filefunc64_32_def_fill used in
unzOpen2_64() is not zero-initialized so fields like zopen32_file and
zopendisk32_file can be indeterminate; fix by zero-initializing
zlib_filefunc64_32_def_fill (or explicitly set zopen32_file and zopendisk32_file
to NULL) before assigning zfile_func64, ztell32_file and zseek32_file, ensuring
any omitted 64-bit callbacks safely fall back instead of using garbage.
- Around line 1487-1495: The variable flags used in the call to
compression_stream_process(&s->pfile_in_zip_read->astream, flags) can be
uninitialized when s->pfile_in_zip_read->stream.avail_in != 0; set a sensible
default (e.g., COMPRESSION_STREAM_FINALIZE or COMPRESSION_STREAM_NOT_FINALIZED
depending on intended behavior) when declaring compression_stream_flags flags
before the if, or explicitly assign flags = COMPRESSION_STREAM_NOT_FINALIZED in
the else branch so flags is always initialized prior to calling
compression_stream_process; update the code near the use of compression_status
and compression_stream_flags in unzip.c to ensure flags is always assigned.
- Around line 1590-1598: The seek/read block for reading local extra-field must
include the archive prepend offset and advance the local extra-field cursor:
when calling ZSEEK64 use s->pfile_in_zip_read->byte_before_the_zipfile +
s->pfile_in_zip_read->offset_local_extrafield +
s->pfile_in_zip_read->pos_local_extrafield (not just offset+pos), and after a
successful ZREAD64 increment s->pfile_in_zip_read->pos_local_extrafield by
read_now so subsequent chunked reads continue from the correct position; adjust
the code around ZSEEK64, ZREAD64 and s->pfile_in_zip_read->pos_local_extrafield
accordingly.
- Around line 1737-1776: Save the current_file_ok state at the start (e.g. bool
current_file_ok_saved = s->current_file_ok) alongside
num_file_saved/pos_in_central_dir_saved/cur_file_info_saved/cur_file_info_internal_saved,
and after the failed search restore it (s->current_file_ok =
current_file_ok_saved) before returning err so that the prior current entry
remains usable; locate this change around the unzGoToFirstFile2/unzGoToNextFile2
loop in unzip.c where the saved state is restored.
- Around line 1915-1970: The seek reset currently uses offset_local_extrafield
directly (which points before the actual file data) and subtracts unsigned
values causing wrap on backward seeks; compute the actual file-data start first
(e.g. file_data_start = s->pfile_in_zip_read->offset_local_extrafield + size of
the local header/filename/extra fields — use the existing fields that record
filename length and extra-field length such as size_filename / size_extra_field
on pfile_in_zip_read), set s->pfile_in_zip_read->pos_in_zipfile =
file_data_start + position, and update rest_read_uncompressed using a guarded
subtraction (only subtract when position >= s->pfile_in_zip_read->total_out_64
or compute the delta as a signed value) so total_out_64, stream.total_out and
rest_read_uncompressed remain consistent for forward and backward seeks.
In `@src/framework/core/unzipper.cpp`:
- Around line 87-91: The unsafe-entry branch currently calls
unzGoToNextFile(zipfile) but ignores its return value, which can leave MiniZip
in a stale state; update the block around entryPath.is_absolute() /
entryPath.string().starts_with("..") to capture the return code from
unzGoToNextFile(zipfile), check for an error (non-UNZ_OK/negative), log an error
via g_logger.error with context (filename and return code) and break/return
appropriately (matching the error handling used in the normal iteration path
that checks return values against global_info.number_entry), so the loop does
not continue on failure and the archive state stays consistent.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4cc04b4f-d77f-43ff-9a40-879f6f2c8f86
📒 Files selected for processing (23)
.dockerignore.gitignoreDockerfile.androidandroid/BUILDING.mdandroid/app/build.gradle.ktsandroid/app/src/main/AndroidManifest.xmlbuild_luajit_android.shsetup_android_deps.shsetup_android_wsl.shsrc/CMakeLists.txtsrc/framework/core/minizip/crypt.csrc/framework/core/minizip/crypt.hsrc/framework/core/minizip/ioapi.csrc/framework/core/minizip/ioapi.hsrc/framework/core/minizip/ioapi_mem.csrc/framework/core/minizip/ioapi_mem.hsrc/framework/core/minizip/unzip.csrc/framework/core/minizip/unzip.hsrc/framework/core/unzipper.cppsrc/framework/platform/androidmanager.cppsrc/framework/platform/androidwindow.cppsrc/framework/ui/uitextedit.cppsrc/main.cpp
✅ Files skipped from review due to trivial changes (7)
- .gitignore
- android/app/src/main/AndroidManifest.xml
- .dockerignore
- src/framework/ui/uitextedit.cpp
- setup_android_wsl.sh
- src/framework/core/minizip/crypt.h
- android/BUILDING.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/framework/platform/androidmanager.cpp
- src/main.cpp
Kizuno18
force-pushed
the
fix/android-black-screen
branch
from
6c174e6 to
097510c
Compare
…aJIT Fixes opentibiabr#1642 Black screen fixes: - Add null check for AAssetManager_open (data.zip missing crash) - Fix unzipper: create_directory -> create_directories for nested paths - Fix APP_CMD_LOW_MEMORY permanently destroying EGL surface (no recovery) - Fix keyboard auto-opening on programmatic focus (only on user tap) Build modernization: - Replace pre-compiled Google Drive archive with self-contained build - LuaJIT cross-compiled via build_luajit_android.sh for all 4 ABIs (arm64-v8a, armeabi-v7a, x86_64, x86) - All other deps (OpenAL, Freetype, protobuf, etc.) via vcpkg - Vendored minizip source (nmoinvaz fork) - Separate Android GLES3 from WASM GLES2 LuaJIT cross-compilation requires HOST_CC="gcc -m32" for 32-bit targets and -DLUAJIT_UNWIND_EXTERNAL for Android NDK compatibility. See android/BUILDING.md for full details and references to LuaJIT issues opentibiabr#440, opentibiabr#477, opentibiabr#664. Build infrastructure: - build_luajit_android.sh: cross-compile LuaJIT for all ABIs - setup_android_deps.sh: complete pre-build automation - Dockerfile.android: reproducible Docker build - android/BUILDING.md: step-by-step build guide with troubleshooting - ALOGD debug logging in main.cpp - Release signing config in build.gradle.kts
Kizuno18
force-pushed
the
fix/android-black-screen
branch
from
097510c to
29c1e12
Compare
✅ Actions performedReview triggered.
|
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.
Summary
Fixes the Android black screen issue (#1642) reported by @tomasz1337 and confirmed by @maianame. Modernizes the Android build pipeline by replacing the pre-compiled Google Drive library archive with a self-contained build system.
Fixes #1642 — Android Black Screen
The black screen was caused by multiple cascading issues:
data.zipnull check —unZipAssetData()dereferences nullAAsset*whendata.zipis missing from APK assets, crashing silentlyunzipper.cppnested directory failure —create_directory()changed tocreate_directories()+ parent dir creation before file writesAPP_CMD_LOW_MEMORYdestroying EGL surface permanently — system never re-sendsAPP_CMD_INIT_WINDOWafter low-memory, app enters permanent blocking wait with no recovery pathFw::MouseFocusReason(user tap), not on code-drivenfocus()calls (e.g.entergame.lua:582)Build modernization
Replaces the pre-compiled 2022 Google Drive archive with a self-contained build:
build_luajit_android.shfor all 4 ABIs using the official cross-compilation approachsrc/framework/core/minizip/LuaJIT cross-compilation
The vcpkg LuaJIT port cannot cross-compile for Android from an x64 host (
buildvm-32feature fails with "Unsupported target architecture"). We solved this by cross-compiling LuaJIT separately using the official Makefile approach with specific flags discovered through debugging:HOST_CC="gcc -m32"is mandatory (gcc-multilibrequired). Without it,buildvmcomputes 64-bit struct offsets for a 32-bit target, causing silent Lua stack corruption that manifests ashasIndex(-n)assertion failures at runtime.-DLUAJIT_UNWIND_EXTERNAL: Required for Android NDK because clang generates unwind tables (-funwind-tables). Without this flag, LuaJIT uses its internal frame unwinder which conflicts with C++ exception handling in the NDK runtime, corrupting the Lua stack when C++ code interacts with LuaJIT across exception boundaries. The Makefile auto-detects this but can silently fail if cross-compiler paths are misconfigured.TARGET_SYS=Android(LuaJIT #440): causes memory corruption inlua_openlibs. UseTARGET_SYS=Linuxor omit (auto-detected on Linux hosts).amalgtarget: Single-file build for better optimization, matching the vcpkg approach.Full build guide with troubleshooting in
android/BUILDING.md. References: LuaJIT #477, #664, #440, vcpkg portfile.ALOGD debug logging
Added
__android_log_printcalls throughoutmain.cpp(platform init, unzip, resource loading, work dir discovery, framework init, client init, init.lua execution). These areANDROID_LOG_DEBUGlevel — only visible viaadb logcat -s "OTClientMobile:*"and have no performance impact on release builds. This was the key missing piece for debugging #1642: without these logs, there was no way to determine where the initialization was failing.Multi-ABI support
gccgcc -m32gccgcc -m32Build instructions
See
android/BUILDING.md. Quick start:Test plan
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Other