Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

lib/Epub/Epub/converters/JpegToFramebufferConverter.cpp (2)

54-60: ⚠️ Potential issue | 🟡 Minor

Guard FsFile allocation failure.

new FsFile() can fail under low heap; guard it (e.g., std::nothrow) before dereferencing.

🔧 Suggested change

-  FsFile* f = new FsFile();
-  if (!Storage.openFileForRead("JPG", std::string(filename), *f)) {
+  FsFile* f = new (std::nothrow) FsFile();
+  if (!f || !Storage.openFileForRead("JPG", std::string(filename), *f)) {
     delete f;
     return nullptr;
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/Epub/Epub/converters/JpegToFramebufferConverter.cpp` around lines 54 -
60, The code in jpegOpen allocates FsFile with new FsFile() without checking for
allocation failure; change the allocation to use std::nothrow (or otherwise
guard against std::bad_alloc) when creating the FsFile in jpegOpen, check the
pointer for nullptr before using it or calling Storage.openFileForRead, and if
allocation fails return nullptr (ensuring no dereference of a null pointer);
adjust cleanup paths (delete f) accordingly so FsFile is only deleted if
allocation succeeded and after use.

---

269-319: ⚠️ Potential issue | 🟡 Minor

Add a zero‑dimension guard before scale math.

If a corrupt JPEG reports 0×0, fineScale will divide by zero; bail out early after dimension validation.

🔧 Suggested change

   int srcWidth = jpeg->getWidth();
   int srcHeight = jpeg->getHeight();
 
   if (!validateImageDimensions(srcWidth, srcHeight, "JPEG")) {
     jpeg->close();
     delete jpeg;
     return false;
   }
+  if (srcWidth <= 0 || srcHeight <= 0) {
+    LOG_ERR("JPG", "Invalid JPEG dimensions: %dx%d", srcWidth, srcHeight);
+    jpeg->close();
+    delete jpeg;
+    return false;
+  }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/Epub/Epub/converters/JpegToFramebufferConverter.cpp` around lines 269 -
319, The code can divide by zero when srcWidth or the computed jpegScaleDenom
produce ctx.scaledSrcWidth==0 (e.g., corrupt 0×0 JPEG); after computing
ctx.scaledSrcWidth and ctx.scaledSrcHeight (and before computing ctx.fineScale)
add a guard that treats zero dimensions as a fatal validation failure: close and
delete the jpeg (same cleanup as earlier) and return false. Locate the block
around jpeg->getWidth()/getHeight(), validateImageDimensions, the
jpegScaleOption/denom logic, and the ctx.fineScale assignment to insert this
early bailout.

scripts/patch_jpegdec.py (3)

24-27: ⚠️ Potential issue | 🟡 Minor

Use PROJECT_LIBDEPS_DIR and guard missing directories.

Hard‑coding .pio/libdeps ignores PlatformIO’s configurable libdeps_dir, and os.listdir() will throw if the directory is missing. Prefer PROJECT_LIBDEPS_DIR (with a fallback) and guard existence.

🔧 Suggested change

-    libdeps_dir = os.path.join(env["PROJECT_DIR"], ".pio", "libdeps")
-    for env_dir in os.listdir(libdeps_dir):
+    libdeps_dir = env.get("PROJECT_LIBDEPS_DIR", os.path.join(env["PROJECT_DIR"], ".pio", "libdeps"))
+    if not os.path.isdir(libdeps_dir):
+        return
+    for env_dir in os.listdir(libdeps_dir):

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/patch_jpegdec.py` around lines 24 - 27, The code currently hard-codes
libdeps_dir and calls os.listdir() without checking existence; update
scripts/patch_jpegdec.py to prefer env.get("PROJECT_LIBDEPS_DIR") with a
fallback to os.path.join(env["PROJECT_DIR"], ".pio", "libdeps"), assign that to
libdeps_dir, then guard with os.path.isdir(libdeps_dir) before calling
os.listdir(libdeps_dir) and handle the missing-dir case (log/raise/exit) so
jpeg_inl path construction (jpeg_inl variable) only runs when the directory
exists.

---

54-56: ⚠️ Potential issue | 🟠 Major

Fail fast when patch hunks are missing.

A warning allows the build to proceed unpatched, reintroducing the progressive JPEG / MCU_SKIP issues. Prefer a hard failure (optionally with an explicit opt‑out env var).

🔧 Suggested change (apply similarly to both patch checks)

-    if OLD not in content:
-        print("WARNING: JPEGDEC AC table patch target not found in %s — library may have been updated" % filepath)
-        return
+    if OLD not in content:
+        if os.getenv("PATCH_ALLOW_MISSING"):
+            print("WARNING: JPEGDEC AC table patch target not found in %s — library may have been updated" % filepath)
+            return
+        raise RuntimeError(
+            "JPEGDEC AC table patch target not found in %s — verify library version/patches" % filepath
+        )

Also applies to: 85-87

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/patch_jpegdec.py` around lines 54 - 56, Replace the silent
warning+return when a patch hunk is missing with a hard failure: in the check
"if OLD not in content:" (and the similar second check later) stop the process
with a non‑zero exit (e.g. raise RuntimeError or call sys.exit(1)) so the build
cannot proceed unpatched; allow an explicit opt‑out by checking an env var
(suggest name SKIP_MISSING_PATCH or ALLOW_MISSING_PATCH) and only exit if that
env var is not set. Ensure you import os and sys if needed and update both
occurrences (the "if OLD not in content:" block and the analogous later block)
to follow this pattern.

---

20-23: ⚠️ Potential issue | 🟡 Minor

Silence Ruff F821/ARG001 for SCons-injected symbols.

Static analysis already flags Import/env as undefined and source/target as unused; add a file‑level ignore and underscore the unused args to keep CI clean.

🔧 Suggested change

+ # ruff: noqa: F821, ARG001
 Import("env")
 import os
 
-def patch_jpegdec(source, target, env):
+def patch_jpegdec(_source, _target, env):

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/patch_jpegdec.py` around lines 20 - 23, Add a file-level ruff ignore
for the SCons-injected symbols and underscore unused args: add a top-of-file
comment like "ruff: noqa: F821, ARG001" to silence undefined-name and unused-arg
warnings for Import and env, and rename the function parameters in patch_jpegdec
from (source, target, env) to (_source, _target, _env) so the linter knows they
are intentionally unused while keeping the SCons signature intact.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

lib/Epub/Epub/converters/JpegToFramebufferConverter.cpp (2)

54-62: new FsFile() can throw / crash on low-heap — std::nothrow missing.

Both JPEGDEC allocations below (lines 218, 250) correctly use new (std::nothrow), but jpegOpen uses a throwing new. On ESP32 with exceptions disabled, a failed allocation calls abort() rather than returning nullptr, bypassing the null check entirely.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/Epub/Epub/converters/JpegToFramebufferConverter.cpp` around lines 54 -
62, The jpegOpen function uses throwing new for FsFile which can abort on low
memory; change the allocation to use non-throwing new (std::nothrow) for FsFile
in jpegOpen, check the returned pointer for null and handle failure by not
calling Storage.openFileForRead and returning nullptr (ensuring no leak), and
keep existing delete f on the error path and setting *size only when f is valid;
reference the jpegOpen function and the FsFile allocation to locate the fix.

---

269-276: Zero-dimension guard before scale math is unverified.

validateImageDimensions may handle zero values, but if it doesn't, ctx.scaledSrcWidth = 0 at line 314 and the division (float)destWidth / ctx.scaledSrcWidth at line 318 would be a divide-by-zero.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/Epub/Epub/converters/JpegToFramebufferConverter.cpp` around lines 269 -
276, The code currently relies on validateImageDimensions to catch zero
dimensions but then uses srcWidth/srcHeight to compute ctx.scaledSrcWidth and
perform division (e.g., (float)destWidth / ctx.scaledSrcWidth), risking a
divide-by-zero; add an explicit guard after int srcWidth = jpeg->getWidth(); int
srcHeight = jpeg->getHeight(); to check srcWidth > 0 && srcHeight > 0 and on
failure call jpeg->close(); delete jpeg; and return false (or propagate an
error) before any scaling math, referencing validateImageDimensions and
ctx.scaledSrcWidth to ensure the division cannot occur with a zero denominator.

scripts/patch_jpegdec.py (4)

54-56: Warn-and-continue allows a silently unpatched build.

If the AC-table patch target is absent, the build continues with the unpatched library, reintroducing the progressive JPEG crash. Consider raise RuntimeError(...) to fail fast.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/patch_jpegdec.py` around lines 54 - 56, The current check using "if
OLD not in content" only prints a warning and returns, allowing a silently
unpatched build; change this to raise a RuntimeError (including filepath and a
clear message) so the script fails fast when the AC-table patch target (OLD) is
missing from content, ensuring the build stops instead of proceeding with an
unpatched library.

---

20-23: Suppress Ruff F821/ARG001 for SCons-injected globals and unused arguments.

Import and env are SCons-injected globals that static analysis cannot resolve, and source/target are unused in patch_jpegdec.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/patch_jpegdec.py` around lines 20 - 23, Add a file-level Ruff
suppression so SCons-injected globals and intentionally unused args are ignored:
insert a top-line directive like "# ruff: noqa: F821, ARG001" above the existing
Import("env") line; keep the Import("env") invocation and the
patch_jpegdec(source, target, env) signature unchanged so SCons behavior
remains, but the linter will no longer flag undefined name Import/env (F821) or
the unused parameters source/target (ARG001).

---

24-31: Use PROJECT_LIBDEPS_DIR and guard against a missing directory.

Hard-coding .pio/libdeps ignores PlatformIO's configurable libdeps_dir. Additionally, os.listdir() on a non-existent path crashes the build.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/patch_jpegdec.py` around lines 24 - 31, Replace the hard-coded
libdeps path and add a directory existence check: use
env.get("PROJECT_LIBDEPS_DIR", os.path.join(env["PROJECT_DIR"], ".pio",
"libdeps")) to set libdeps_dir, then guard with if not
os.path.isdir(libdeps_dir): return (or log and exit) before calling os.listdir;
keep the same logic that iterates env_dir and calls
_apply_ac_table_patch(jpeg_inl) and _apply_mcu_skip_patch(jpeg_inl) when
jpeg.inl is found so the functions (_apply_ac_table_patch,
_apply_mcu_skip_patch) and variable names remain unchanged.

---

85-88: Same warn-and-continue issue for the MCU_SKIP patch.

Same as above – a missing OLD_DC target silently skips patching the JPEGDecodeMCU_P guard, leaving the wild-pointer write intact.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/patch_jpegdec.py` around lines 85 - 88, The current MCU_SKIP patch
check quietly returns when OLD_DC is missing, which can leave the
JPEGDecodeMCU_P guard unpatched; instead, when OLD_DC is not found in content,
stop processing and fail loudly—replace the silent return with an error exit or
raise (e.g., call sys.exit(1) or raise RuntimeError) after printing a clear
error message that includes filepath and mentions the MCU_SKIP/JPEGDecodeMCU_P
patch, so the CI/dev knows the patch did not apply.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

lib/Epub/Epub/converters/JpegToFramebufferConverter.cpp (4)

5-9: Remove the stray null preprocessor directive.
The lone # is likely an accidental artifact and should be removed.

🧹 Proposed fix

 `#include` <JPEGDEC.h>
 `#include` <Logging.h>
-#
 `#include` <cstdlib>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/Epub/Epub/converters/JpegToFramebufferConverter.cpp` around lines 5 - 9,
The top of JpegToFramebufferConverter.cpp has a stray lone preprocessor token
causing a compile error; remove the single `#` line between the include block
(near the includes for JPEGDEC and Logging and the stdlib/new includes) so only
valid include directives remain (e.g., keep `#include` <JPEGDEC.h>, `#include`
<Logging.h>, `#include` <cstdlib>, `#include` <new> and delete the stray `#`).

---

76-89: ⚠️ Potential issue | 🟡 Minor

Don’t update iPos on failed read/seek.
If read() returns an error or seek() fails, iPos becomes inconsistent with the real file position.

🔧 Proposed fix

 int32_t bytesRead = f->read(pBuf, len);
-  pFile->iPos += bytesRead;
-  return bytesRead;
+  if (bytesRead > 0) pFile->iPos += bytesRead;
+  else bytesRead = 0;
+  return bytesRead;
 }
 
 int32_t jpegSeek(JPEGFILE* pFile, int32_t pos) {
   FsFile* f = reinterpret_cast<FsFile*>(pFile->fHandle);
   if (!f) return -1;
-  f->seek(pos);
-  pFile->iPos = pos;
-  return pos;
+  if (!f->seek(pos)) return -1;
+  pFile->iPos = pos;
+  return pos;
 }

#!/bin/bash
# Verify FsFile read/seek return contracts in the repo
rg -nP --type=cpp --type=h -C2 '\bclass\s+FsFile\b'
rg -nP --type=cpp --type=h -C2 '\bFsFile::read\s*\('
rg -nP --type=cpp --type=h -C2 '\bFsFile::seek\s*\('

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/Epub/Epub/converters/JpegToFramebufferConverter.cpp` around lines 76 -
89, The jpegRead and jpegSeek helpers update pFile->iPos unconditionally which
can desync state on errors; change jpegRead (function jpegRead) to call
FsFile::read, check its return value and only add to pFile->iPos when bytesRead
> 0 (handle zero/negative as errors and return as-is), and change jpegSeek
(function jpegSeek) to inspect the return from FsFile::seek (use its
success/position return contract) and only set pFile->iPos when seek succeeded;
do not modify iPos on failure and propagate the error/return value from the
underlying FsFile::read/seek instead.

---

54-61: ⚠️ Potential issue | 🟡 Minor

Guard FsFile allocation failure in jpegOpen.
If allocation fails, dereferencing f can crash on low heap.

🔧 Proposed fix

-  FsFile* f = new FsFile();
-  if (!Storage.openFileForRead("JPG", std::string(filename), *f)) {
+  FsFile* f = new (std::nothrow) FsFile();
+  if (!f || !Storage.openFileForRead("JPG", std::string(filename), *f)) {
     delete f;
     return nullptr;
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/Epub/Epub/converters/JpegToFramebufferConverter.cpp` around lines 54 -
61, The function jpegOpen currently assumes FsFile allocation succeeds and
immediately dereferences f; change allocation to use std::nothrow (FsFile* f =
new (std::nothrow) FsFile()) and check for nullptr before calling
Storage.openFileForRead or dereferencing f; if allocation fails return nullptr
(and set *size to 0 or leave unchanged per convention). Ensure the null-check is
placed before the call to Storage.openFileForRead("JPG", std::string(filename),
*f) and keep existing cleanup (delete f) and return paths intact in jpegOpen.

---

294-326: ⚠️ Potential issue | 🟠 Major

useExactDimensions scales by width only, causing cropping.
Using width-only targetScale and fineScale can silently crop when height is the limiting axis.

⚠️ Partial fix (scale selection only)

-    targetScale = (float)destWidth / srcWidth;
+    float scaleX = (float)destWidth / srcWidth;
+    float scaleY = (float)destHeight / srcHeight;
+    targetScale = (scaleX < scaleY) ? scaleX : scaleY;

A full fix still needs fineScale to respect both axes (e.g., separate X/Y scales or aspect-preserving dest sizing).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/Epub/Epub/converters/JpegToFramebufferConverter.cpp` around lines 294 -
326, The current useExactDimensions branch picks targetScale from width only,
which can crop vertically; update the logic so targetScale and the fine scaling
respect both axes: when useExactDimensions is true, compute destWidth and
destHeight from config.maxWidth/maxHeight but derive separate fineScaleX =
(float)destWidth / ctx.scaledSrcWidth and fineScaleY = (float)destHeight /
ctx.scaledSrcHeight (or compute ctx.fineScale = min(fineScaleX, fineScaleY) and
adjust destWidth/destHeight to preserve aspect) and store these on ctx (e.g.,
ctx.fineScaleX/ctx.fineScaleY or set ctx.fineScale using the min) instead of
using a width-only targetScale; keep the JPEG built-in scaling selection
(isProgressive, chooseJpegScale, JPEG_SCALE_EIGHTH) the same and compute
ctx.scaledSrcWidth/ctx.scaledSrcHeight before deriving the per-axis fine scales.

scripts/patch_jpegdec.py (1)

23-33: ⚠️ Potential issue | 🟡 Minor

Avoid hard-coded .pio/libdeps to respect PlatformIO config.
This path can be customized, so use PROJECT_LIBDEPS_DIR with a fallback.

🔧 Proposed fix

-    libdeps_dir = os.path.join(env["PROJECT_DIR"], ".pio", "libdeps")
+    libdeps_dir = env.get("PROJECT_LIBDEPS_DIR", os.path.join(env["PROJECT_DIR"], ".pio", "libdeps"))
     if not os.path.isdir(libdeps_dir):
         return

#!/bin/bash
# Check for custom libdeps_dir usage and existing PROJECT_LIBDEPS_DIR references
rg -n "libdeps_dir|PROJECT_LIBDEPS_DIR" platformio.ini
rg -n "PROJECT_LIBDEPS_DIR" -g '*.py'

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/patch_jpegdec.py` around lines 23 - 33, The code in patch_jpegdec
currently hardcodes libdeps_dir to os.path.join(env["PROJECT_DIR"], ".pio",
"libdeps"); change it to prefer env["PROJECT_LIBDEPS_DIR"] if set and only fall
back to os.path.join(env["PROJECT_DIR"], ".pio", "libdeps") so PlatformIO
customizations are respected. Update the patch_jpegdec function to obtain
libdeps_dir = env.get("PROJECT_LIBDEPS_DIR", os.path.join(env["PROJECT_DIR"],
".pio", "libdeps")), keep the existing isdir check and loop that looks for
jpeg_inl and invokes _apply_ac_table_patch and _apply_mcu_skip_patch; ensure
behavior is unchanged when the directory is absent.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

lib/Epub/Epub/converters/JpegToFramebufferConverter.cpp (2)

54-61: ⚠️ Potential issue | 🟡 Minor

Guard FsFile allocation failure. Line 55 uses new FsFile() without handling allocation failure; on low heap this can throw or return null depending on build flags.

🔧 Proposed change

-  FsFile* f = new FsFile();
-  if (!Storage.openFileForRead("JPG", std::string(filename), *f)) {
+  FsFile* f = new (std::nothrow) FsFile();
+  if (!f || !Storage.openFileForRead("JPG", std::string(filename), *f)) {
     delete f;
     return nullptr;
   }

You can confirm how new behaves in this build by checking exception flags:

#!/bin/bash
rg -n "fno-exceptions|fexceptions" -g "*.ini" -g "*.txt" -g "*.mk" -g "*.cfg"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/Epub/Epub/converters/JpegToFramebufferConverter.cpp` around lines 54 -
61, The FsFile allocation in jpegOpen is not guarded: change the allocation to
handle allocation failure by either using std::nothrow (e.g., new (std::nothrow)
FsFile()) and checking for nullptr, or wrap the allocation and subsequent
Storage.openFileForRead call in a try/catch to catch std::bad_alloc; if
allocation fails return nullptr and avoid calling Storage.openFileForRead.
Update jpegOpen to validate the FsFile pointer (or catch the exception) before
dereferencing and ensure cleanup on failure.

---

295-327: ⚠️ Potential issue | 🟠 Major

useExactDimensions uses width-only scale, which can crop vertically. Line 298 computes targetScale from width only, while fineScale is uniform; if height is the tighter constraint, the render path silently crops instead of scaling to fit.

🔧 Directional fix (partial) + follow-up

-    targetScale = (float)destWidth / srcWidth;
+    float tScaleX = (float)destWidth / srcWidth;
+    float tScaleY = (float)destHeight / srcHeight;
+    targetScale = (tScaleX < tScaleY) ? tScaleX : tScaleY;

To fully resolve, either (a) compute separate fineScaleX/fineScaleY, or (b) adjust destWidth/destHeight to preserve the chosen aspect ratio so a single fineScale is valid.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@lib/Epub/Epub/converters/JpegToFramebufferConverter.cpp` around lines 295 -
327, The useExactDimensions branch sets destWidth/destHeight to
config.maxWidth/config.maxHeight but computes targetScale using width only,
which can crop vertically; update the logic in the
JpegToFramebufferConverter.cpp block (the branch guarded by
config.useExactDimensions) to either compute separate fineScaleX/fineScaleY and
store both on ctx (e.g., ctx.fineScaleX and ctx.fineScaleY) and downstream use
them, or compute a uniform targetScale from the tighter of width/height
constraints and then adjust destWidth or destHeight to preserve aspect ratio
before computing ctx.fineScale; reference the variables
config.useExactDimensions, destWidth, destHeight, targetScale,
ctx.scaledSrcWidth/Height and ctx.fineScale when making the change.

scripts/patch_jpegdec.py (1)

23-30: ⚠️ Potential issue | 🟡 Minor

Honor PlatformIO’s configurable libdeps_dir. Line 25 hard‑codes .pio/libdeps, so patches won’t run if libdeps_dir is customized. Consider using PROJECT_LIBDEPS_DIR with a fallback.

🔧 Proposed change

-    libdeps_dir = os.path.join(env["PROJECT_DIR"], ".pio", "libdeps")
+    libdeps_dir = env.get("PROJECT_LIBDEPS_DIR",
+                          os.path.join(env["PROJECT_DIR"], ".pio", "libdeps"))
     if not os.path.isdir(libdeps_dir):
         return

To verify whether libdeps_dir is customized in this repo, you can run:

#!/bin/bash
rg -n "libdeps_dir" -g "platformio.ini" -g "*.ini"

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/patch_jpegdec.py` around lines 23 - 30, The patch_jpegdec function
currently hardcodes libdeps_dir to os.path.join(env["PROJECT_DIR"], ".pio",
"libdeps"); change it to honor PlatformIO's configurable libdeps_dir by reading
env["PROJECT_LIBDEPS_DIR"] with a sensible fallback to the existing path. Update
the libdeps_dir assignment in patch_jpegdec to prefer
env.get("PROJECT_LIBDEPS_DIR") (or similar) and only fall back to
os.path.join(env["PROJECT_DIR"], ".pio", "libdeps") if that key is missing, so
the subsequent logic that looks for JPEGDEC/src/jpeg.inl works with customized
setups.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro