Fix CBR preview: replace internal RAR parser with UnRAR SDK dynamic loading#5
Fix CBR preview: replace internal RAR parser with UnRAR SDK dynamic loading#5
Conversation
…oading Co-authored-by: theotocopulitos <1540135+theotocopulitos@users.noreply.github.com>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds runtime UnRAR DLL integration: new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RarArchive
participant UnRarDll
participant UnrarDLL
Client->>RarArchive: Open(filename)
RarArchive->>UnRarDll: EnsureLoaded()
UnRarDll->>UnrarDLL: LoadLibrary / GetProcAddresses
alt DLL loaded
RarArchive->>UnrarDLL: RAROpenArchiveEx(mode=LIST)
UnrarDLL-->>RarArchive: archive handle / success
RarArchive->>UnrarDLL: RARReadHeaderEx / iterate
UnrarDLL-->>RarArchive: RARHeaderDataEx entries
Client->>RarArchive: ReadRarFileHeader(target)
RarArchive->>UnrarDLL: RAROpenArchiveEx(mode=EXTRACT)
RarArchive->>UnrarDLL: RARProcessFileW(EXTRACT, callback)
UnrarDLL->>RarArchive: UnRarCallback(data chunks)
RarArchive-->>Client: file bytes
RarArchive->>UnrarDLL: RARCloseArchive()
else DLL missing
RarArchive-->>Client: empty listing / graceful fallback
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
dll.hpp (1)
71-128: Preserve the caller's packing state.Use
#pragma pack(push, 1)/#pragma pack(pop)here. The current#pragma pack(1)/#pragma pack()pair resets global packing instead of restoring the includer's previous state, which is risky in a public ABI header.Safer packing wrapper
-#pragma pack(1) +#pragma pack(push, 1) ... -#pragma pack() +#pragma pack(pop)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dll.hpp` around lines 71 - 128, The header currently forces global packing with `#pragma` pack(1)/#pragma pack(), which can break an includer's packing; change to a scoped packing pair by replacing that sequence with `#pragma` pack(push, 1) before the definitions of RARHeaderDataEx and RAROpenArchiveDataEx and `#pragma` pack(pop) after them so the caller's previous packing state is preserved while keeping these structs packed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dll.hpp`:
- Around line 7-8: dll.hpp currently uses Win32 types (CALLBACK, UINT, LPARAM,
HANDLE, PASCAL) but doesn't include their declarations; make the header
self-contained by adding the appropriate Windows type declarations—e.g., wrap an
`#include` <windows.h> (or the narrower <windef.h> / <winnt.h>) in an `#ifdef`
_WIN32 guard at the top of dll.hpp so symbols referenced in the callback and API
prototypes (search for CALLBACK, UINT, LPARAM, HANDLE, PASCAL in the file) are
defined when the header is included without relying on pch.h.
In `@RarArchive.cpp`:
- Around line 24-55: The bug is that Load() publishes hDll early allowing other
threads to see a non-null hDll before the function pointers (pfnOpenArchiveEx,
pfnCloseArchive, pfnReadHeaderEx, pfnProcessFileW, pfnSetCallback) are
initialized; fix by delaying publication of hDll: call ::LoadLibraryW into a
local HMODULE (e.g. tmpH), resolve all GetProcAddress results into local
function-pointer variables (or the existing pfn* members only after success), if
any lookup fails call ::FreeLibrary(tmpH) and return false, and only after all
pointers are valid assign hDll = tmpH (or use an atomic store) so other threads
never see a non-null hDll with uninitialized pfn* values; also ensure you
clear/reset the pfn* members on failure as currently done.
- Around line 105-109: HeaderFileName() currently computes wlen with
MultiByteToWideChar which returns the count including the terminating NUL, but
then resizes name to wlen - 1 and calls MultiByteToWideChar again with buffer
size wlen, allowing a one-wchar overflow; fix by allocating enough space and
removing the NUL afterwards: resize name to wlen (not wlen - 1) before calling
::MultiByteToWideChar(CP_ACP, 0, hdr.FileName, -1, &name[0], wlen) and then, if
the last character is L'\0', pop it (or resize to wlen - 1) so name contains the
string without the terminating NUL; reference HeaderFileName(), name,
hdr.FileName and the MultiByteToWideChar calls when making the change.
- Around line 28-29: The code loads unrar.dll via LoadLibraryW(L"unrar.dll")
which searches the process DLL paths instead of the extension's directory;
change the logic around hDll so you first call GetModuleFileName for the current
module (the ComicTooltipExt DLL), compute the directory and append
L"\\unrar.dll" to form a full path, and then call LoadLibraryW with that full
path (use the result to set hDll and keep the existing !hDll check). Ensure you
use the module handle for the extension when calling GetModuleFileName so the
lookup locates unrar.dll placed next to ComicTooltipExt.dll.
- Around line 284-303: The loop in ParseRarHeader and the similar logic in
ReadRarFileHeader currently ignore g_unrar.pfnProcessFileW return values and
derive overall success from m_fileList.empty()/data.empty(), which can hide
partial reads or SDK errors (e.g., password-protected or corrupt archives);
update ParseRarHeader() to check the return code of g_unrar.pfnProcessFileW
after each header iteration and break/return a failure on any non-success
status, use the final g_unrar.pfnReadHeaderEx return code (ret) to determine why
the loop ended (and treat non-ERAR_END_ARCHIVE as an error), and apply the same
pattern in ReadRarFileHeader() for operations that use RAR_TEST so success is
keyed off the SDK return codes rather than non-empty data; ensure errors from
the UnRAR SDK are propagated up (do not convert partial data into success) and
preserve correct handling for truly empty but valid archives.
In `@README.md`:
- Around line 74-75: Update the README statement that currently claims "no
external dependencies" to reflect the runtime dependency on the RARLAB UnRAR DLL
(unrar.dll) for CBR support: replace the absolute claim with a caveat like "no
compile-time dependencies; CBR preview requires the RARLAB UnRAR DLL (unrar.dll)
at runtime — otherwise CBRs degrade to filename/size tooltips" and apply the
same change where the duplicate claim appears (the other occurrence of "no
external dependencies"). Ensure the wording also mentions that the DLL is loaded
dynamically and that absence triggers graceful degradation.
---
Nitpick comments:
In `@dll.hpp`:
- Around line 71-128: The header currently forces global packing with `#pragma`
pack(1)/#pragma pack(), which can break an includer's packing; change to a
scoped packing pair by replacing that sequence with `#pragma` pack(push, 1) before
the definitions of RARHeaderDataEx and RAROpenArchiveDataEx and `#pragma`
pack(pop) after them so the caller's previous packing state is preserved while
keeping these structs packed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 11bb2273-18eb-48f5-b3c0-16568c574bf9
📒 Files selected for processing (4)
ComicTooltipExt.vcxprojREADME.mdRarArchive.cppdll.hpp
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Verify each finding against the current code and only fix it if needed. Inline comments:
In
In
Nitpick comments:
|
…e <windows.h> Co-authored-by: theotocopulitos <1540135+theotocopulitos@users.noreply.github.com>
Fixed in 0cbc2db. |
|
Verify each finding against the current code and only fix it if needed. Inline comments: In @RarArchive.cpp: Around line 24-55: The bug is that Load() publishes hDll early allowing other |
…re resolved Co-authored-by: theotocopulitos <1540135+theotocopulitos@users.noreply.github.com>
Fixed in 62b2557. |
|
Verify each finding against the current code and only fix it if needed. In @RarArchive.cpp: Around line 105-109: HeaderFileName() currently computes wlen with Around line 284-303: The loop in ParseRarHeader and the similar logic in |
…SDK error propagation Co-authored-by: theotocopulitos <1540135+theotocopulitos@users.noreply.github.com>
Fixed in d56ca67. Three changes made:
|
|
@copilot Around line 74-75: Update the README statement that currently claims "no |
Co-authored-by: theotocopulitos <1540135+theotocopulitos@users.noreply.github.com>
... Fixed in 8f45abb. The Security section's "No network access or external dependencies" line has been updated to:
The archive-support bullet at line 74 already correctly described the UnRAR SDK dependency, so no change was needed there. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
RarArchive.cpp (1)
349-356:⚠️ Potential issue | 🟡 MinorDon't collapse valid empty archives into corruption.
After the
ret != ERAR_END_ARCHIVEcheck, the SDK has already reported a clean end-of-archive.return !m_fileList.empty()still makes an empty but valid RAR failOpen().♻️ Suggested fix
- return !m_fileList.empty(); + return true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@RarArchive.cpp` around lines 349 - 356, The current logic treats an empty but properly-terminated archive as a failure because after checking if (ret != ERAR_END_ARCHIVE) it returns !m_fileList.empty(); change this so that when ret == ERAR_END_ARCHIVE the function returns true (a valid, possibly-empty archive) and only returns false when ret != ERAR_END_ARCHIVE; update the code around the ERAR_END_ARCHIVE check (the ret variable and m_fileList handling in RarArchive::Open()) to avoid collapsing valid empty archives into an error.
🧹 Nitpick comments (1)
dll.hpp (1)
72-129: Use#pragma pack(push, 1)and#pragma pack(pop)for defensive consistency.While the risk here is limited (dll.hpp is only directly included in RarArchive.cpp and no structs follow the final
#pragma pack()reset), using scoped packing follows the UnRAR SDK best practice and prevents accidental layout issues if the header is later included elsewhere or extended. The suggested change is straightforward and costs nothing:♻️ Suggested fix
-#pragma pack(1) +#pragma pack(push, 1) struct RARHeaderDataEx {-#pragma pack() +#pragma pack(pop)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dll.hpp` around lines 72 - 129, Replace the unscoped packing pragmas with scoped ones: wrap the packed struct definitions for RARHeaderDataEx and RAROpenArchiveDataEx with `#pragma` pack(push, 1) before the structs and `#pragma` pack(pop) after them to ensure packing is restored safely; update the existing bare `#pragma` pack(1) and final `#pragma` pack() to use these push/pop pragmas so the struct layouts remain 1-byte aligned without risking accidental global packing changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@RarArchive.cpp`:
- Around line 28-93: The Load() method has a race: the unguarded if (hDll)
return true; and subsequent assignments to hDll and pfn* can be executed
concurrently causing double LoadLibraryW calls and torn publishes; fix by
serializing initialization (e.g. add a std::once_flag and use std::call_once or
use Windows INIT_ONCE) around the whole initialization body in Load() so only
one thread performs LoadLibraryW/GetProcAddress and assigns pfnOpenArchiveEx,
pfnCloseArchive, pfnReadHeaderEx, pfnProcessFileW, pfnSetCallback and hDll;
ensure any temporary module (tmpH) is freed on early failure and keep the public
check of hDll only after call_once completes to preserve the intended atomic
publish.
- Around line 107-124: The extraction currently allows unbounded allocation from
archive-controlled sizes: before calling data.reserve(fileInfo.FileSize) (the
code that handles per-entry allocation) check fileInfo.FileSize against a
defined MAX_ENTRY_SIZE and abort processing of that entry if it exceeds the cap;
similarly, extend ExtractionContext with a cumulative size counter and enforce a
MAX_TOTAL_BUFFER (or reuse the same MAX_ENTRY_SIZE) inside UnRarCallback to
refuse/appended data when ctx->currentSize + size > MAX_TOTAL_BUFFER, returning
a failure status (and cleaning up) instead of appending; update places using
ExtractionContext and UnRarCallback to handle the error return so oversized
entries are rejected early and cannot exhaust memory.
---
Duplicate comments:
In `@RarArchive.cpp`:
- Around line 349-356: The current logic treats an empty but properly-terminated
archive as a failure because after checking if (ret != ERAR_END_ARCHIVE) it
returns !m_fileList.empty(); change this so that when ret == ERAR_END_ARCHIVE
the function returns true (a valid, possibly-empty archive) and only returns
false when ret != ERAR_END_ARCHIVE; update the code around the ERAR_END_ARCHIVE
check (the ret variable and m_fileList handling in RarArchive::Open()) to avoid
collapsing valid empty archives into an error.
---
Nitpick comments:
In `@dll.hpp`:
- Around line 72-129: Replace the unscoped packing pragmas with scoped ones:
wrap the packed struct definitions for RARHeaderDataEx and RAROpenArchiveDataEx
with `#pragma` pack(push, 1) before the structs and `#pragma` pack(pop) after them
to ensure packing is restored safely; update the existing bare `#pragma` pack(1)
and final `#pragma` pack() to use these push/pop pragmas so the struct layouts
remain 1-byte aligned without risking accidental global packing changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4799e25f-59e5-4cd7-93bd-2c19ba5b52d4
📒 Files selected for processing (4)
ComicTooltipExt.vcxprojREADME.mdRarArchive.cppdll.hpp
| bool Load() | ||
| { | ||
| if (hDll) return true; | ||
|
|
||
| // Build a full path to unrar.dll placed alongside ComicTooltipExt.dll | ||
| // so we never accidentally load an unrar.dll from an arbitrary location | ||
| // on the process DLL search path. | ||
| wchar_t dllPath[MAX_PATH] = {}; | ||
| HMODULE hSelf = NULL; | ||
| if (::GetModuleHandleExW( | ||
| GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS | | ||
| GET_MODULE_HANDLE_EX_FLAG_UNCHANGED_REFCOUNT, | ||
| reinterpret_cast<LPCWSTR>(UnRarCallback), // address inside this DLL | ||
| &hSelf) && hSelf) | ||
| { | ||
| DWORD len = ::GetModuleFileNameW(hSelf, dllPath, MAX_PATH); | ||
| if (len > 0 && len < MAX_PATH) | ||
| { | ||
| wchar_t* lastSlash = wcsrchr(dllPath, L'\\'); | ||
| if (lastSlash) | ||
| { | ||
| // Replace filename portion with unrar.dll. | ||
| // Guard against arithmetic underflow before computing remaining. | ||
| ptrdiff_t offset = lastSlash + 1 - dllPath; | ||
| if (offset > 0 && static_cast<size_t>(offset) <= MAX_PATH) | ||
| { | ||
| size_t remaining = MAX_PATH - static_cast<size_t>(offset); | ||
| if (wcscpy_s(lastSlash + 1, remaining, L"unrar.dll") != 0) | ||
| dllPath[0] = L'\0'; // on failure fall back to bare name | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| HMODULE tmpH = ::LoadLibraryW(dllPath[0] ? dllPath : L"unrar.dll"); | ||
| if (!tmpH) return false; | ||
|
|
||
| PFN_RAROpenArchiveEx tmpOpenArchiveEx = reinterpret_cast<PFN_RAROpenArchiveEx>( | ||
| ::GetProcAddress(tmpH, "RAROpenArchiveEx")); | ||
| PFN_RARCloseArchive tmpCloseArchive = reinterpret_cast<PFN_RARCloseArchive>( | ||
| ::GetProcAddress(tmpH, "RARCloseArchive")); | ||
| PFN_RARReadHeaderEx tmpReadHeaderEx = reinterpret_cast<PFN_RARReadHeaderEx>( | ||
| ::GetProcAddress(tmpH, "RARReadHeaderEx")); | ||
| PFN_RARProcessFileW tmpProcessFileW = reinterpret_cast<PFN_RARProcessFileW>( | ||
| ::GetProcAddress(tmpH, "RARProcessFileW")); | ||
| PFN_RARSetCallback tmpSetCallback = reinterpret_cast<PFN_RARSetCallback>( | ||
| ::GetProcAddress(tmpH, "RARSetCallback")); | ||
|
|
||
| if (!tmpOpenArchiveEx || !tmpCloseArchive || !tmpReadHeaderEx || | ||
| !tmpProcessFileW || !tmpSetCallback) | ||
| { | ||
| ::FreeLibrary(tmpH); | ||
| return false; | ||
| } | ||
|
|
||
| // All pointers resolved — publish atomically: hDll becomes non-null | ||
| // only after every pfn* member is valid, so other threads can never | ||
| // observe a non-null hDll with uninitialized function pointers. | ||
| pfnOpenArchiveEx = tmpOpenArchiveEx; | ||
| pfnCloseArchive = tmpCloseArchive; | ||
| pfnReadHeaderEx = tmpReadHeaderEx; | ||
| pfnProcessFileW = tmpProcessFileW; | ||
| pfnSetCallback = tmpSetCallback; | ||
| hDll = tmpH; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "RarArchive.cpp" -o -name "RarArchive.h" | head -20Repository: theotocopulitos/comicInfoTooltip
Length of output: 107
🏁 Script executed:
rg -l "RarArchive" --type cpp --type h | head -20Repository: theotocopulitos/comicInfoTooltip
Length of output: 169
🏁 Script executed:
fd -e cpp -e h | grep -i rarRepository: theotocopulitos/comicInfoTooltip
Length of output: 103
🏁 Script executed:
cat -n RarArchive.hRepository: theotocopulitos/comicInfoTooltip
Length of output: 2142
🏁 Script executed:
cat -n RarArchive.cppRepository: theotocopulitos/comicInfoTooltip
Length of output: 18633
🏁 Script executed:
# Verify if there are any synchronization mechanisms I may have missed
rg -A 5 -B 5 "INIT_ONCE|call_once|mutex|critical|lock" RarArchive.cppRepository: theotocopulitos/comicInfoTooltip
Length of output: 58
🏁 Script executed:
# Check for volatile or atomic usage on hDll/pfn*
rg "volatile|atomic" RarArchive.cppRepository: theotocopulitos/comicInfoTooltip
Length of output: 152
🏁 Script executed:
# Check where g_unrar.Load() is called from
rg -B 3 "g_unrar\.Load()" RarArchive.cppRepository: theotocopulitos/comicInfoTooltip
Length of output: 285
Add synchronization to Load() — it has a data race.
The if (hDll) return true; check at line 30 is unguarded. Two threads can both pass this check, call LoadLibraryW() twice (leaking a refcount), and race to write hDll and the pfn* pointers without synchronization. The comment claiming "atomic" publication is incorrect—plain member assignments have no synchronicity guarantee. In the multi-threaded context of Explorer/prevhost.exe, wrap the entire initialization in INIT_ONCE (Windows) or std::call_once (C++11).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@RarArchive.cpp` around lines 28 - 93, The Load() method has a race: the
unguarded if (hDll) return true; and subsequent assignments to hDll and pfn* can
be executed concurrently causing double LoadLibraryW calls and torn publishes;
fix by serializing initialization (e.g. add a std::once_flag and use
std::call_once or use Windows INIT_ONCE) around the whole initialization body in
Load() so only one thread performs LoadLibraryW/GetProcAddress and assigns
pfnOpenArchiveEx, pfnCloseArchive, pfnReadHeaderEx, pfnProcessFileW,
pfnSetCallback and hDll; ensure any temporary module (tmpH) is freed on early
failure and keep the public check of hDll only after call_once completes to
preserve the intended atomic publish.
| struct ExtractionContext | ||
| { | ||
| std::vector<BYTE>* pData = nullptr; | ||
| }; | ||
|
|
||
| static int CALLBACK UnRarCallback(UINT msg, LPARAM userData, LPARAM p1, LPARAM p2) | ||
| { | ||
| if (msg == UCM_PROCESSDATA) | ||
| { | ||
| ExtractionContext* ctx = reinterpret_cast<ExtractionContext*>(userData); | ||
| if (ctx && ctx->pData) | ||
| { | ||
| const BYTE* ptr = reinterpret_cast<const BYTE*>(p1); | ||
| size_t size = static_cast<size_t>(p2); | ||
| ctx->pData->insert(ctx->pData->end(), ptr, ptr + size); | ||
| } | ||
| } | ||
| return 1; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "RarArchive.cpp" -type fRepository: theotocopulitos/comicInfoTooltip
Length of output: 92
🏁 Script executed:
cat -n RarArchive.cppRepository: theotocopulitos/comicInfoTooltip
Length of output: 18633
🏁 Script executed:
rg -i "max|limit|size.*cap" RarArchive.cppRepository: theotocopulitos/comicInfoTooltip
Length of output: 505
🏁 Script executed:
sed -n '363,412p' RarArchive.cpp | head -50Repository: theotocopulitos/comicInfoTooltip
Length of output: 1901
Implement hard caps on buffered entry size to prevent resource exhaustion.
The extraction flow at lines 369–370 and the callback at line 121 buffer entire RAR entries without size validation. fileInfo.FileSize is populated from the archive header's UnpSize (archive-controlled), and data.reserve(fileInfo.FileSize) can allocate gigabytes based on attacker input. The callback then appends chunks unbounded via vector::insert(). A crafted CBR can force multi-GB allocation attempts, causing out-of-memory conditions in the preview host. Reject oversized entries before processing and enforce a maximum cumulative buffer size in the callback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@RarArchive.cpp` around lines 107 - 124, The extraction currently allows
unbounded allocation from archive-controlled sizes: before calling
data.reserve(fileInfo.FileSize) (the code that handles per-entry allocation)
check fileInfo.FileSize against a defined MAX_ENTRY_SIZE and abort processing of
that entry if it exceeds the cap; similarly, extend ExtractionContext with a
cumulative size counter and enforce a MAX_TOTAL_BUFFER (or reuse the same
MAX_ENTRY_SIZE) inside UnRarCallback to refuse/appended data when
ctx->currentSize + size > MAX_TOTAL_BUFFER, returning a failure status (and
cleaning up) instead of appending; update places using ExtractionContext and
UnRarCallback to handle the error return so oversized entries are rejected early
and cannot exhaust memory.
|
Verify each finding against the current code and only fix it if needed. Inline comments:
Duplicate comments:
Nitpick comments:
|
…, scoped pack pragmas Co-authored-by: theotocopulitos <1540135+theotocopulitos@users.noreply.github.com>
Fixed in 1b423d0. Four changes made:
|
Review Summary by QodoFix CBR preview by replacing internal RAR parser with UnRAR SDK
WalkthroughsDescription• Replace internal RAR parser with UnRAR SDK dynamic loading for RAR4/RAR5 support • Implement thread-safe DLL loading with allocation safety caps (64 MB limits) • Add callback-based file extraction with decompressed data accumulation • Update project configuration and documentation for unrar.dll runtime dependency Diagramflowchart LR
A["CBR File"] -->|"Open with UnRAR SDK"| B["RAROpenArchiveEx"]
B -->|"List/Extract"| C["RARReadHeaderEx"]
C -->|"Process entries"| D["RARProcessFileW"]
D -->|"Decompress callback"| E["UnRarCallback"]
E -->|"Accumulate data"| F["ExtractionContext"]
F -->|"Return file list/data"| G["RarArchive"]
H["unrar.dll"] -.->|"Dynamic load"| B
File Changes1. RarArchive.cpp
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR replaces the custom CBR (RAR) parsing/extraction logic with runtime dynamic loading of the RARLAB UnRAR SDK (unrar.dll) to enable proper RAR4/RAR5 support in the shell extension’s tooltip/preview pipeline.
Changes:
- Added
dll.hppwith UnRAR SDK declarations and updated the VS project to include it. - Rewrote
RarArchive.cppto enumerate/extract entries via UnRAR (RAROpenArchiveEx/RARReadHeaderEx/RARProcessFileW) with init-once loading and extraction size caps. - Updated
README.mdto document the runtime dependency and troubleshooting guidance for blank CBR previews.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| dll.hpp | Adds UnRAR SDK struct/function-pointer declarations for dynamic loading. |
| RarArchive.cpp | Implements dynamic DLL loading, archive listing, and extraction via UnRAR callbacks. |
| README.md | Documents unrar.dll runtime requirement and behavior when absent. |
| ComicTooltipExt.vcxproj | Adds dll.hpp to the project includes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // RarFileInfo.FileSize is DWORD (32-bit); images in comic archives | ||
| // are never ≥ 4 GB so storing the lower 32 bits of UnpSize is safe. | ||
| fi.FileSize = hdr.UnpSize; | ||
| fi.CompressedSize = hdr.PackSize; |
There was a problem hiding this comment.
Sizes are truncated to 32-bit (hdr.UnpSize/hdr.PackSize) and UnpSizeHigh/PackSizeHigh are ignored. This can misreport entry sizes and bypass the MAX_ENTRY_BYTES pre-check when the high dword is non-zero. Consider combining high/low into a 64-bit size and rejecting entries whose true uncompressed size exceeds the safety cap.
There was a problem hiding this comment.
Fixed in bdf7f97. In ParseRarHeader(), hdr.UnpSize and hdr.PackSize are now combined with their high-dword counterparts before the size cap is applied:
uint64_t unpSize64 = (static_cast<uint64_t>(hdr.UnpSizeHigh) << 32) | hdr.UnpSize;
uint64_t packSize64 = (static_cast<uint64_t>(hdr.PackSizeHigh) << 32) | hdr.PackSize;Entries where unpSize64 > MAX_ENTRY_BYTES are skipped (not added to m_fileList). Only after the 64-bit check passes are the low 32 bits written to fi.FileSize and fi.CompressedSize — at that point truncation is safe since the value is guaranteed ≤ 64 MB.
| // RAR_TEST decompresses into the callback; nothing is written to disk. | ||
| // Key success off the SDK return code, not data.empty(), so that a | ||
| // legitimately zero-byte entry is handled correctly. | ||
| int procRet = g_unrar.pfnProcessFileW(hArc, RAR_TEST, NULL, NULL); | ||
| found = true; |
There was a problem hiding this comment.
ReadRarFileHeader() can return false after partially appending bytes via the callback (e.g., when MAX_TOTAL_BYTES is exceeded). Since ExtractFile() ignores the boolean return, callers may still receive truncated/invalid image data. Clear data on failure and/or make ExtractFile() return an empty vector when ReadRarFileHeader() fails.
There was a problem hiding this comment.
Fixed in 323109d. Two changes:
ReadRarFileHeader()now callsdata.clear()before returningfalse, ensuring no partial bytes survive an error (including mid-streamMAX_TOTAL_BYTESexhaustion).ExtractFile()now checks theReadRarFileHeader()boolean return and callsresult.clear()on failure, so callers always receive either a complete, valid buffer or an empty vector.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…heck in ParseRarHeader() Co-authored-by: theotocopulitos <1540135+theotocopulitos@users.noreply.github.com>
… empty vector on error Co-authored-by: theotocopulitos <1540135+theotocopulitos@users.noreply.github.com>
…rt UTF-16/UTF-8 BOMs in ComicInfo.xml; match ComicInfo.xml case-insensitively by basename only
… extension in IsCBZ/IsCBR methods
dll.hpp— standard RARLAB UnRAR SDK headerRarArchive.cpp— UnRAR SDK dynamic loading (RAR4 + RAR5 support)ComicTooltipExt.vcxproj— adddll.hppto projectClIncludeitemsREADME.md— documentunrar.dllruntime dependencydll.hpp— remove literal backtick characters corrupting#pragma onceand#include <windows.h>RarArchive.cppLoad()race condition — delayhDllpublication until all function pointers resolvedHeaderFileName()off-by-one: resize towlen, callMultiByteToWideChar, thenpop_back()trailingL'\0'Load()DLL path: useGetModuleHandleEx(FROM_ADDRESS)+GetModuleFileNameWto build full path tounrar.dllpfnProcessFileWreturn values; treat non-ERAR_END_ARCHIVEas errorLoad()init-once race: serialize withstd::once_flag/std::call_once; only one thread ever callsLoadLibraryWreserve()and callback accumulation atMAX_ENTRY_BYTES/MAX_TOTAL_BYTES(64 MB each)ParseRarHeader(): returntruefor a valid empty archive whenret == ERAR_END_ARCHIVEdll.hpp#pragma pack: replace barepack(1)/pack()with scopedpack(push,1)/pack(pop)ParseRarHeader(): combineUnpSizeHigh/UnpSizeandPackSizeHigh/PackSizeinto 64-bit values before theMAX_ENTRY_BYTEScheckReadRarFileHeader()clearsdatabefore returningfalse;ExtractFile()checks the return value and returns an empty vector on failureOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes