MFV r268120: 4936 lz4 could theoretically overflow a pointer with a certain input

[allanjude]

Summary

Somehow r268120 was marked as merged, but the change was lost

Also brings in: "3705 stack overflow due to zfs lz4 compression" for diff reduction
This change is a noop as STACKLIMIT is always 0 on FreeBSD

And fixes a local typo

[delphij]

Could you please take a look at my comment inline?

I think the overflow prevention change itself should be landed regardless (and thanks for discovering and merging this), even it's not supposed to happen (because data is validated against their checksums before feeding into decompressor), it is in general a good thing to do to further reduce the chance of having reliability issues found on non-ECC systems.

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/lz4.c

195 ↗

(On Diff #20250)

Why? (this is mainly to avoid stack overflow, and previously we did not get evidence that this would give meaningful performance boost).

[allanjude]

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/lz4.c

195 ↗

(On Diff #20250)

Yeah, the setting on IllumOS is now 9 (2**(N+2) = 2kb) for both amd64 and i386 (was 11 and 9 respectively before)

Since the lz4 data is 16kb, I don't think the 'stack mode' ever gets used at all.

The re-arrangement is just to reduce the diff with upstream, when they lowered the value of STACKLIMIT for 64bit from 11 to 9, and moved the define outside of the LZ4_ARCH64 ifdef

The diff against upstream is now:
-#define STACKLIMIT 9
+/* FreeBSD: Use heap for all platforms for now */
+#define STACKLIMIT 0

[delphij]

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/lz4.c

195 ↗

(On Diff #20250)

Hmm I see, sorry I have misread the code block and the change is correct.