fix: skip binaries if eu-strip clears all program headers #1284
Conversation
In some cases, `eu-strip` produces broken ELF binaries when stripping certain shared libraries or executables. Specifically, the resulting binary may contain only NULL program headers (as reported by `readelf -l`), leading to the loss of essential segments such as PT_DYNAMIC. Such binaries are unusable by the dynamic loader. To avoid overwriting a valid binary with a corrupted one, an additional validation step has been introduced. The stripped output is now checked for ELF integrity: - The total number of program headers is parsed via `readelf -l` - The number of `NULL` type headers is counted - If all headers are NULL (and the total is non-zero), the stripped binary is considered invalid In this case, the replacement of the original binary is skipped,and all temporary files are discarded.This fix addresses issues encountered when stripping Qt plugins,system libraries, or other ELF files that may exhibit this behavior. Log: skip binaries if eu-strip clears all program headers
Reviewer's GuideIntroduce a safe-strip workflow that uses temporary files and an ELF integrity check via readelf to detect and skip corrupted binaries when eu-strip clears all program headers. Sequence diagram for the new safe-strip workflow with ELF integrity checksequenceDiagram
participant Script as symbols-strip.sh
participant File as ELF Binary
participant Temp as Temp Files
participant EuStrip as eu-strip
participant ReadElf as readelf
participant Debug as Debug File
Script->>Temp: Create tmp_strip and tmp_debug
Script->>Temp: Copy File to tmp_strip
Script->>EuStrip: Strip tmp_strip, output to tmp_debug
Script->>ReadElf: Check program headers in tmp_strip
ReadElf-->>Script: Return total and NULL header count
alt All headers are NULL and total > 0
Script->>Temp: Remove tmp_strip and tmp_debug
Script->>Script: Skip replacing original File
else Valid headers
Script->>File: Replace with tmp_strip
Script->>Debug: Move tmp_debug to debugIDFile
Script->>Debug: Create/Update symlink for debug file
end
Class diagram for updated symbols-strip.sh workflowclassDiagram
class symbols-strip.sh {
+process(filepath)
+safe_strip(filepath)
+check_elf_integrity(tmp_strip)
}
class eu-strip {
+strip(input, output)
}
class readelf {
+get_program_headers(file)
}
class debugIDFile
class tmp_strip
class tmp_debug
symbols-strip.sh --> eu-strip : uses
symbols-strip.sh --> readelf : uses
symbols-strip.sh --> tmp_strip : creates
symbols-strip.sh --> tmp_debug : creates
symbols-strip.sh --> debugIDFile : manages
tmp_strip <.. eu-strip : input
tmp_debug <.. eu-strip : output
debugIDFile <.. tmp_debug : destination
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| buildID=$(readelf -n "$filepath" | grep 'Build ID' | awk '{print $NF}') | ||
| debugIDFile="$prefix/lib/debug/.build-id/${buildID:0:2}/${buildID:2}.debug" | ||
| mkdir -p "$(dirname "$debugIDFile")" | ||
| eu-strip "$filepath" -f "$debugIDFile" | ||
|
|
||
| tmp_strip=$(mktemp) | ||
| tmp_debug=$(mktemp) | ||
| cp "$filepath" "$tmp_strip" | ||
| eu-strip "$tmp_strip" -f "$tmp_debug" | ||
|
|
||
| total=$(readelf -l "$tmp_strip" 2>/dev/null | grep "There are" | awk '{print $3}') | ||
| null_count=$(readelf -l "$tmp_strip" 2>/dev/null | grep -c "^ NULL") | ||
| if [ -n "$total" ] && [ "$total" -eq "$null_count" ] && [ "$total" -ne 0 ]; then | ||
| echo "invalid program headers after stripping, skipping: $filepath" | ||
| rm -f "$tmp_strip" "$tmp_debug" | ||
| continue | ||
| fi | ||
| mv "$tmp_strip" "$filepath" | ||
| mv "$tmp_debug" "$debugIDFile" | ||
|
|
||
| echo "striped $filepath to $debugIDFile" | ||
|
|
||
| debugFile="$prefix/lib/debug/$filepath.debug" | ||
| mkdir -p "$(dirname "$debugFile")" | ||
| ln -s "$debugIDFile" "$debugFile" | ||
| ln -sf "$debugIDFile" "$debugFile" | ||
| done <"$tmpFile" |
There was a problem hiding this comment.
Error Handling Issue
The script lacks comprehensive error handling for external commands (readelf, mkdir, cp, mv, etc.). This can lead to partial operations and inconsistent states if any command fails.
Recommendation:
Implement error checks after each critical operation. For instance, after mkdir -p "$(dirname "$debugIDFile")" (line 26), you can check if the directory was created successfully:
mkdir -p "$(dirname "$debugIDFile")" || { echo "Failed to create directory"; exit 1; }| buildID=$(readelf -n "$filepath" | grep 'Build ID' | awk '{print $NF}') | ||
| debugIDFile="$prefix/lib/debug/.build-id/${buildID:0:2}/${buildID:2}.debug" | ||
| mkdir -p "$(dirname "$debugIDFile")" | ||
| eu-strip "$filepath" -f "$debugIDFile" | ||
|
|
||
| tmp_strip=$(mktemp) | ||
| tmp_debug=$(mktemp) | ||
| cp "$filepath" "$tmp_strip" | ||
| eu-strip "$tmp_strip" -f "$tmp_debug" | ||
|
|
||
| total=$(readelf -l "$tmp_strip" 2>/dev/null | grep "There are" | awk '{print $3}') | ||
| null_count=$(readelf -l "$tmp_strip" 2>/dev/null | grep -c "^ NULL") | ||
| if [ -n "$total" ] && [ "$total" -eq "$null_count" ] && [ "$total" -ne 0 ]; then | ||
| echo "invalid program headers after stripping, skipping: $filepath" | ||
| rm -f "$tmp_strip" "$tmp_debug" | ||
| continue | ||
| fi | ||
| mv "$tmp_strip" "$filepath" | ||
| mv "$tmp_debug" "$debugIDFile" | ||
|
|
||
| echo "striped $filepath to $debugIDFile" | ||
|
|
||
| debugFile="$prefix/lib/debug/$filepath.debug" | ||
| mkdir -p "$(dirname "$debugFile")" | ||
| ln -s "$debugIDFile" "$debugFile" | ||
| ln -sf "$debugIDFile" "$debugFile" | ||
| done <"$tmpFile" |
There was a problem hiding this comment.
Security Issue: Potential Command Injection
The script directly uses variable expansions in shell commands without sanitizing or properly quoting them, which could lead to command injection if the variables contain special characters or malicious inputs.
Recommendation:
Ensure all variable expansions are properly quoted and consider sanitizing inputs that are used in shell commands. For example, use "$filepath" securely by ensuring it does not contain unexpected characters or use tools like printf %q "$filepath" to sanitize it.
- Change the storage location of temporary files - Run readelf -l only once and reuse its output for all checks - Only calculate null_count when total is valid Log: cache readelf output to avoid redundant calls
|
Here's the code health analysis summary for commits Analysis Summary
|
| tmp_strip=$(mktemp "$prefix/stripXXX") | ||
| tmp_debug=$(mktemp "$prefix/debugXXX") | ||
| cp -p "$filepath" "$tmp_strip" | ||
| eu-strip "$tmp_strip" -f "$tmp_debug" | ||
|
|
||
| readelf_output=$(readelf -l "$tmp_strip" 2>/dev/null) | ||
| total=$(echo "$readelf_output" | grep "There are" | awk '{print $3}') | ||
| if [ -n "$total" ] && [ "$total" -ne 0 ]; then | ||
| null_count=$(echo "$readelf_output" | grep -c "^ NULL") | ||
| if [ "$total" -eq "$null_count" ]; then | ||
| echo "invalid program headers after stripping, skipping: $filepath" | ||
| rm -f "$tmp_strip" "$tmp_debug" | ||
| continue | ||
| fi | ||
| fi | ||
| mv "$tmp_strip" "$filepath" | ||
| mv "$tmp_debug" "$debugIDFile" |
There was a problem hiding this comment.
Temporary File Cleanup and Error Handling
The script creates temporary files and performs critical file operations without ensuring that these files are cleaned up in case of errors or script interruption. This can lead to leftover temporary files which might contain sensitive data or consume system resources.
Recommendation:
Implement a trap mechanism to clean up temporary files on script exit or interruption. For example:
trap 'rm -f "$tmp_strip" "$tmp_debug"' EXITAdditionally, add error checking after each file operation (mv, cp, ln) to ensure they complete successfully and handle failures gracefully:
mv "$tmp_strip" "$filepath" || { echo "Failed to move file"; exit 1; }| mv "$tmp_strip" "$filepath" | ||
| mv "$tmp_debug" "$debugIDFile" | ||
|
|
||
| echo "striped $filepath to $debugIDFile" | ||
|
|
||
| debugFile="$prefix/lib/debug/$filepath.debug" | ||
| mkdir -p "$(dirname "$debugFile")" | ||
| ln -s "$debugIDFile" "$debugFile" | ||
| ln -sf "$debugIDFile" "$debugFile" |
There was a problem hiding this comment.
Potential Data Race and Lack of Concurrency Control
The script performs file operations such as moving and linking files within a loop that processes multiple files. If multiple instances of this script run concurrently, it could lead to data races, resulting in corrupted or unexpected file states.
Recommendation:
Implement file locking mechanisms or check if another instance of the script is running before proceeding with file operations. This can be achieved using lock files or system-specific locking mechanisms to ensure that only one instance manipulates files at a time.
- If eu-strip fails, fallback to using objcopy to extract the debug file and store it in the build-id directory. Log: optimize symbols-strip.sh logic
| tmp_strip=$(mktemp "$prefix/strip.XXXXXX") | ||
| tmp_debug=$(mktemp "$prefix/debug.XXXXXX") |
There was a problem hiding this comment.
Security Concern: Unsafe Temporary File Creation
The script creates temporary files using mktemp with a directory prefix derived from the $prefix variable (lines 29-30). This approach can lead to security vulnerabilities if $prefix contains special characters, relative paths, or is manipulated by an external user. This could potentially allow directory traversal or unintended file writes.
Recommendation:
Ensure that the $prefix variable is properly sanitized and validated to contain only absolute paths and no special characters. Additionally, consider setting a secure default location for temporary files if $prefix is not set.
| rm -f "$tmp_strip" "$tmp_debug" | ||
| objcopy --only-keep-debug "$filepath" "$debugIDFile" || true | ||
| [ -s "$debugIDFile" ] || continue | ||
| ln -sf "$debugIDFile" "$debugFile" |
There was a problem hiding this comment.
Reliability Issue: Unchecked Symbolic Link Creation
The script uses ln -sf to create symbolic links without checking if the operation was successful (line 42). This could lead to issues if the target directory does not exist or if there are permission issues.
Recommendation:
Add error checking after the ln -sf command to ensure that the symbolic link was created successfully. For example:
ln -sf "$debugIDFile" "$debugFile" || { echo "Failed to create symbolic link"; exit 1; }1 participant
In some cases,
eu-stripproduces broken ELF binaries when stripping certain shared libraries or executables. Specifically, the resulting binary may contain only NULL program headers (as reported byreadelf -l), leading to the loss of essential segments such as PT_DYNAMIC. Such binaries are unusable by the dynamic loader. To avoid overwriting a valid binary with a corrupted one, an additional validation step has been introduced. The stripped output is now checked for ELF integrity:readelf -lNULLtype headers is countedLog: skip binaries if eu-strip clears all program headers
Summary by Sourcery
Add validation to the symbols-strip script to avoid corrupting binaries when eu-strip removes all program headers
Bug Fixes:
Enhancements: