Update fork to upstream CharLS 2.4.4#1
Open
sedghi wants to merge 9 commits into
Open
Conversation
…arls#272) GCC\Clang provide 3 overloads for __builtin_clz: int __builtin_clz (unsigned int x) int __builtin_clzl (unsigned long) int __builtin_clzll (unsigned long long) GCC and Clang can be used to build CharLS for Linux\macOS (LP64 data model) and Windows (LLP64 data model). Using __builtin_clzl (unsigned long) for 64 bits works for LP64 (long is 64 bits) but fails for the LLP64 data model (long is 32 bits) __builtin_clzll (unsigned long long) works for both the LP64 and the LLP64 data model (unsigned long long int is always at least 64 bits)
…eam-charls#367) * fix: Add support to parse a non-conforming padding zero byte before the EOI marker The JPEG-LS standard requires that the EOI markers follows the coded data. There are however JPEG-LS images (part of official DICOM samples) that add sometimes an additional padding byte. The HP JPEG-LS codec and CharLS versions before v2.3.0 were able to decode this images, as they don't perform strict checking for the EOI marker. Extend the read_end_of_image method to also be able to handle a single zero padding byte. * Update CHANGELOG.md Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Add extra unit test and tune code --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* fix: Prevent overflow in stride calculation by limiting height and width - By limiting the maximum supported height and width to 100000 * 100000 prevent that internal calculations overflow. - Improve the checking in checked_mul (intrinsic make it cheap). * Update checked_mul * Add limits to encoder * small improvements to the fix
📝 WalkthroughWalkthroughThis PR sets a new maximum image dimension (100,000), adds encoder/decoder validation for that bound, rewrites checked_mul with compiler-specific overflow detection, refines JPEG marker/EOI parsing, adds a decode benchmark, and updates CI workflows, build configs, analysis rules, version/changelog/security docs. ChangesDimension limits, overflow safety, and stream parsing
CI, tooling, build, benchmark, and release metadata updates
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Decoder as jpegls_decoder
participant Reader as jpeg_stream_reader
participant Frame as check_frame_info
participant Util as checked_mul
Decoder->>Reader: decode(destination, destination_size)
Reader->>Frame: validate width/height
Frame-->>Reader: throw invalid_parameter_width/height if > maximum
Reader->>Util: checked_mul(components, bytes_per_sample, width)
Util-->>Reader: minimum_stride or throw parameter_value_not_supported
Reader->>Util: checked_mul(bytes_per_plane, plane_count)
Util-->>Reader: minimum_destination_size or throw
Reader->>Reader: read_end_of_image()
Reader-->>Decoder: decoded data or thrown error
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jpeg_stream_reader.cpp (1)
111-130: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
bytes_per_planewithchecked_mulinsrc/jpeg_stream_reader.cpp:128-130On 32-bit builds,
stride * rect_.Heightcan wrap before the finalchecked_mul, which can makeminimum_destination_sizetoo small and let an undersized destination buffer pass the guard.Suggested fix
- const size_t bytes_per_plane{stride * rect_.Height}; + const size_t bytes_per_plane{checked_mul(stride, static_cast<size_t>(rect_.Height))};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jpeg_stream_reader.cpp` around lines 111 - 130, The destination size calculation in `jpeg_stream_reader::` buffer layout is vulnerable because `bytes_per_plane` is computed as `stride * rect_.Height` before overflow checking, which can wrap on 32-bit builds. Update the `bytes_per_plane` and `minimum_destination_size` logic to use `checked_mul` for the stride-by-height multiplication, and only subtract the stride adjustment after both size computations are validated. Keep the fix local to the buffer guard code in `src/jpeg_stream_reader.cpp` and preserve the existing `auto_calculate_stride`, `minimum_stride`, and `plane_count` behavior.
🧹 Nitpick comments (1)
benchmark/decode.cpp (1)
26-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
read_filecarries unused offset/bytes complexity.The
offsetandbytesparameters are never used by the only caller (bm_decodepasses just a filename). This adds dead code and potential for misuse (e.g., the negative-offset branch at lines 37-40). Consider simplifying to a basic file reader, or if the full functionality is intended for future benchmarks, document that intent.♻️ Optional simplification
-vector<uint8_t> read_file(const char* filename, long offset = 0, size_t bytes = 0) +vector<uint8_t> read_file(const char* filename) try { ifstream input; input.exceptions(ios::eofbit | ios::failbit | ios::badbit); input.open(filename, ios::in | ios::binary); input.seekg(0, ios::end); - const auto byte_count_file{static_cast<int>(input.tellg())}; - input.seekg(offset, ios::beg); - - if (offset < 0) - { - offset = static_cast<long>(byte_count_file - bytes); - } - if (bytes == 0) - { - bytes = static_cast<size_t>(byte_count_file) - offset; - } + const auto byte_count_file{static_cast<size_t>(input.tellg())}; + input.seekg(0, ios::beg); - vector<uint8_t> buffer(bytes); + vector<uint8_t> buffer(byte_count_file); read(input, buffer); return buffer; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmark/decode.cpp` around lines 26 - 55, Simplify read_file by removing the unused offset and bytes parameters and the related seek/size branching, since bm_decode only calls it with a filename. Keep the function as a straightforward binary file reader using read(input, buffer), and update the read_file definition and any related usage so the remaining logic is only the actual file-loading path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build.yml:
- Around line 16-18: The checkout step in the workflow still persists the
default GITHUB_TOKEN in git config, which should be disabled for this job.
Update the existing actions/checkout configuration by adding persist-credentials
set to false alongside the current fetch-depth setting so credentials are not
retained after checkout. Keep the change localized to the checkout step in
build.yml.
In @.github/workflows/codeql-analysis.yml:
- Around line 30-31: The checkout step in the CodeQL workflow should disable
persisted Git credentials for consistency and hardening. Update the existing
actions/checkout invocation in the workflow job to set persist-credentials to
false, matching the behavior used in build.yml and ensuring the checkout step
does not leave credentials available after fetch.
In `@appveyor.yml`:
- Line 1: The AppVeyor configuration still references the old 2.4.3 release, so
update the version in appveyor.yml to match the CharLS 2.4.4 release if AppVeyor
is still in use; otherwise remove the obsolete AppVeyor config entirely. Use the
version setting in appveyor.yml as the unique place to locate and adjust this
release mismatch.
In `@benchmark/decode.cpp`:
- Around line 58-72: The benchmark in bm_decode currently hardcodes a
Windows-only absolute image path, which makes it non-portable. Update bm_decode
to obtain the test image location from a configurable source instead of
"d:/benchmark-test-image.jls", such as a benchmark argument via Google Benchmark
Args() or a relative repo-checked test asset, and keep the read_file call using
that resolved path so the benchmark runs across platforms and in CI.
In `@benchmark/vcpkg.json`:
- Around line 2-5: The vcpkg manifest is using an invalid builtin-baseline in
benchmark/vcpkg.json, so update the baseline to a real vcpkg commit that
resolves successfully and then verify the benchmark dependency entry still
satisfies version>=1.8.5. Check the manifest fields in vcpkg.json and make sure
the baseline SHA matches an existing vcpkg repository state so dependency
resolution works.
---
Outside diff comments:
In `@src/jpeg_stream_reader.cpp`:
- Around line 111-130: The destination size calculation in
`jpeg_stream_reader::` buffer layout is vulnerable because `bytes_per_plane` is
computed as `stride * rect_.Height` before overflow checking, which can wrap on
32-bit builds. Update the `bytes_per_plane` and `minimum_destination_size` logic
to use `checked_mul` for the stride-by-height multiplication, and only subtract
the stride adjustment after both size computations are validated. Keep the fix
local to the buffer guard code in `src/jpeg_stream_reader.cpp` and preserve the
existing `auto_calculate_stride`, `minimum_stride`, and `plane_count` behavior.
---
Nitpick comments:
In `@benchmark/decode.cpp`:
- Around line 26-55: Simplify read_file by removing the unused offset and bytes
parameters and the related seek/size branching, since bm_decode only calls it
with a filename. Keep the function as a straightforward binary file reader using
read(input, buffer), and update the read_file definition and any related usage
so the remaining logic is only the actual file-loading path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7426a771-d168-44be-8e06-06852ad877e3
📒 Files selected for processing (32)
.clang-tidy.editorconfig.github/workflows/build.yml.github/workflows/codeql-analysis.ymlCHANGELOG.mdCharLS.sln.DotSettingsSECURITY.mdappveyor.ymlazure-pipelines.ymlbenchmark/benchmark.cppbenchmark/benchmark.vcxprojbenchmark/benchmark.vcxproj.filtersbenchmark/decode.cppbenchmark/vcpkg.jsondefault.rulesetdefault.ruleset.mdinclude/charls/public_types.hinclude/charls/version.hspelling.dicsrc/charls_jpegls_encoder.cppsrc/color_transform.hsrc/constants.hsrc/context_regular_mode.hsrc/decoder_strategy.hsrc/jpeg_stream_reader.cppsrc/jpeg_stream_reader.hsrc/util.htest/util.hunittest/jpeg_stream_reader_test.cppunittest/jpegls_decoder_test.cppunittest/jpegls_encoder_test.cppunittest/pch.h
Comment on lines
+16
to
18
| - uses: actions/checkout@v6 | ||
| with: | ||
| fetch-depth: 0 # Shallow clones should be disabled for a better relevancy of analysis |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Set persist-credentials: false on checkout to prevent credential persistence.
The actions/checkout action defaults to persist-credentials: true, which leaves the GITHUB_TOKEN in the local git config. This is flagged by zizmor as an artipacked risk. Since this workflow only needs the token for checkout (SonarCloud uses its own SONAR_TOKEN), disabling credential persistence is safe and recommended.
🔒️ Proposed fix
- uses: actions/checkout@v6
with:
fetch-depth: 0 # Shallow clones should be disabled for a better relevancy of analysis
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - uses: actions/checkout@v6 | |
| with: | |
| fetch-depth: 0 # Shallow clones should be disabled for a better relevancy of analysis | |
| - uses: actions/checkout@v6 | |
| with: | |
| fetch-depth: 0 # Shallow clones should be disabled for a better relevancy of analysis | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 16-18: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/build.yml around lines 16 - 18, The checkout step in the
workflow still persists the default GITHUB_TOKEN in git config, which should be
disabled for this job. Update the existing actions/checkout configuration by
adding persist-credentials set to false alongside the current fetch-depth
setting so credentials are not retained after checkout. Keep the change
localized to the checkout step in build.yml.
Source: Linters/SAST tools
Comment on lines
30
to
+31
| - name: Checkout repository | ||
| uses: actions/checkout@v2 | ||
| uses: actions/checkout@v6 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Set persist-credentials: false on checkout for consistency and security.
Same as the build.yml workflow, the checkout action here defaults to persisting credentials. Adding persist-credentials: false is a low-cost hardening measure consistent with the recommendation in build.yml.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 30-33: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/codeql-analysis.yml around lines 30 - 31, The checkout
step in the CodeQL workflow should disable persisted Git credentials for
consistency and hardening. Update the existing actions/checkout invocation in
the workflow job to set persist-credentials to false, matching the behavior used
in build.yml and ensuring the checkout step does not leave credentials available
after fetch.
Source: Linters/SAST tools
| @@ -1,4 +1,4 @@ | |||
| version: 2.1.1.{build} | |||
| version: 2.4.3.{build} | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
AppVeyor version (2.4.3) doesn't match target release (2.4.4).
The PR updates to CharLS 2.4.4, but appveyor.yml is set to 2.4.3.{build}. If AppVeyor is still active, this should be 2.4.4.{build} to match the release version. If AppVeyor is deprecated, consider removing the file instead.
🔧 Proposed fix
-version: 2.4.3.{build}
+version: 2.4.4.{build}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| version: 2.4.3.{build} | |
| version: 2.4.4.{build} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@appveyor.yml` at line 1, The AppVeyor configuration still references the old
2.4.3 release, so update the version in appveyor.yml to match the CharLS 2.4.4
release if AppVeyor is still in use; otherwise remove the obsolete AppVeyor
config entirely. Use the version setting in appveyor.yml as the unique place to
locate and adjust this release mismatch.
Comment on lines
+58
to
+72
| static void bm_decode(benchmark::State& state) | ||
| { | ||
| const auto source{read_file("d:/benchmark-test-image.jls")}; | ||
|
|
||
| // Pre-allocate the destination outside the measurement loop. | ||
| // std::vector initializes its elements and this step needs to be excluded from the measurement. | ||
| vector<uint8_t> destination(jpegls_decoder{source, true}.destination_size()); | ||
|
|
||
| for (const auto _ : state) | ||
| { | ||
| jpegls_decoder decoder(source.data(), source.size()); | ||
| decoder.decode(destination); | ||
| } | ||
| } | ||
| BENCHMARK(bm_decode); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚖️ Poor tradeoff
Hardcoded absolute path makes benchmark non-portable.
Line 60 hardcodes "d:/benchmark-test-image.jls" — a Windows-specific drive path that won't resolve on Linux/macOS or on machines without a D: drive. This makes the benchmark unusable outside the original developer's environment and will fail in CI.
Consider accepting the path as a command-line argument (Google Benchmark supports Args()) or using a relative path to a checked-in test image.
♻️ Suggested fix: accept path via benchmark args
static void bm_decode(benchmark::State& state)
{
- const auto source{read_file("d:/benchmark-test-image.jls")};
+ const auto source{read_file(static_cast<const char*>(state.range(0)))};
// Pre-allocate the destination outside the measurement loop.
// std::vector initializes its elements and this step needs to be excluded from the measurement.
vector<uint8_t> destination(jpegls_decoder{source, true}.destination_size());
for (const auto _ : state)
{
jpegls_decoder decoder(source.data(), source.size());
decoder.decode(destination);
}
}
-BENCHMARK(bm_decode);
+BENCHMARK(bm_decode)->Args({"d:/benchmark-test-image.jls"});Alternatively, use a relative path to a test file already in the repo:
- const auto source{read_file("d:/benchmark-test-image.jls")};
+ const auto source{read_file("../unittest/DataFiles/test8.jls")};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmark/decode.cpp` around lines 58 - 72, The benchmark in bm_decode
currently hardcodes a Windows-only absolute image path, which makes it
non-portable. Update bm_decode to obtain the test image location from a
configurable source instead of "d:/benchmark-test-image.jls", such as a
benchmark argument via Google Benchmark Args() or a relative repo-checked test
asset, and keep the read_file call using that resolved path so the benchmark
runs across platforms and in CI.
Comment on lines
+2
to
+5
| "$schema": "https://raw.githubusercontent.com/microsoft/vcpkg/master/scripts/vcpkg.schema.json", | ||
| "dependencies": [ { "name": "benchmark", "version>=":"1.8.5" } ], | ||
| "builtin-baseline": "3508985146f1b1d248c67ead13f8f54be5b4f5da" | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the vcpkg baseline commit exists and check the benchmark port version
curl -s "https://api.github.com/repos/microsoft/vcpkg/commits/3508985146f1b1d248c67ead13f8f54be5b4f5da" | jq -r '.sha // "NOT FOUND"'Repository: cornerstonejs/charls
Length of output: 168
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== manifest =="
nl -ba benchmark/vcpkg.json
echo
echo "== commit lookup =="
python3 - <<'PY'
import json, urllib.request, urllib.error
url = "https://api.github.com/repos/microsoft/vcpkg/commits/3508985146f1b1d248c67ead13f8f54be5b4f5da"
req = urllib.request.Request(url, headers={"Accept":"application/vnd.github+json","User-Agent":"curl"})
try:
with urllib.request.urlopen(req, timeout=20) as r:
print("status", r.status)
data = json.load(r)
print("sha", data.get("sha"))
except urllib.error.HTTPError as e:
print("status", e.code)
body = e.read().decode("utf-8", "replace")
print(body[:1000])
PY
echo
echo "== baseline file exists in vcpkg repo? =="
python3 - <<'PY'
import json, urllib.request, urllib.error
sha = "3508985146f1b1d248c67ead13f8f54be5b4f5da"
url = f"https://api.github.com/repos/microsoft/vcpkg/git/commits/{sha}"
req = urllib.request.Request(url, headers={"Accept":"application/vnd.github+json","User-Agent":"curl"})
try:
with urllib.request.urlopen(req, timeout=20) as r:
print("status", r.status)
data = json.load(r)
print("tree", data.get("tree", {}).get("sha"))
except urllib.error.HTTPError as e:
print("status", e.code)
print(e.read().decode("utf-8", "replace")[:1000])
PYRepository: cornerstonejs/charls
Length of output: 213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("benchmark/vcpkg.json")
print("== manifest ==")
for i, line in enumerate(path.read_text().splitlines(), 1):
if 1 <= i <= 20:
print(f"{i:4d}: {line}")
PY
echo
python3 - <<'PY'
import json, urllib.request, urllib.error
sha = "3508985146f1b1d248c67ead13f8f54be5b4f5da"
url = f"https://api.github.com/repos/microsoft/vcpkg/commits/{sha}"
req = urllib.request.Request(url, headers={"Accept":"application/vnd.github+json","User-Agent":"curl"})
try:
with urllib.request.urlopen(req, timeout=20) as r:
data = json.load(r)
print("== commit lookup ==")
print("status:", r.status)
print("sha:", data.get("sha"))
except urllib.error.HTTPError as e:
print("== commit lookup ==")
print("status:", e.code)
print(e.read().decode("utf-8", "replace")[:1000])
PYRepository: cornerstonejs/charls
Length of output: 3034
Fix the vcpkg baseline SHA benchmark/vcpkg.json:2-4 points builtin-baseline at a non-existent vcpkg commit, so the manifest won’t resolve. Update it to a valid baseline, then re-check the benchmark version>=1.8.5 constraint.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmark/vcpkg.json` around lines 2 - 5, The vcpkg manifest is using an
invalid builtin-baseline in benchmark/vcpkg.json, so update the baseline to a
real vcpkg commit that resolves successfully and then verify the benchmark
dependency entry still satisfies version>=1.8.5. Check the manifest fields in
vcpkg.json and make sure the baseline SHA matches an existing vcpkg repository
state so dependency resolution works.
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
2 participants
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.
Advances the cornerstonejs CharLS fork from its pin
38d95d0(~2.4.1-era, Jan 2023) to upstream 2.4.4 (latest stable, 2026-06-08).No custom patches to carry: the fork's pinned commit is a plain upstream commit with zero cornerstonejs divergence (merge-base with upstream == HEAD), and
38d95d0is a direct ancestor of2.4.4— a clean version advance, not a rebase.Validation runs on the codecs-repo submodule-bump PR (wasm build + pixel-correctness goldens). Not built locally.
Summary by CodeRabbit
New Features
Bug Fixes
Chores