Closed xsscx closed 2 months ago
These are mostly good changes which I have mostly incorporated into the PCS_Refactor branch. There are some aspects that I question.
Note: Several IccXmlMpe fixes were also incorporated into the PCS_Refactor branch. It still needs testing and a bit of documentation, but I'm thinking its going in the right direction.
@maxderhak A quick check on the PCS_Refactor Branch, applying the revert, build, and then Asan Reports that allocator issue when creating profiles.
/bin/bash -c "$(curl -fsSL https://raw.githubusercontent.com/xsscx/PatchIccMAX/refs/heads/development/contrib/UnitTest/pcs_revert_build_asan_report.sh)"
| ^
/tmp/tmm/DemoIccMAX/IccXML/IccLibXML/IccTagXml.cpp: In member function ‘virtual bool CIccTagXmlCurve::ParseXml(xmlNode*, icConvertType, std::string&)’:
/tmp/tmm/DemoIccMAX/IccXML/IccLibXML/IccTagXml.cpp:2601:15: warning: ‘void free(void*)’ called on pointer returned from a mismatched allocation function [-Wmismatched-new-delete]
2601 | free(buf);
| ~~~~^~~~~
/tmp/tmm/DemoIccMAX/IccXML/IccLibXML/IccTagXml.cpp:2594:42: note: returned from ‘void* operator new [](std::size_t)’
2594 | char *buf = (char *) new char[num];
| ^
/tmp/tmm/DemoIccMAX/IccXML/IccLibXML/IccTagXml.cpp:2611:15: warning: ‘void free(void*)’ called on pointer returned from a mismatched allocation function [-Wmismatched-new-delete]
2611 | free(buf);
| ~~~~^~~~~
/tmp/tmm/DemoIccMAX/IccXML/IccLibXML/IccTagXml.cpp:2594:42: note: returned from ‘void* operator new [](std::size_t)’
2594 | char *buf = (char *) new char[num];
| ^
/tmp/tmm/DemoIccMAX/IccXML/IccLibXML/IccTagXml.cpp:2626:17: warning: ‘void free(void*)’ called on pointer returned from a mismatched allocation function [-Wmismatched-new-delete]
2626 | free(buf);
| ~~~~^~~~~
/tmp/tmm/DemoIccMAX/IccXML/IccLibXML/IccTagXml.cpp:2594:42: note: returned from ‘void* operator new [](std::size_t)’
2594 | char *buf = (char *) new char[num];
| ^
/tmp/tmm/DemoIccMAX/IccXML/IccLibXML/IccTagXml.cpp:2663:17: warning: ‘void free(void*)’ called on pointer returned from a mismatched allocation function [-Wmismatched-new-delete]
2663 | free(buf);
| ~~~~^~~~~
/tmp/tmm/DemoIccMAX/IccXML/IccLibXML/IccTagXml.cpp:2594:42: note: returned from ‘void* operator new [](std::size_t)’
2594 | char *buf = (char *) new char[num];
| ^
/tmp/tmm/DemoIccMAX/IccXML/IccLibXML/IccTagXml.cpp:2695:17: warning: ‘void free(void*)’ called on pointer returned from a mismatched allocation function [-Wmismatched-new-delete]
2695 | free(buf);
| ~~~~^~~~~
/tmp/tmm/DemoIccMAX/IccXML/IccLibXML/IccTagXml.cpp:2594:42: note: returned from ‘void* operator new [](std::size_t)’
2594 | char *buf = (char *) new char[num];
| ^
#!/bin/sh
##
## Unit Test for PCS_Refactor
## Asan Checks for CreateAllProfiles.sh
##
## David Hoyt for DemoIccMAX Project
## Date: 27-Sept-24
##
echo "Step 1: Configuring Git user for this session"
git config --global user.email "github-actions@github.com" || { echo "Error: Git config failed. Exiting."; exit 1; }
git config --global user.name "GitHub Actions" || { echo "Error: Git config failed. Exiting."; exit 1; }
echo "Git user configuration done."
echo "Step 2: Cloning DemoIccMAX repository"
git clone https://github.com/InternationalColorConsortium/DemoIccMAX.git || { echo "Error: Git clone failed. Exiting."; exit 1; }
cd DemoIccMAX/ || { echo "Error: Failed to change directory to DemoIccMAX. Exiting."; exit 1; }
echo "Repository cloned and switched to DemoIccMAX directory."
echo "Step 3: Checking out PCS_Refactor branch"
git checkout PCS_Refactor || { echo "Error: Git checkout PCS_Refactor failed. Exiting."; exit 1; }
echo "PCS_Refactor branch checked out."
echo "Step 4: Attempting to revert commit b90ac3933da99179df26351c39d8d9d706ac1cc6 non-interactively"
# Perform the revert non-interactively, skip conflicts, and use the current branch's version.
git revert --no-edit b90ac3933da99179df26351c39d8d9d706ac1cc6 || { echo "Revert conflict detected. Attempting to resolve conflicts automatically."; }
# Check if there are conflicts
if [ $(git ls-files -u | wc -l) -gt 0 ]; then
echo "Conflicts found. Resolving conflicts automatically by using the current branch version."
git ls-files -u | awk '{print $4}' | xargs git add
echo "Continuing the revert after resolving conflicts"
git revert --continue || { echo "Error: Git revert --continue failed. Exiting."; exit 1; }
else
echo "No conflicts detected. Revert completed successfully."
fi
echo "Step 5: Changing to Build directory"
cd Build/ || { echo "Error: Directory change to Build/ failed. Exiting."; exit 1; }
echo "Changed to Build directory."
echo "Step 6: Configuring the build with CMake"
cmake -DCMAKE_INSTALL_PREFIX=$HOME/.local \
-DCMAKE_BUILD_TYPE=Debug \
-DCMAKE_CXX_FLAGS="-g -fsanitize=address,undefined -fno-omit-frame-pointer -Wall" \
-Wno-dev Cmake/ || { echo "Error: CMake configuration failed. Exiting."; exit 1; }
echo "CMake configuration completed."
echo "Step 7: Building the code with make"
make -j32 || { echo "Error: Build failed. Exiting."; exit 1; }
echo "Build completed successfully."
# Optionally, uncomment and adjust the test step if required
echo "Step 8: Running the tests"
cd ../Testing || { echo "Error: Directory change to Testing failed. Exiting."; exit 1; }
/bin/bash -c "$(curl -fsSL https://raw.githubusercontent.com/xsscx/PatchIccMAX/refs/heads/development/contrib/UnitTest/pcs_refactor_create_profiles.sh)" > CreateAllProfiles.log 2>&1 || { echo "Error: Test script execution failed. Exiting."; ex
it 1; }
echo "Tests completed. Logs saved in CreateAllProfiles.log."
echo "All steps completed successfully."
name: PCS_Refactor Revert 06ac1cc6
on:
workflow_dispatch: # Allows manual triggering from the Actions tab in GitHub
push: # Automatically triggers on push events to the PCS_Refactor branch (optional)
branches:
- PCS_Refactor
jobs:
revert_commit_and_build:
runs-on: ubuntu-latest
steps:
- name: Set up Git anonymous identity
run: |
git config --global user.email "github-actions@github.com"
git config --global user.name "GitHub Actions"
- name: Remove existing DemoIccMAX directory (if it exists)
run: |
if [ -d "DemoIccMAX" ]; then
rm -rf DemoIccMAX
fi
- name: Checkout Code
run: |
git clone https://github.com/InternationalColorConsortium/DemoIccMAX.git
cd DemoIccMAX
git checkout PCS_Refactor
# Install Dependencies
- name: Install Dependencies
run: |
sudo apt-get update
# Check and install if packages are not installed
for pkg in libwxgtk-media3.0-gtk3-dev libwxgtk-webview3.0-gtk3-dev \
libwxgtk3.0-gtk3-dev libxml2 libtiff5 libxml2-dev libtiff5-dev \
clang-tools; do
if ! dpkg-query -W -f='${Status}' $pkg | grep "install ok installed"; then
echo "Installing $pkg"
sudo apt-get install -y $pkg
else
echo "$pkg is already installed"
fi
done
- name: Attempt Revert
run: |
cd DemoIccMAX
git revert --no-edit b90ac3933da99179df26351c39d8d9d706ac1cc6 || true
- name: Handle Conflict Locally
run: |
cd DemoIccMAX
if [ -f Tools/CmdLine/IccCommon/IccCmmConfig.cpp ]; then
# Resolve conflict by removing the file (this will not persist in the repository)
git rm Tools/CmdLine/IccCommon/IccCmmConfig.cpp
git add .
git revert --continue || true
else
echo "No conflict resolution needed"
fi
# Configure the build with CMake (keeping Cmake/ directory)
- name: Configure the build with CMake
run: |
cd DemoIccMAX/Build/
cmake -DCMAKE_INSTALL_PREFIX=$HOME/.local \
-DCMAKE_BUILD_TYPE=Debug \
-DCMAKE_CXX_FLAGS="-g -fsanitize=address,undefined -fno-omit-frame-pointer -Wall" \
-Wno-dev Cmake/
# Build the project with make and log output
- name: Build project with make and log output
run: |
cd DemoIccMAX/Build/
make -j8 > ../../build.log 2>&1 || true # Ensuring log goes to correct path relative to root
# Check build success (by searching for errors in the build log)
- name: Check build success
run: |
if grep -i "error" ../../build.log; then
echo "❌ Build Failed"
exit 1
else
echo "✅ Build Succeeded"
fi
# Create artifact directory for binaries
- name: Create artifact directory
run: mkdir -p DemoIccMAX/Build/artifact
# Move only .lib, .a, .so, .dll, and binaries (including a.out) to artifact directory
- name: Move libraries and binaries to artifact directory
run: |
find DemoIccMAX/Build/ -type f \( -name '*.lib' -o -name '*.a' -o -name '*.so' -o -name '*.dll' -o -name 'a.out' -o -executable \) -exec mv {} DemoIccMAX/Build/artifact/ \;
# Clean the target directory before moving artifacts
- name: Clean target directory before moving artifacts
run: |
if [ -d "DemoIccMAX-build-ubuntu-latest" ]; then
rm -rf DemoIccMAX-build-ubuntu-latest
fi
# Label Artifacts for ubuntu-latest
- name: Label Artifacts for ubuntu-latest
run: mv DemoIccMAX/Build/artifact DemoIccMAX-build-ubuntu-latest
# Upload build artifacts for ubuntu-latest
- name: Upload build artifacts
uses: actions/upload-artifact@v4
with:
name: DemoIccMAX-build-ubuntu-latest
path: DemoIccMAX-build-ubuntu-latest/
# Upload build log as an artifact
- name: Upload build log
uses: actions/upload-artifact@v4
with:
name: build-log
path: build.log
- name: Debug Git Status (Optional for checking result)
run: |
cd DemoIccMAX
git status
- name: Clean Up
run: |
cd DemoIccMAX
git reset --hard HEAD
git clean -fd
PR94
Refactored By: @xsscx Date: 10 September 2024 Original Issue in https://github.com/InternationalColorConsortium/DemoIccMAX/issues/84
Method of Identification: A regex pattern matching approach was used to search for the known typo icSigUnkownPlatform across all .h and .cpp files in the project. The incorrect occurrences were found and replaced with the correct spelling icSigUnknownPlatform.
Description: This PR resolves a typo present in several files where icSigUnkownPlatform is mistakenly used instead of icSigUnknownPlatform. This typo was identified using regex pattern matching and has been corrected across all relevant files.
Changes Made: Search: icSigUnkownPlatform Replace: icSigUnknownPlatform The following files have been updated:
IccProfLib/icProfileHeader.h: Line: icSigUnkownPlatform = 0x00000000 → icSigUnknownPlatform = 0x00000000 IccXML/IccLibXML/IccProfileXml.cpp: Condition check: if (m_Header.platform != icSigUnkownPlatform) → if (m_Header.platform != icSigUnknownPlatform) IccProfLib/IccUtil.cpp: Case: case icSigUnkownPlatform: → case icSigUnknownPlatform: IccProfLib/IccProfile.cpp: Case: case icSigUnkownPlatform: → case icSigUnknownPlatform:
Memory Management Issues
Description: The code in CIccTagSpectralViewingConditions and CIccProfile exhibits improper memory management practices. There is a mismatch in allocation and deallocation methods for dynamically allocated memory. This leads to the detection of alloc-dealloc-mismatch errors by AddressSanitizer.
PoC
Issue 1
File: IccTagBasic.cpp Line: 10908 Incorrect use of delete[] for memory allocated using malloc. CIccProfile Cleanup Function:
malloc()
The code from IccTagXml.cpp#L2105 shows the allocation of m_observer using malloc:
Corrected Destructor
Given that m_observer is allocated using malloc, it should be deallocated using free in the destructor.
Here is the corrected destructor code:
Refactored Code
History
The initial Commit is from 2015. Address Sanitizer reported a alloc-dealloc-mismatch (malloc vs operator delete []) when run on Ubuntu and macOS in 2024.
malloc () Guidelines
Allocation Functions:
Deallocation Functions:
Other Functions:
Issue 2
File: IccProfile.cpp Line: 263 Improper handling of dynamically allocated memory using manual delete and free operations.
Refactored Code
Explanation of Changes
Nullptr Assignment: Replaced NULL with nullptr for better type safety and clarity. After deleting pointers, set them to nullptr to avoid dangling pointers.
Range-based Loop: Replaced the manual iterator loop with a range-based loop for better readability and to reduce the potential for errors.
Clear Lists: Clear the tag lists using the clear method of the standard library containers.
Memset: Used std::memset instead of memset to adhere to C++ standards.
Memory Allocation Consistency: Ensure that the memory for m_pAttachIO and tagVal.ptr is consistently allocated with new if they are deleted with delete.
Issue 3 - Other Patches
Issue 4
Update IccTagComposite.cpp FIX: alloc-dealloc-mismatch in m_TagVals and m_pArray