@coderabbitai[bot] commented on this pull request.
**Actionable comments posted: 10**
<details>
<summary>π§Ή Nitpick comments (6)</summary><blockquote>
<details>
<summary>lib/package.cc (2)</summary><blockquote>
`393-407`: _π©Ί Stability & Availability_ | _π΅ Trivial_ | _π€ Low value_
**Consider heap allocation for the 128 KiB copy buffer.**
`UNCOMPRESS_BUFSIZE` is 128 KiB. `copyBytes`, `copyPayloadAlt`, and
`rangeDigest` each place that buffer on the stack. Library code can run on
threads with small stacks, so a static-sized 128 KiB frame is risky. Use a heap
buffer or reduce the size.
<details>
<summary>π€ Prompt for AI Agents</summary>
```
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/package.cc` around lines 393 - 407, Replace the stack-allocated buf in
copyBytes with a heap-allocated 128 KiB buffer, ensuring allocation failure
returns an error and the buffer is released on every exit path. Apply the same
stack-buffer change to copyPayloadAlt and rangeDigest, preserving their existing
copy and digest behavior.
```
</details>
<!-- cr-comment:v1:14f3f1b3597caacaf3b5eccf -->
---
`727-742`: _π Maintainability & Code Quality_ | _π΅ Trivial_ | _π€ Low value_
**Make the reserved-space adjustment arithmetic explicit.**
`wantSize - curSize` uses `unsigned` arithmetic. When `curSize > wantSize`, the
expression wraps and `utd.count +=` relies on modular arithmetic to shrink the
count. The preceding guard makes the result correct, but the intent is hard to
verify. Compute the signed delta and branch explicitly.
<details>
<summary>β»οΈ Suggested clarification</summary>
```diff
- utd.count += wantSize - curSize;
+ if (curSize > wantSize)
+ utd.count -= curSize - wantSize;
+ else
+ utd.count += wantSize - curSize;
```
</details>
<details>
<summary>π€ Prompt for AI Agents</summary>
```
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/package.cc` around lines 727 - 742, Update the reserved-space
adjustment
loop around headerGet and utd.count so it computes an explicit signed delta
between wantSize and curSize, branches separately for growing versus shrinking
reserved space, and preserves the existing guard that stops when shrinking
cannot be satisfied. Avoid relying on unsigned wraparound in the utd.count
update while retaining the current headerMod behavior.
```
</details>
<!-- cr-comment:v1:0b9716567d635a2cff9a9eed -->
</blockquote></details>
<details>
<summary>lib/cpio.cc (1)</summary><blockquote>
`252-276`: _ποΈ Data Integrity & Integration_ | _π΅ Trivial_ | _β‘ Quick win_
**Enforce the `fileend` invariant in the fast write path.**
Every other writer in this file checks `cpio->fileend` against `cpio->offset`.
`rpmcpioHeaderWrite` and `rpmcpioStrippedHeaderWrite` reject a mismatch, and
`rpmcpioWrite` clamps the write to `fileend - offset`. `rpmcpioWriteFile`
applies neither. It writes exactly `size` bytes and advances `cpio->offset` by
`size`.
If `size` does not equal `cpio->fileend - cpio->offset`, the fast path emits a
member whose content length disagrees with the header `filesize`. The fallback
path would have clamped instead. The archive then desynchronizes at the next
header, and the corruption is only detected at read time.
Add the same check the other writers use.
<details>
<summary>π‘οΈ Proposed check</summary>
```diff
/* Absolute offset in the package where this file's content begins. */
if (size < 0 || cpio->base > maxoff - cpio->offset)
return RPMCPIO_COPY_FALLBACK;
+ /* The header already fixed the member length; refuse a mismatch. */
+ if (cpio->offset + size != cpio->fileend)
+ return RPMERR_WRITE_FAILED;
pos = cpio->base + cpio->offset;
```
</details>
<details>
<summary>π€ Prompt for AI Agents</summary>
```
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/cpio.cc` around lines 252 - 276, Update the fast path in
rpmcpioWriteFile
to require size == cpio->fileend - cpio->offset before flushing or copying;
reject mismatches using the same failure behavior as rpmcpioHeaderWrite,
rpmcpioStrippedHeaderWrite, and rpmcpioWrite. Preserve the existing copy and
offset-advance logic for valid sizes.
```
</details>
<!-- cr-comment:v1:b83cc12bebca09ad530a6ee7 -->
</blockquote></details>
<details>
<summary>build/pack.cc (1)</summary><blockquote>
`88-88`: _π Maintainability & Code Quality_ | _π΅ Trivial_ | _β‘ Quick win_
**Derive `uncompressed` once and pass it down.**
`writeRPM` computes the same predicate at Line 494 with the same
`strstr(rpmio_flags, "ufdio")` expression, and `getIOFlags` already classifies
the same string with `rstreq(s+1, "ufdio")`. Three independent derivations
decide whether digests are taken during the write pass or the post-write pass.
If they ever disagree, the ALT digest is either computed twice or never, and
the package ships a wrong `RPMTAG_PAYLOADSHA*ALT` value. Pass the
already-computed flag into `cpio_doio` instead.
<details>
<summary>π€ Prompt for AI Agents</summary>
```
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@build/pack.cc` at line 88, Compute the uncompressed flag once in the
existing
caller and pass it through to cpio_doio, removing its local strstr-based
derivation. Update writeRPM to reuse the same propagated flag rather than
recomputing the "ufdio" predicate, while preserving the existing digest timing
behavior.
```
</details>
<!-- cr-comment:v1:77359598f48b19d78f22ac6a -->
</blockquote></details>
<details>
<summary>lib/rpmchecksig.cc (1)</summary><blockquote>
`140-151`: _π Performance & Scalability_ | _π΅ Trivial_ | _β‘ Quick win_
**Import the main header only when the payload is framed.**
`readPayload` runs on every package read through `rpmpkgRead`. It calls
`hdrblobImport` before it reads the first payload byte. The imported header
supplies only `align`, which is used only in the framed branch that starts at
Line 171. Compressed payloads and unframed raw payloads return at Line 162
without ever using `align`. Move the import after the first-byte check to
remove a header import and free from the common path.
The fallback semantics stay the same. The first byte already passes through the
fd digest bundle, so a later import failure can still hand the remainder to
`readFile`.
<details>
<summary>β»οΈ Proposed reordering</summary>
```diff
static int readPayload(struct rpmvs_s *vs, FD_t fd, hdrblob blob, char **msg)
{
Header h = NULL;
char *importmsg = NULL;
off_t start = Ftell(fd);
off_t aligned = 0;
unsigned char first;
char magic[4];
int rc = 1;
- /*
- * An unimportable main header carries no usable alignment. Digest the
- * payload as stored so verification reports the header damage itself.
- */
- if (hdrblobImport(blob, 0, &h, &importmsg) != RPMRC_OK) {
- free(importmsg);
- return readFile(fd, msg);
- }
- uint64_t align = headerGetNumber(h, RPMTAG_PAYLOADALIGNMENT);
-
/* This byte also enters all attached rpmvs payload digest contexts. */
ssize_t plen = (start < 0) ? -1 : Fread(&first, 1, 1, fd);
if (plen < 0) {
rasprintf(msg, _("Fread failed: %s"), Fstrerror(fd));
goto exit;
}
/* A missing payload still digests as stored. */
if (plen == 0) {
rc = 0;
goto exit;
}
if (first != '\0') {
if (readFile(fd, msg) == 0)
rc = 0;
goto exit;
}
+ /*
+ * An unimportable main header carries no usable alignment. Digest the
+ * payload as stored so verification reports the header damage itself.
+ */
+ if (hdrblobImport(blob, 0, &h, &importmsg) != RPMRC_OK) {
+ free(importmsg);
+ if (readFile(fd, msg) == 0)
+ rc = 0;
+ goto exit;
+ }
+ uint64_t align = headerGetNumber(h, RPMTAG_PAYLOADALIGNMENT);
+
/*
* A leading NUL is outer framing. The alignment tag fixes the only valid
* cpio start; the first framing byte was already consumed.
*/
```
</details>
<details>
<summary>π€ Prompt for AI Agents</summary>
```
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/rpmchecksig.cc` around lines 140 - 151, In readPayload, move the
hdrblobImport call and its importmsg cleanup into the framed-payload branch
after the first-byte check, before align is read or used. Keep compressed and
unframed paths returning through their existing logic without importing the
header, and preserve the import-failure fallback to readFile(fd, msg) after the
first byte has been consumed.
```
</details>
<!-- cr-comment:v1:a8b75d798707bfa57bf5f21c -->
</blockquote></details>
<details>
<summary>lib/rpmfi.cc (1)</summary><blockquote>
`2017-2022`: _π Maintainability & Code Quality_ | _π΅ Trivial_ | _β‘ Quick win_
**The write-side alignment setter does not enforce its documented contract.**
The public documentation requires `align` to be a power of two no greater than
1 MiB, or 0 to disable. The implementation forwards any value to
`rpmcpioSetWriteAlign()` unchecked, so an out-of-contract value produces a
malformed payload with no diagnostic. The read side already has a validator,
`rpmAlignIsValid()` in `lib/rpmalign.hh`.
- `lib/rpmfi.cc#L2017-L2022`: reject values that fail `rpmAlignIsValid()`
before you call `rpmcpioSetWriteAlign()`, and keep 0 as the disable value.
- `include/rpm/rpmarchive.h#L126-L135`: document the behavior for
out-of-contract values once the setter enforces them.
<details>
<summary>π€ Prompt for AI Agents</summary>
```
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/rpmfi.cc` around lines 2017 - 2022, Update rpmfiArchiveSetWriteAlign in
lib/rpmfi.cc (lines 2017-2022) to validate nonzero align values with
rpmAlignIsValid() before calling rpmcpioSetWriteAlign(), while preserving 0 as
the disable value; update the setter documentation in include/rpm/rpmarchive.h
(lines 126-135) to describe the handling of out-of-contract values.
```
</details>
<!-- cr-comment:v1:e3fc72bc700f005370e354c8 -->
</blockquote></details>
</blockquote></details>
<details>
<summary>π€ Prompt for all review comments with AI agents</summary>
```
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 `@docs/manual/format_v6.md`:
- Around line 133-135: Update the payload description to hyphenate the compound
modifier, changing β8 byte hex stringβ to β8-byte hex stringβ while preserving
the surrounding documentation.
In `@lib/cpio.cc`:
- Around line 656-664: Update rpmcpioSetExpectedFileSize and the stripped-header
read path so the initial 4-byte rpmcpioReadPad step is skipped when alignment is
configured; let the deferred rpmcpioReadContentPad call validate the entire
alignment gap with zero checking. Preserve the existing behavior for unaligned
or non-stripped members.
- Around line 105-110: In the plain-stream initialization, preserve the failure
state when Ftell returns -1 instead of clamping cpio->base to 0, and record that
the base is unknown via the existing nobase state. Update rpmcpioSetReadAlign
and rpmcpioSetWriteAlign to reject nonzero alignment when nobase is set,
allowing callers to use the unaligned path rather than misaligning reads or
writes.
- Around line 164-224: Guard the next: label in the copy loop with
HAVE_COPY_FILE_RANGE, matching the conditional goto next inside copy_file_range
handling. Ensure the label remains available when that feature is enabled and is
omitted when userspace-buffer copying is compiled without it.
In `@lib/package.cc`:
- Around line 822-827: Restore fdoβs position to end after a successful
patchSignatureHeader call in rpmUncompressPackage, before returning RPMRC_OK;
preserve the existing failure handling for Ftell, ftruncate, and
patchSignatureHeader.
In `@lib/rpmfi.cc`:
- Around line 1684-1687: Update the alignment handling around rpmAlignIsValid so
the uint64_t result from headerGetNumber is validated before assigning it to the
uint32_t fi->align field. Preserve the existing behavior of setting fi->align to
zero for invalid or out-of-range values, and only narrow the value after
full-width validation succeeds.
In `@lib/signature.cc`:
- Around line 92-112: Change rpmUnwrapSignature in lib/signature.hh and
lib/signature.cc to return an int or rpmRC, preserving *sighp unchanged and
returning failure when headerImport or headerCopy fails; only replace the
original header after successful materialization, assigning the copied header
directly instead of linking then freeing it. Update materializedSignatureHeader
in lib/package.cc and the caller in sign/rpmgensig.cc to check and propagate the
result before dereferencing or using the header.
In `@tests/rpmuncompress.at`:
- Around line 38-45: Update every test invocation of rpmuncompress in this file
to use the runroot wrapper, matching the existing runroot rpmuncompress calls in
the root-case tests. Preserve any existing arguments, redirections, pipelines,
and expected-output checks; only retain direct invocation where it is explicitly
intentional.
In `@tests/rpmuncompresspkg.c`:
- Around line 91-98: The setAltSize flow around headerPutUint64, headerSizeof,
and headerWrite must operate on the raw main-header blob rather than the merged
header returned by rpmReadPackageFile(). Read/import the physical main-header
region separately, remove or replace the existing RPMTAG_PAYLOADSIZEALT array
item instead of appending via headerPutUint64(), derive the payload offset from
that blobβs serialized size, then rewrite the header at the correct location.
- Around line 217-231: In the MODE_REUSE branch, rewind fdo to the beginning
immediately before the second rpmUncompressPackage call so the second
materialization overwrites the first output rather than appending after it.
Preserve the existing length and comparison checks around the first and second
buffers.
---
Nitpick comments:
In `@build/pack.cc`:
- Line 88: Compute the uncompressed flag once in the existing caller and pass it
through to cpio_doio, removing its local strstr-based derivation. Update
writeRPM to reuse the same propagated flag rather than recomputing the "ufdio"
predicate, while preserving the existing digest timing behavior.
In `@lib/cpio.cc`:
- Around line 252-276: Update the fast path in rpmcpioWriteFile to require size
== cpio->fileend - cpio->offset before flushing or copying; reject mismatches
using the same failure behavior as rpmcpioHeaderWrite,
rpmcpioStrippedHeaderWrite, and rpmcpioWrite. Preserve the existing copy and
offset-advance logic for valid sizes.
In `@lib/package.cc`:
- Around line 393-407: Replace the stack-allocated buf in copyBytes with a
heap-allocated 128 KiB buffer, ensuring allocation failure returns an error and
the buffer is released on every exit path. Apply the same stack-buffer change to
copyPayloadAlt and rangeDigest, preserving their existing copy and digest
behavior.
- Around line 727-742: Update the reserved-space adjustment loop around
headerGet and utd.count so it computes an explicit signed delta between wantSize
and curSize, branches separately for growing versus shrinking reserved space,
and preserves the existing guard that stops when shrinking cannot be satisfied.
Avoid relying on unsigned wraparound in the utd.count update while retaining the
current headerMod behavior.
In `@lib/rpmchecksig.cc`:
- Around line 140-151: In readPayload, move the hdrblobImport call and its
importmsg cleanup into the framed-payload branch after the first-byte check,
before align is read or used. Keep compressed and unframed paths returning
through their existing logic without importing the header, and preserve the
import-failure fallback to readFile(fd, msg) after the first byte has been
consumed.
In `@lib/rpmfi.cc`:
- Around line 2017-2022: Update rpmfiArchiveSetWriteAlign in lib/rpmfi.cc (lines
2017-2022) to validate nonzero align values with rpmAlignIsValid() before
calling rpmcpioSetWriteAlign(), while preserving 0 as the disable value; update
the setter documentation in include/rpm/rpmarchive.h (lines 126-135) to describe
the handling of out-of-contract values.
```
</details>
<details>
<summary>πͺ Autofix</summary>
Fix all unresolved CodeRabbit comments on this PR:
- [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a
commit to this branch (recommended)
- [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a
new PR with the fixes
</details>
---
<details>
<summary>βΉοΈ Review info</summary>
<details>
<summary>βοΈ Run configuration</summary>
**Configuration used**: defaults
**Review profile**: CHILL
**Plan**: Pro Plus
**Run ID**: `76af3be0-7c6d-4196-9164-475a3e847771`
</details>
<details>
<summary>π₯ Commits</summary>
Reviewing files that changed from the base of the PR and between
c1fe256483b4802af27c2fe67a31443ac4045bd4 and
59ea81b464d476319210bd1b028ba2fa2d2969a1.
</details>
<details>
<summary>π Files selected for processing (46)</summary>
* `CMakeLists.txt`
* `build/pack.cc`
* `config.h.in`
* `docs/man/rpmuncompress.1.scd`
* `docs/manual/format_v6.md`
* `docs/manual/tags.md`
* `include/rpm/rpmarchive.h`
* `include/rpm/rpmlib.h`
* `include/rpm/rpmtag.h`
* `lib/CMakeLists.txt`
* `lib/cpio.cc`
* `lib/cpio.hh`
* `lib/fsm.cc`
* `lib/fsm.hh`
* `lib/package.cc`
* `lib/rpmalign.hh`
* `lib/rpmchecksig.cc`
* `lib/rpmds.cc`
* `lib/rpmfi.cc`
* `lib/rpmpayload.cc`
* `lib/rpmpayload.hh`
* `lib/rpmte.cc`
* `lib/rpmte_internal.hh`
* `lib/rpmvs.cc`
* `lib/rpmvs.hh`
* `lib/signature.cc`
* `lib/signature.hh`
* `macros.in`
* `rpmio/rpmio.cc`
* `rpmio/rpmio_internal.hh`
* `sign/rpmgensig.cc`
* `sign/rpmsignverity.cc`
* `tests/CMakeLists.txt`
* `tests/data/SPECS/aligntest.spec`
* `tests/rpmalign.at`
* `tests/rpmcfr.at`
* `tests/rpmcfrprobe.c`
* `tests/rpmcfrpython.at`
* `tests/rpmcorruptpad.c`
* `tests/rpmgeneral.at`
* `tests/rpmpayloadconsumer.at`
* `tests/rpmuncompress.at`
* `tests/rpmuncompresspkg.c`
* `tools/CMakeLists.txt`
* `tools/rpm2archive.cc`
* `tools/rpmuncompress.cc`
</details>
</details>
<!-- This is an auto-generated comment by CodeRabbit for review status -->
> +The payload is an RPM-specific extension of cpio, using `07070X` as magic
+bytes. The file header only contains the index number of the file in the RPM
+header as an 8 byte hex string. The payload may be compressed. Features such
_π Maintainability & Code Quality_ | _π‘ Minor_ | _β‘ Quick win_
**Hyphenate the compound modifier.**
Change `8 byte hex string` to `8-byte hex string`.
<details>
<summary>Proposed fix</summary>
```diff
- header as an 8 byte hex string.
+ header as an 8-byte hex string.
```
</details>
<!-- suggestion_start -->
<details>
<summary>π Committable suggestion</summary>
> βΌοΈ **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.
```suggestion
The payload is an RPM-specific extension of cpio, using `07070X` as magic
bytes. The file header only contains the index number of the file in the RPM
header as an 8-byte hex string. The payload may be compressed. Features such
```
</details>
<!-- suggestion_end -->
<details>
<summary>π§° Tools</summary>
<details>
<summary>πͺ LanguageTool</summary>
[grammar] ~135-~135: Use a hyphen to join words.
Context: ...er of the file in the RPM header as an 8 byte hex string. The
payload may be comp...
(QB_NEW_EN_HYPHEN)
</details>
</details>
<details>
<summary>π€ Prompt for AI Agents</summary>
```
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/manual/format_v6.md` around lines 133 - 135, Update the payload
description to hyphenate the compound modifier, changing β8 byte hex stringβ to
β8-byte hex stringβ while preserving the surrounding documentation.
```
</details>
<!-- fingerprinting:phantom:medusa:tapir -->
<!-- cr-indicator-types:potential_issue -->
<!-- cr-comment:v1:3288c991ee4a0291a7505be3 -->
_Source: Linters/SAST tools_
<!-- This is an auto-generated comment by CodeRabbit -->
> + /* Plain streams use physical file offsets. Compressed streams align the
+ * canonical cpio relative to its decompressed start. */
+ if (fdIsPlain(fd)) {
+ off_t base = Ftell(fd);
+ cpio->base = (base > 0) ? base : 0;
+ }
_π©Ί Stability & Availability_ | _π Major_ | _β‘ Quick win_
**Do not silently substitute base 0 when the stream offset is unknown.**
`Ftell` returns -1 for a plain but nonseekable stream, for example a payload
read from a pipe. The clamp then sets `base` to 0. `rpmcpioContentAligned` and
`rpmcpioReadContentPad` later align relative to 0 instead of the true physical
offset. The reader consumes the wrong number of padding bytes and
desynchronizes from the cpio stream, so the archive misparses rather than
reporting an error.
`tools/rpm2archive.cc` avoids this by spooling stdin to a temp file before it
probes, which shows the case is real. Record the failure instead of hiding it,
and make the aligned read path reject a stream with an unknown base.
<details>
<summary>π οΈ Sketch of the fix</summary>
```diff
if (fdIsPlain(fd)) {
off_t base = Ftell(fd);
- cpio->base = (base > 0) ? base : 0;
+ if (base < 0)
+ cpio->nobase = 1; /* unknown offset; refuse content alignment */
+ else
+ cpio->base = base;
}
```
Then reject a nonzero alignment in `rpmcpioSetReadAlign`/`rpmcpioSetWriteAlign`
when `nobase` is set, so callers fall back to the unaligned path.
</details>
<details>
<summary>π€ Prompt for AI Agents</summary>
```
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/cpio.cc` around lines 105 - 110, In the plain-stream initialization,
preserve the failure state when Ftell returns -1 instead of clamping cpio->base
to 0, and record that the base is unknown via the existing nobase state. Update
rpmcpioSetReadAlign and rpmcpioSetWriteAlign to reject nonzero alignment when
nobase is set, allowing callers to use the unaligned path rather than
misaligning reads or writes.
```
</details>
<!-- fingerprinting:phantom:medusa:komodo -->
<!-- cr-indicator-types:potential_issue -->
<!-- cr-comment:v1:5c155bb70d368ae36e743563 -->
<!-- This is an auto-generated comment by CodeRabbit -->
> + char buf[BUFSIZ * 4];
+ off_t copied = 0;
+#ifdef HAVE_COPY_FILE_RANGE
+ const off_t chunkmax = 16 * 1024 * 1024;
+ bool try_cfr = true;
+#endif
+
+ while (copied < len) {
+ off_t left = len - copied;
+ ssize_t n;
+
+#ifdef HAVE_COPY_FILE_RANGE
+ /* First try bounded range copies so progress remains observable. */
+ if (try_cfr) {
+ off_t in = src_off + copied, out = dst_off + copied;
+ size_t want = left > chunkmax ? (size_t)chunkmax : (size_t)left;
+
+ n = copy_file_range(src_fd, &in, dst_fd, &out, want, 0);
+ if (n < 0) {
+ if (errno == EINTR)
+ continue;
+ if (errno != EINVAL && errno != ENOSYS && errno != EXDEV &&
+ errno != EBADF && errno != EOPNOTSUPP)
+ return RPMERR_COPY_FAILED;
+ try_cfr = false;
+ } else if (n > 0) {
+ goto next;
+ } else {
+ return RPMERR_READ_FAILED;
+ }
+ }
+#endif
+
+ /* Otherwise, copy through a userspace buffer. */
+ {
+ size_t want = left > (off_t)sizeof(buf) ?
+ sizeof(buf) : (size_t)left;
+ n = pread(src_fd, buf, want, src_off + copied);
+ if (n < 0 && errno == EINTR)
+ continue;
+ if (n <= 0)
+ return RPMERR_READ_FAILED;
+
+ ssize_t written = 0;
+ while (written < n) {
+ ssize_t nw = pwrite(dst_fd, buf + written, n - written,
+ dst_off + copied + written);
+ if (nw < 0 && errno == EINTR)
+ continue;
+ if (nw <= 0)
+ return RPMERR_WRITE_FAILED;
+ written += nw;
+ }
+ }
+
+next:
+ copied += n;
+ if (notify)
+ notify(data, copied);
+ }
+ return 0;
_π Maintainability & Code Quality_ | _π‘ Minor_ | _β‘ Quick win_
**The `next:` label becomes unused when `HAVE_COPY_FILE_RANGE` is undefined.**
The only `goto next` sits inside the `#ifdef HAVE_COPY_FILE_RANGE` block at
Line 190. The label at Line 219 is outside any guard. On a build without
`copy_file_range`, for example a non-Linux target or an older glibc, the
compiler emits `-Wunused-label`. A build with `-Werror` fails.
Guard the label the same way, or restructure the loop so the label is not
needed.
<details>
<summary>π§ Proposed guard</summary>
```diff
}
`#endif`
/* Otherwise, copy through a userspace buffer. */
{
size_t want = left > (off_t)sizeof(buf) ?
sizeof(buf) : (size_t)left;
n = pread(src_fd, buf, want, src_off + copied);
@@
}
+#ifdef HAVE_COPY_FILE_RANGE
next:
+#endif
copied += n;
if (notify)
notify(data, copied);
}
```
</details>
<!-- suggestion_start -->
<details>
<summary>π Committable suggestion</summary>
> βΌοΈ **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.
```suggestion
char buf[BUFSIZ * 4];
off_t copied = 0;
`#ifdef` HAVE_COPY_FILE_RANGE
const off_t chunkmax = 16 * 1024 * 1024;
bool try_cfr = true;
`#endif`
while (copied < len) {
off_t left = len - copied;
ssize_t n;
`#ifdef` HAVE_COPY_FILE_RANGE
/* First try bounded range copies so progress remains observable. */
if (try_cfr) {
off_t in = src_off + copied, out = dst_off + copied;
size_t want = left > chunkmax ? (size_t)chunkmax : (size_t)left;
n = copy_file_range(src_fd, &in, dst_fd, &out, want, 0);
if (n < 0) {
if (errno == EINTR)
continue;
if (errno != EINVAL && errno != ENOSYS && errno != EXDEV &&
errno != EBADF && errno != EOPNOTSUPP)
return RPMERR_COPY_FAILED;
try_cfr = false;
} else if (n > 0) {
goto next;
} else {
return RPMERR_READ_FAILED;
}
}
`#endif`
/* Otherwise, copy through a userspace buffer. */
{
size_t want = left > (off_t)sizeof(buf) ?
sizeof(buf) : (size_t)left;
n = pread(src_fd, buf, want, src_off + copied);
if (n < 0 && errno == EINTR)
continue;
if (n <= 0)
return RPMERR_READ_FAILED;
ssize_t written = 0;
while (written < n) {
ssize_t nw = pwrite(dst_fd, buf + written, n - written,
dst_off + copied + written);
if (nw < 0 && errno == EINTR)
continue;
if (nw <= 0)
return RPMERR_WRITE_FAILED;
written += nw;
}
}
`#ifdef` HAVE_COPY_FILE_RANGE
next:
`#endif`
copied += n;
if (notify)
notify(data, copied);
}
return 0;
```
</details>
<!-- suggestion_end -->
<details>
<summary>π€ Prompt for AI Agents</summary>
```
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/cpio.cc` around lines 164 - 224, Guard the next: label in the copy loop
with HAVE_COPY_FILE_RANGE, matching the conditional goto next inside
copy_file_range handling. Ensure the label remains available when that feature
is enabled and is omitted when userspace-buffer copying is compiled without it.
```
</details>
<!-- fingerprinting:phantom:medusa:komodo -->
<!-- cr-indicator-types:potential_issue -->
<!-- cr-comment:v1:1d27d8d08a368d8de6a1ab53 -->
<!-- This is an auto-generated comment by CodeRabbit -->
> +int rpmcpioSetExpectedFileSize(rpmcpio_t cpio, mode_t fmode, off_t fsize)
{
+ int rc = 0;
+ /* Stripped headers carry no size, so the content alignment padding could
not
+ * be skipped at header-read time; skip it now that the size is known. */
+ if (rpmcpioContentAligned(cpio, fmode, fsize))
+ rc = rpmcpioReadContentPad(cpio, fmode, fsize);
cpio->fileend = cpio->offset + fsize;
+ return rc;
_ποΈ Data Integrity & Integration_ | _π‘ Minor_ | _β‘ Quick win_
**Aligned stripped members skip zero validation on their first padding bytes.**
For a stripped header the reader pads twice. Line 612 applies
`rpmcpioReadPad(cpio, 4)` with `require_zero` off. `rpmcpioSetExpectedFileSize`
then applies the alignment padding at Line 662 with `require_zero` on. The
total byte count matches the single aligned pad the writer emitted at Line 536,
because the alignment is a multiple of 4. The absolute offsets agree.
The consequence is that up to three padding bytes of every aligned stripped
member are never checked for zero. Corrupted bytes in that prefix pass
validation, while the same corruption a few bytes later is rejected. Skip the
4-byte step for a stripped header when an alignment is configured, and let the
deferred aligned pad cover the whole gap.
<details>
<summary>π€ Prompt for AI Agents</summary>
```
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/cpio.cc` around lines 656 - 664, Update rpmcpioSetExpectedFileSize and
the stripped-header read path so the initial 4-byte rpmcpioReadPad step is
skipped when alignment is configured; let the deferred rpmcpioReadContentPad
call validate the entire alignment gap with zero checking. Preserve the existing
behavior for unaligned or non-stripped members.
```
</details>
<!-- fingerprinting:phantom:medusa:komodo -->
<!-- cr-indicator-types:potential_issue -->
<!-- cr-comment:v1:0b1eed89fbac3fded1e92bc5 -->
<!-- This is an auto-generated comment by CodeRabbit -->
> + off_t end = Ftell(fdo);
+ if (end < 0 || ftruncate(Fileno(fdo), end) ||
+ patchSignatureHeader(fdo, *sighp, outHdrStart, end))
+ return RPMRC_FAIL;
+ return RPMRC_OK;
+}
_ποΈ Data Integrity & Integration_ | _π‘ Minor_ | _β‘ Quick win_
**Restore the output position after patching the signature header.**
`patchSignatureHeader` seeks `fdo` back to `RPMLEAD_SIZE` and rewrites the
signature header. On success it leaves `fdo` positioned at `outHdrStart`, not
at `end`. `rpmUncompressPackage` returns with that position, so a caller that
appends or calls `Ftell(fdo)` to learn the output length gets a mid-file offset.
`tools/rpmuncompress.cc` seeks to 0 first and is unaffected, but the position
is part of the public contract. Seek back to `end` before returning, or
document the position explicitly in the `rpmUncompressPackage` declaration.
<details>
<summary>π Proposed fix</summary>
```diff
off_t end = Ftell(fdo);
if (end < 0 || ftruncate(Fileno(fdo), end) ||
patchSignatureHeader(fdo, *sighp, outHdrStart, end))
return RPMRC_FAIL;
+ if (Fseek(fdo, end, SEEK_SET) < 0)
+ return RPMRC_FAIL;
return RPMRC_OK;
```
</details>
<!-- suggestion_start -->
<details>
<summary>π Committable suggestion</summary>
> βΌοΈ **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.
```suggestion
off_t end = Ftell(fdo);
if (end < 0 || ftruncate(Fileno(fdo), end) ||
patchSignatureHeader(fdo, *sighp, outHdrStart, end))
return RPMRC_FAIL;
if (Fseek(fdo, end, SEEK_SET) < 0)
return RPMRC_FAIL;
return RPMRC_OK;
```
</details>
<!-- suggestion_end -->
<details>
<summary>π€ Prompt for AI Agents</summary>
```
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/package.cc` around lines 822 - 827, Restore fdoβs position to end
after a
successful patchSignatureHeader call in rpmUncompressPackage, before returning
RPMRC_OK; preserve the existing failure handling for Ftell, ftruncate, and
patchSignatureHeader.
```
</details>
<!-- fingerprinting:phantom:medusa:komodo -->
<!-- cr-indicator-types:potential_issue -->
<!-- cr-comment:v1:d980718b53bb7088e5ab8635 -->
<!-- This is an auto-generated comment by CodeRabbit -->
> + /* alignment must be a power of two; ignore untrusted junk */
+ fi->align = headerGetNumber(h, RPMTAG_PAYLOADALIGNMENT);
+ if (!rpmAlignIsValid(fi->align))
+ fi->align = 0;
_ποΈ Data Integrity & Integration_ | _π Major_ | _β‘ Quick win_
**Validate the alignment before you narrow it to `uint32_t`.**
`headerGetNumber()` returns a `uint64_t`. Line 1685 assigns it to `fi->align`,
which is `uint32_t` (line 120). The narrowing happens before
`rpmAlignIsValid()` runs, so the validator inspects the truncated value.
A crafted header value of `0x100000400` truncates to `1024`.
`rpmAlignIsValid(1024)` returns true, so the reader accepts an alignment that
the header never declared. The comment states the intent to ignore untrusted
junk, but the truncation defeats it.
Validate the full 64-bit value first.
<details>
<summary>π Proposed fix</summary>
```diff
/* alignment must be a power of two; ignore untrusted junk */
- fi->align = headerGetNumber(h, RPMTAG_PAYLOADALIGNMENT);
- if (!rpmAlignIsValid(fi->align))
- fi->align = 0;
+ uint64_t align = headerGetNumber(h, RPMTAG_PAYLOADALIGNMENT);
+ fi->align = rpmAlignIsValid(align) ? (uint32_t)align : 0;
```
</details>
<!-- suggestion_start -->
<details>
<summary>π Committable suggestion</summary>
> βΌοΈ **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.
```suggestion
/* alignment must be a power of two; ignore untrusted junk */
uint64_t align = headerGetNumber(h, RPMTAG_PAYLOADALIGNMENT);
fi->align = rpmAlignIsValid(align) ? (uint32_t)align : 0;
```
</details>
<!-- suggestion_end -->
<details>
<summary>π€ Prompt for AI Agents</summary>
```
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/rpmfi.cc` around lines 1684 - 1687, Update the alignment handling
around
rpmAlignIsValid so the uint64_t result from headerGetNumber is validated before
assigning it to the uint32_t fi->align field. Preserve the existing behavior of
setting fi->align to zero for invalid or out-of-range values, and only narrow
the value after full-width validation succeeds.
```
</details>
<!-- fingerprinting:phantom:medusa:komodo -->
<!-- cr-indicator-types:potential_issue -->
<!-- cr-comment:v1:9afe18249cf2c98cb0fff2a8 -->
<!-- This is an auto-generated comment by CodeRabbit -->
> +void rpmUnwrapSignature(Header *sighp)
+{
+ struct rpmtd_s td;
+ Header oh = NULL;
+
+ if (headerGet(*sighp, RPMTAG_HEADERSIGNATURES, &td,
+ HEADERGET_DEFAULT)) {
+ oh = headerImport(td.data, td.count, HEADERIMPORT_COPY);
+ rpmtdFreeData(&td);
+ } else {
+ oh = headerLink(*sighp);
+ }
+
+ if (oh) {
+ Header nh = headerCopy(oh);
+ headerFree(*sighp);
+ *sighp = headerLink(nh);
+ headerFree(nh);
+ headerFree(oh);
+ }
+}
_π©Ί Stability & Availability_ | _π Major_ | _β‘ Quick win_
**Signal failure from `rpmUnwrapSignature`.**
Two failure paths are silent because the function returns `void`:
1. If `headerImport` fails on a corrupt `RPMTAG_HEADERSIGNATURES` region, `oh`
is `NULL`, the `if (oh)` block is skipped, and `*sighp` stays wrapped.
`materializedSignatureHeader` in `lib/package.cc` then iterates the
still-wrapped header, accepts `RPMTAG_HEADERSIGNATURES` in its allow-list, and
returns a header that still carries the region blob. The caller cannot detect
this.
2. If `headerCopy(oh)` fails, `nh` is `NULL`, the original is freed, and
`*sighp` becomes `NULL` through `headerLink(NULL)`.
`materializedSignatureHeader` calls `headerInitIterator(*sighp)` before any
NULL check.
Return an `int` or `rpmRC` and leave `*sighp` unchanged on failure.
Also, `*sighp = headerLink(nh); headerFree(nh);` is equivalent to `*sighp =
nh;`.
<details>
<summary>π Proposed fix</summary>
```diff
-void rpmUnwrapSignature(Header *sighp)
+int rpmUnwrapSignature(Header *sighp)
{
struct rpmtd_s td;
Header oh = NULL;
+ Header nh = NULL;
if (headerGet(*sighp, RPMTAG_HEADERSIGNATURES, &td,
HEADERGET_DEFAULT)) {
oh = headerImport(td.data, td.count, HEADERIMPORT_COPY);
rpmtdFreeData(&td);
} else {
oh = headerLink(*sighp);
}
- if (oh) {
- Header nh = headerCopy(oh);
- headerFree(*sighp);
- *sighp = headerLink(nh);
- headerFree(nh);
- headerFree(oh);
- }
+ if (oh == NULL)
+ return -1;
+ nh = headerCopy(oh);
+ headerFree(oh);
+ if (nh == NULL)
+ return -1;
+ headerFree(*sighp);
+ *sighp = nh;
+ return 0;
}
```
Update `lib/signature.hh`, `lib/package.cc` (line 424), and `sign/rpmgensig.cc`
(line 717) to check the result.
</details>
<details>
<summary>π€ Prompt for AI Agents</summary>
```
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/signature.cc` around lines 92 - 112, Change rpmUnwrapSignature in
lib/signature.hh and lib/signature.cc to return an int or rpmRC, preserving
*sighp unchanged and returning failure when headerImport or headerCopy fails;
only replace the original header after successful materialization, assigning the
copied header directly instead of linking then freeing it. Update
materializedSignatureHeader in lib/package.cc and the caller in
sign/rpmgensig.cc to check and propagate the result before dereferencing or
using the header.
```
</details>
<!-- fingerprinting:phantom:medusa:komodo -->
<!-- cr-indicator-types:potential_issue -->
<!-- cr-comment:v1:47b97ab1b6c2c3aca220eb08 -->
<!-- This is an auto-generated comment by CodeRabbit -->
> +rpmuncompress "${source}" | cmp - "${source}"
+if rpmuncompress --package ${RPMTEST}/data/SPECS/aligntest.spec \
+ > ${RPMTEST}/tmp/aligntest-notpkg.log 2>&1; then
+ exit 1
+fi
+grep -q 'not an rpm package' ${RPMTEST}/tmp/aligntest-notpkg.log
+
+rpmuncompress --package "${source}" > ${RPMTEST}/build/aligntest-converted.rpm
_π Maintainability & Code Quality_ | _π‘ Minor_ | _β‘ Quick win_
<details>
<summary>𧩠Analysis chain</summary>
π Script executed:
```shell
#!/bin/bash
# Description: Check how other tests invoke rpmuncompress and what runroot does.
rg -n 'rpmuncompress' tests --glob '*.at' | head -50
fd -t f 'runroot' tests --exec sed -n '1,40p' {}
```
Repository: rpm-software-management/rpm
Length of output: 3347
---
π Script executed:
```shell
#!/bin/bash
set -euo pipefail
echo "== tests/rpmuncompress.at context =="
sed -n '1,160p' tests/rpmuncompress.at
echo
sed -n '230,295p' tests/rpmuncompress.at
echo
echo "== locate runroot definitions =="
rg -n
'^(runroot|define_testroot|AT_KEYWORDS|AT_SETUP|RPMTEST_SETUP|AT_CLEANUP)'
tests/rpmuncompress.at tests/rpmmacro.at tests/rpmbuild.at -g '*.at' | sed -n
'1,200p'
echo
echo "== runroot implementation references =="
rg -n '\brunroot\b|setup|macro|configure|macrofile|rpmmacro' tests --glob
'*.at' | sed -n '1,240p'
```
Repository: rpm-software-management/rpm
Length of output: 35531
---
π Script executed:
```shell
#!/bin/bash
set -euo pipefail
echo "== configure/m4 references for runroot and RPMTEST_SETUP_RW =="
rg -n
'\b(runroot|RPMTEST_SETUP_RW|RPMTEST_SETUP|define_testroot|AT_PROG_RPM|AT_PATH_RPMUNCOMPRESS|AC_PATH_RPMUNCOMPRESS|RPMUNCOMPRESS|rpmuncompress)'
\
configure acinclude.m4 tests --glob '*.m4' --glob '*.in' --glob '*.ac' --glob
'*.at' \
| sed -n '1,260p'
echo
echo "== local.at =="
cat -n tests/local.at | sed -n '1,110p'
echo
echo "== top-level runroot occurrences with context =="
rg -n -C 3 '^[[:space:]]*runroot[[:space:]]' tests/local.at
tests/rpmuncompress.at tests/rpmmacro.at | sed -n '1,160p'
```
Repository: rpm-software-management/rpm
Length of output: 16807
---
</details>
**Use `runroot` for `rpmuncompress` calls.**
Lines 38, 45, 53, 66, 70, 81, 100, 114, 125, and 139 call `rpmuncompress`
directly, while the test root cases on lines 261 and 283 call `runroot
rpmuncompress`. Direct calls resolve the host macro/config paths instead of the
RPM test root path, so these cases do not exercise the same configuration. Use
the same form for all invocation tests unless a direct invocation is
intentional.
<details>
<summary>π€ Prompt for AI Agents</summary>
```
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/rpmuncompress.at` around lines 38 - 45, Update every test invocation
of
rpmuncompress in this file to use the runroot wrapper, matching the existing
runroot rpmuncompress calls in the root-case tests. Preserve any existing
arguments, redirections, pipelines, and expected-output checks; only retain
direct invocation where it is explicitly intentional.
```
</details>
<!-- fingerprinting:phantom:medusa:komodo -->
<!-- cr-indicator-types:potential_issue -->
<!-- cr-comment:v1:c3a0ac0644465d3856a3c1a9 -->
<!-- This is an auto-generated comment by CodeRabbit -->
> + payload = Ftell(fd);
+ hsize = headerSizeof(h, 1);
+ if (payload < (off_t)hsize ||
+ !headerPutUint64(h, RPMTAG_PAYLOADSIZEALT, &size, 1) ||
+ headerSizeof(h, 1) != hsize ||
+ Fseek(fd, payload - hsize, SEEK_SET) < 0 ||
+ headerWrite(fd, h, 1) || Fflush(fd))
+ goto exit;
_π― Functional Correctness_ | _π Major_ | _ποΈ Heavy lift_
<details>
<summary>𧩠Analysis chain</summary>
π Script executed:
```shell
#!/bin/bash
# Description: Check headerPutUint64 replace-vs-append semantics and legacy sig
merging.
fd -t f 'header.cc|header.c' lib rpmio --exec rg -n -C6
'headerPut\b|headerAddEntry|HEADERPUT' {} | head -60
rg -n -C6 'headerMergeLegacySigs' lib | head -40
```
Repository: rpm-software-management/rpm
Length of output: 2900
---
π Script executed:
```shell
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg
'(^|/)(tests/rpmuncompresspkg\.c|lib/package\.cc|lib/header\.(c|cc)|include/header\.h|include/rpm)4$|tests/rpmuncompresspkg\.c$'
|| true
echo "== tests/rpmuncompresspkg.c relevant section =="
wc -l tests/rpmuncompresspkg.c
sed -n '1,150p' tests/rpmuncompresspkg.c
echo "== headerPut/headerPutUint64 definitions/usages =="
rg -n -C8
'headerPutUint64|headerPut\(|HEADERPUT_APPEND|HEADERPUT_DEFAULT|intAppendEntry|findEntry\('
lib include tests | head -160
echo "== headerGet/headerSizeof definitions/usages =="
rg -n -C8 'headerSizeof|rpmReadPackageFile|headerGetNumber|headerGet\(' lib
include tests | head -180
echo "== locate and inspect read header blob/parser relevant sections =="
fd -t f 'hdrblob|header' lib | while read -r f; do
echo "--- $f ---"
wc -l "$f"
done
```
Repository: rpm-software-management/rpm
Length of output: 21099
---
π Script executed:
```shell
#!/bin/bash
set -euo pipefail
echo "== lib/header.cc headerPut / int* helpers =="
sed -n '1450,1590p' lib/header.cc
echo "== lib/headerutil.cc headerPutType =="
sed -n '100,150p' lib/headerutil.cc
echo "== lib/package.cc rpmReadPackageFile / headerMergeLegacySigs =="
sed -n '75,90p' lib/package.cc
sed -n '330,360p' lib/package.cc
echo "== rpmReadPackageFile call sites / headerGet with HEADERGET_RAW =="
rg -n -C4
'rpmReadPackageFile\(|HEADERGET_RAW|rpmTagGetTagType|RPMTAG_PAYLOADSIZEALT|headerRead|hdrblobImport|headerWrite'
include lib tests | head -220
```
Repository: rpm-software-management/rpm
Length of output: 16905
---
π Script executed:
```shell
#!/bin/bash
set -euo pipefail
echo "== lib/pkg.c: headersize/read/write definitions/usages =="
fd -t f 'pkg\.c|pkg' lib | head -20
grep -RIn -C5
'readHeader\|headerSizeof\|headerWrite\|HEADER_MAGIC_YES\|intReadHeaders' lib
include tests | head -240
echo "== read header parsing around signature/main header =="
rg -n -C8
'readHeader\b|hdrblobRead\(|hdrblobImport\(|findRegion|hdrblob.*Region|regiontag|hdrblob.*entry'
lib include | head -260
```
Repository: rpm-software-management/rpm
Length of output: 19153
---
</details>
**Use the raw main-header blob for `setAltSize()`.**
`rpmReadPackageFile()` returns a linked metadata header that includes merged
signature tags and retrofits, so `headerSizeof(h, 1)` does not equal the
physical main-header payload recorded by the header trailer. Line 96 seeks from
the byte stream on the wrong basis and `headerWrite()` can overwrite
payload-adjacent data. Also, `RPMTAG_PAYLOADSIZEALT` is tagged `l` /
`RPM_ARRAY_RETURN_TYPE`, so `headerPutUint64()` appends if the item exists
instead of replacing it. Read/import the main header region separately, delete
or overwrite `RPMTAG_PAYLOADSIZEALT`, then rewrite using the blob-derived
payload offset.
<details>
<summary>π€ Prompt for AI Agents</summary>
```
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/rpmuncompresspkg.c` around lines 91 - 98, The setAltSize flow around
headerPutUint64, headerSizeof, and headerWrite must operate on the raw
main-header blob rather than the merged header returned by rpmReadPackageFile().
Read/import the physical main-header region separately, remove or replace the
existing RPMTAG_PAYLOADSIZEALT array item instead of appending via
headerPutUint64(), derive the payload offset from that blobβs serialized size,
then rewrite the header at the correct location.
```
</details>
<!-- fingerprinting:phantom:medusa:komodo -->
<!-- cr-indicator-types:potential_issue -->
<!-- cr-comment:v1:2eb3f9724e0e5415a410cd3c -->
<!-- This is an auto-generated comment by CodeRabbit -->
> + if (rc == RPMRC_OK && mode == MODE_REUSE) {
+ off_t len = Ftell(fdo);
+ char *first = (len >= 0) ? malloc(len ? len : 1) : NULL;
+ if (first == NULL || Fseek(fdo, 0, SEEK_SET) < 0 ||
+ Fread(first, 1, len, fdo) != len) {
+ free(first);
+ rc = RPMRC_FAIL;
+ goto exit;
+ }
+ rc = rpmUncompressPackage(ts, fdi, fdo);
+ if (rc == RPMRC_OK) {
+ char *second = malloc(len ? len : 1);
+ if (second == NULL || Ftell(fdo) != len ||
+ Fseek(fdo, 0, SEEK_SET) < 0 ||
+ Fread(second, 1, len, fdo) != len ||
_π― Functional Correctness_ | _π‘ Minor_ | _β‘ Quick win_
<details>
<summary>𧩠Analysis chain</summary>
π Script executed:
```shell
#!/bin/bash
# Description: Check whether rpmUncompressPackage repositions or truncates the
output fd.
rg -n -C10 'rpmUncompressPackage|uncompressPackage' lib/package.cc | head -80
```
Repository: rpm-software-management/rpm
Length of output: 1940
---
π Script executed:
```shell
#!/bin/bash
set -euo pipefail
# Inspect the relevant test code and implementation context around file
handling.
sed -n '190,255p' tests/rpmuncompresspkg.c | cat -n -v
echo '---'
sed -n '740,851p' lib/package.cc | cat -n -v
```
Repository: rpm-software-management/rpm
Length of output: 6476
---
</details>
**Rewind `fdo` before the second materialization.**
In `MODE_REUSE`, the code reads `fdo` to get `len`, leaving the output position
at EOF. `rpmUncompressPackage()` does not rewind or truncate `fdo`, so the
second pass writes after the first. This makes `Ftell(fdo) != len` fail and
reports a spurious output difference.
<details>
<summary>π Proposed fix to rewind the output before the second pass</summary>
```diff
- rc = rpmUncompressPackage(ts, fdi, fdo);
+ if (Fseek(fdo, 0, SEEK_SET) < 0) {
+ free(first);
+ rc = RPMRC_FAIL;
+ goto exit;
+ }
+ rc = rpmUncompressPackage(ts, fdi, fdo);
```
</details>
<!-- suggestion_start -->
<details>
<summary>π Committable suggestion</summary>
> βΌοΈ **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.
```suggestion
if (rc == RPMRC_OK && mode == MODE_REUSE) {
off_t len = Ftell(fdo);
char *first = (len >= 0) ? malloc(len ? len : 1) : NULL;
if (first == NULL || Fseek(fdo, 0, SEEK_SET) < 0 ||
Fread(first, 1, len, fdo) != len) {
free(first);
rc = RPMRC_FAIL;
goto exit;
}
if (Fseek(fdo, 0, SEEK_SET) < 0) {
free(first);
rc = RPMRC_FAIL;
goto exit;
}
rc = rpmUncompressPackage(ts, fdi, fdo);
if (rc == RPMRC_OK) {
char *second = malloc(len ? len : 1);
if (second == NULL || Ftell(fdo) != len ||
Fseek(fdo, 0, SEEK_SET) < 0 ||
Fread(second, 1, len, fdo) != len ||
```
</details>
<!-- suggestion_end -->
<details>
<summary>π€ Prompt for AI Agents</summary>
```
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/rpmuncompresspkg.c` around lines 217 - 231, In the MODE_REUSE branch,
rewind fdo to the beginning immediately before the second rpmUncompressPackage
call so the second materialization overwrites the first output rather than
appending after it. Preserve the existing length and comparison checks around
the first and second buffers.
```
</details>
<!-- fingerprinting:phantom:medusa:komodo -->
<!-- cr-indicator-types:potential_issue -->
<!-- cr-comment:v1:9758dfd4e92f98122fd4d5b0 -->
<!-- This is an auto-generated comment by CodeRabbit -->
--
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/4294#pullrequestreview-4882168209
You are receiving this because you are subscribed to this thread.
Message ID: <rpm-software-management/rpm/pull/4294/review/[email protected]>_______________________________________________
Rpm-maint mailing list
[email protected]
https://lists.rpm.org/mailman/listinfo/rpm-maint
