Conversation
…ge-size builds Fixes #562 — crash on Samsung S21 Plus (Android 14) with 3.1.1-16k-min: dlopen failed: can't enable GNU RELRO protection for libCGEExt.so: Out of memory Root cause: target_link_options(CGE PUBLIC ... -Wl,-z,relro,-z,now) propagated these flags to CGEExt via PUBLIC linkage. With 16KB page alignment enabled, the linker pads PT_GNU_RELRO to 16KB boundaries, making MemSiz (~10KB) far exceed the actual RW LOAD segment size (~1.5KB). The Android dynamic linker then calls mprotect() over a region larger than what was mapped, which fails with ENOMEM on devices under memory pressure. Android (API 23+) enforces full RELRO by default; these flags are redundant. Changes: - CMakeLists.txt: remove -Wl,-z,relro,-z,now; change scope to PRIVATE so the remaining -fPIE -fPIC -pie flags do not propagate unnecessarily. The -Wl,-z,max-page-size=16384 block retains PUBLIC scope so CGEExt inherits 16KB alignment automatically via CMake interface propagation. - cgeDemo/build.gradle: add disableVideoModule() helper and make the testDemoWithGradleLibs dependency string dynamic (supports all four AAR variants: default, -16k, -min, -16k-min)."
|
Caution Review failedThe pull request is closed. WalkthroughUpdates Gradle to select a dynamic gpuimage-plus variant via new helper flags and bumps the version to 3.1.2; modifies CMake to enable -O3 for non-Debug builds and removes explicit GNU RELRO linker flags; adds a CHANGE.md entry describing the RELRO fix. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Pull request overview
Removes problematic RELRO linker flags from the native build to prevent UnsatisfiedLinkError crashes on 16KB-page-size builds, and improves the demo app’s ability to test published AAR variants by selecting the correct artifact suffix based on local.properties flags.
Changes:
- Remove
-Wl,-z,relro,-z,nowfromCGElink options (and avoid propagating unrelated link flags to dependents). - Keep 16KB page-size linker flags propagated so
CGEExtinherits alignment settings when enabled. - Update
cgeDemoto dynamically pick the correctorg.wysaid:gpuimage-plusAAR variant suffix (-16k,-min,-16k-min) during integration testing.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
library/src/main/jni/CMakeLists.txt |
Adjusts native linker options to avoid RELRO-related runtime load failures on 16KB page-size builds while preserving 16KB alignment propagation. |
cgeDemo/build.gradle |
Adds a helper flag reader and makes the demo’s Maven dependency variant selection match local.properties build flags. |
library/src/main/jni/CMakeLists.txt
Outdated
| target_compile_options(CGE PRIVATE $<$<NOT:$<CONFIG:Debug>>:-O3>) | ||
|
|
||
| target_link_options(CGE PUBLIC -fPIE -fPIC -pie -Wl,-z,relro,-z,now) | ||
| # Note: -Wl,-z,relro,-z,now is intentionally omitted here， fixing issue: <https://github.com/wysaid/android-gpuimage-plus/issues/562> |
There was a problem hiding this comment.
The comment uses a full-width Chinese comma ("，") in an otherwise English sentence, which can be hard to search for and looks like a typo. Consider replacing it with a normal comma and tightening the wording (e.g., "...omitted here, fixing issue #562: ...").
Suggested change
| # Note: -Wl,-z,relro,-z,now is intentionally omitted here， fixing issue: <https://github.com/wysaid/android-gpuimage-plus/issues/562> | |
| # Note: -Wl,-z,relro,-z,now is intentionally omitted here, fixing issue #562: <https://github.com/wysaid/android-gpuimage-plus/issues/562> |
library/src/main/jni/CMakeLists.txt
Outdated
|
|
||
| target_link_options(CGE PUBLIC -fPIE -fPIC -pie -Wl,-z,relro,-z,now) | ||
| # Note: -Wl,-z,relro,-z,now is intentionally omitted here， fixing issue: <https://github.com/wysaid/android-gpuimage-plus/issues/562> | ||
| target_link_options(CGE PRIVATE -fPIE -fPIC -pie) |
There was a problem hiding this comment.
-pie is a linker flag for building position-independent executables; applying it via target_link_options on a library target is unusual and can cause toolchain-specific warnings or errors (especially when the target is SHARED). Consider removing -pie here and, if you still need PIC, prefer POSITION_INDEPENDENT_CODE or target_compile_options(... -fPIC) instead of passing compile flags as link options.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
library/src/main/jni/CMakeLists.txt (2)
83-85: Remove the redundant Chinese comment now that an English equivalent is added.Lines 83 and 84 say the same thing in two languages. The pre-existing Chinese comment can be dropped.
♻️ Proposed cleanup
-# 非 Debug 模式下添加 -O3 # Add -O3 in non-Debug mode target_compile_options(CGE PRIVATE $<$<NOT:$<CONFIG:Debug>>:-O3>)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@library/src/main/jni/CMakeLists.txt` around lines 83 - 85, Remove the redundant Chinese comment above the target_compile_options line: delete the comment "# 非 Debug 模式下添加 -O3" and keep the English comment "# Add -O3 in non-Debug mode" that precedes the target_compile_options(CGE PRIVATE $<$<NOT:$<CONFIG:Debug>>:-O3>) statement so only the English comment remains.
88-88:-pie,-fPIE, and-fPICare inappropriate/redundant link options for aSHAREDlibrary.
-pieis an executable-only linker flag;lldsilently ignores it on shared objects but it is semantically incorrect.-fPICis a compile-time code-generation flag and is a no-op when forwarded to the linker.-fPIEat link time targets PIE executables, not shared libraries.- CMake automatically adds
-fPICtoSHAREDtargets; all three flags are effectively dead weight.Since the scope is already being narrowed to
PRIVATEin this PR, this is a good opportunity to remove all three:♻️ Proposed cleanup
-# Note: -Wl,-z,relro,-z,now is intentionally omitted here, fixing issue: <https://github.com/wysaid/android-gpuimage-plus/issues/562> -target_link_options(CGE PRIVATE -fPIE -fPIC -pie) +# Note: -Wl,-z,relro,-z,now is intentionally omitted here, fixing issue: <https://github.com/wysaid/android-gpuimage-plus/issues/562> +# Android API 23+ enforces full RELRO by default; no explicit linker hardening flags are needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@library/src/main/jni/CMakeLists.txt` at line 88, The target_link_options invocation for target CGE is adding inappropriate/redundant flags (-pie, -fPIE, -fPIC) for a SHARED library; remove the entire call to target_link_options(CGE PRIVATE -fPIE -fPIC -pie) so the linker isn’t passed executable-only or compile-time flags and let CMake manage -fPIC for the SHARED target, or if you intended a specific link option keep only valid linker flags and apply them to target_link_options(CGE ...) appropriately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@library/src/main/jni/CMakeLists.txt`:
- Line 87: The comment line containing "# Note: -Wl,-z,relro,-z,now is
intentionally omitted here， fixing issue:
<https://github.com/wysaid/android-gpuimage-plus/issues/562>" uses a full-width
Chinese comma (U+FF0C); edit that comment (the CMakeLists.txt comment starting
with "# Note: -Wl,-z,relro,-z,now is intentionally omitted here") and replace
the full-width comma with an ASCII comma (U+002C), and scan nearby comment text
for any other full-width punctuation to normalize to ASCII.
---
Nitpick comments:
In `@library/src/main/jni/CMakeLists.txt`:
- Around line 83-85: Remove the redundant Chinese comment above the
target_compile_options line: delete the comment "# 非 Debug 模式下添加 -O3" and keep
the English comment "# Add -O3 in non-Debug mode" that precedes the
target_compile_options(CGE PRIVATE $<$<NOT:$<CONFIG:Debug>>:-O3>) statement so
only the English comment remains.
- Line 88: The target_link_options invocation for target CGE is adding
inappropriate/redundant flags (-pie, -fPIE, -fPIC) for a SHARED library; remove
the entire call to target_link_options(CGE PRIVATE -fPIE -fPIC -pie) so the
linker isn’t passed executable-only or compile-time flags and let CMake manage
-fPIC for the SHARED target, or if you intended a specific link option keep only
valid linker flags and apply them to target_link_options(CGE ...) appropriately.
- Remove redundant Chinese comment '# 非 Debug 模式下添加 -O3' (keep English only) - Fix full-width comma (U+FF0C) to ASCII comma in RELRO note comment - Remove target_link_options(CGE PRIVATE -fPIE -fPIC -pie): these flags are inappropriate/redundant for a SHARED library; CMake handles -fPIC automatically
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
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.
Problem
Fixes #562 — crash on Samsung S21 Plus (Android 14) with
3.1.1-16k-min:Root Cause
target_link_options(CGE PUBLIC ... -Wl,-z,relro,-z,now)propagated these flags toCGEExtvia CMake'sPUBLICinterface linkage. With 16KB page alignment enabled, the linker padsPT_GNU_RELROto 16KB boundaries, makingMemSiz(~10KB) far larger than the actual RW LOAD segment (~1.5KB). The Android dynamic linker then callsmprotect()over a larger-than-mapped region, which fails withENOMEMon memory-pressured devices.Android API 23+ already enforces full RELRO by default — these flags are redundant and harmful here.
Verified with
readelf(arm64libCGEExt.so)MemSiz0x0610= 1,552 bytes0x0628= 1,576 bytesPT_GNU_RELRO MemSiz0x28b0= 10,416 bytes → padded to 16KB at runtime0x3f50= 16,208 bytes ≈ 16KB ✓In the bug version the RELRO
MemSiz(10,416) far exceedsFileSiz(1,552), causing the out-of-boundsmprotect()failure.Changes
library/src/main/jni/CMakeLists.txt-Wl,-z,relro,-z,nowfromtarget_link_optionsPUBLICtoPRIVATEfor-fPIE -fPIC -pie(no-ops for shared libraries, no need to propagate)PUBLICscope for-Wl,-z,max-page-size=16384soCGEExtinherits the 16KB alignment flag automatically via CMake interface propagationcgeDemo/build.gradledisableVideoModule()helper functiontestDemoWithGradleLibsdependency dynamic: automatically selects the correct AAR variant suffix (-16k,-min,-16k-min) based onlocal.propertiesflags, instead of hardcoding3.1.1How to Verify
Build the demo with
testDemoWithGradleLibs=true,enable16kPageSizes=true,disableVideoModule=true— the dependency will resolve toorg.wysaid:gpuimage-plus:3.1.1-16k-min(the crashing version) for regression testing. The fixedlibCGEExt.socan be verified statically:Summary by CodeRabbit
Bug Fixes
Chores
Documentation