Walkthrough

The changes update the CHUNKCOPY function in the RISC-V architecture code. The if-statement condition was modified from if (dist >= len) to if (dist < 0 || dist >= len), thereby including cases where the out pointer is positioned before the from pointer. The change ensures that a memcpy operation is executed when either the pointer distance is negative or exceeds the length. The remainder of the function’s operations remains unchanged.

Changes

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant CHUNKCOPY
    participant memcpy
    Caller->>CHUNKCOPY: Call CHUNKCOPY(out, from, len)
    CHUNKCOPY->>CHUNKCOPY: Compute dist = out - from
    alt dist < 0 or dist >= len
        CHUNKCOPY->>memcpy: Execute memcpy(out, from, len)
    else
        CHUNKCOPY->>CHUNKCOPY: Handle overlapping copy scenario
    end
    CHUNKCOPY->>Caller: Return result

Possibly related PRs

• Improve pipeling for AVX512 chunking #1821: Adjusted pointer check logic in CHUNKCOPY to address edge case issues similar to the modifications in this PR.

Suggested labels

bug

Suggested reviewers

• KungFuJesus
• Dead2

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.74%. Comparing base (64d16e1) to head (1b3a2c2).
Report is 3 commits behind head on develop.

@@             Coverage Diff              @@
##           develop    #1889       +/-   ##
============================================
+ Coverage    42.20%   82.74%   +40.54%     
============================================
  Files           71      143       +72     
  Lines         7353    12510     +5157     
  Branches      1265     2845     +1580     
============================================
+ Hits          3103    10352     +7249     
+ Misses        4016     1209     -2807     
- Partials       234      949      +715     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I will say, I don't think it's expected for chunkcopy to ever get a from pointer from which it can't read at least a chunk's worth of bytes. Whether that is from behind or in front of out shouldn't actually matter, we gate whether or not chunk copy is called from the outside based on the distance and whether or not they are at least chunk_t sizes apart.

We leverage chunk_memset more in the manner you're handling here, where negative distances are handled by a single memmove (we do in fact have negative distances for chunk_memset that can overlap). Everywhere you see a chunked copy operation, it's a surrogate for the copy idiom of a self modifying copy, a byte a time as in *to++ = *from++. When this is done with to behind from, it is effectively the same thing as a memmove (where the contents slide down).

@KungFuJesus Well the reason for the SIGSEGV is quite simple, but a little implicit to non-language-lawyers -- later in the function, it's comparing a (possibly) negative value typed ptrdiff_t (which should be of the same size with size_t, but signed) with the result of sizeof (which is a size_t), that triggers the promote of signed value to unsigned and converting a negative value to a huge unsigned value, which makes dist >= sizeof(chunk_t) true for negative values (which is unexpected). Then dist is passed to memcpy(), which expects an unsigned value, and the negative value gets interpreted as copying nearly the whole address space -- it will surely fail.

BTW I think the problem of RVV CHUNKCOPY is only exhibited after chunk_memset is refactored (which I bisected to).

Ping because this is a big issue that renders zlib-ng nearly totally unfunctional (and 2.2.3/2.2.4 releases are broken).

Have you isolated the issue to be RISC-V specific? I guess my main concern is whether or not it's a wider issue and you just have a dataset invoking the error before. What's weird to me is the fact that RISCV's chunkcopy implementation has to take into account the distance at all. That's something we are deferring to chunkmemset et al elsewhere to determine whether or not chunkcopy should be called.

Do you know the line calling it where it's a problem? The templated chunkmemset should be handling these scenarios.

Ahh, it looks like at least part of the issue stems from the fact that this variant tries to maximize how many chunks can be copied with the distance and passes it all to memcpy at once, rather than having a loop that does chunk bytes for the entirety of it. That does require determining the distance between the two. Perhaps something was dispatching to the libc function with memcpy and this was to avoid that.

Now it makes some sense to me why this is affected and nothing else. The bigger change with that refactor is that we call chunkcopy in more opportunities. Most of the time chunkcopy doesn't have to care if the source is behind or in front of the destination.

This is surely a RISC-V specific issue because only RISC-V has a specialized implementation of CHUNKCOPY (others all uses the on in the template header). I was going to remove the override of CHUNKCOPY in #1888 but it seems that at least some people isn't fond of this.

@KungFuJesus so is the comment in chunkset_tpl.h before the standard implementation of CHUNKCOPY still correct and accurate?

BTW I think the whole RVV optimized chunkset implementation relies on compiler unrolling memcpy -- the code contains memcpy's but no RVV intrinsics, even in chunk load/store functions.

BTW #1890 fixes the CI of RISC-V and correctly exhibits the problem solved in this PR.

@KungFuJesus so is the comment in chunkset_tpl.h before the standard implementation of CHUNKCOPY still correct and accurate?

BTW I think the whole RVV optimized chunkset implementation relies on compiler unrolling memcpy -- the code contains memcpy's but no RVV intrinsics, even in chunk load/store functions.

Yes, CHUNKCOPY from the templates is only ever called if there's at least sizeof(chunk_t)'s worth of bytes between them. On everything but AVX512 we also only call CHUNKCOPY when it's safe to over write past len by a chunk_t in the destination buffer. With AVX512 we mitigate having to worry about all that by leveraging masked writes.