zio_compress_zeroed_cb() might be broken

[adamdmoss]

System information

Type	Version/Name
OpenZFS Version	trunk (& back a few years)

Describe the problem you're observing

/*ARGSUSED*/
static int
zio_compress_zeroed_cb(void *data, size_t len, void *private)
{
    uint64_t *end = (uint64_t *)((char *)data + len);
    for (uint64_t *word = (uint64_t *)data; word < end; word++)
        if (*word != 0)
            return (1);

    return (0);
}

This callback function checks if all bytes in an abd hunk is 0, and early-outs abd_iterate_func() if any non-0 data is found. It uses an optimization where it checks for non-0 against eight bytes at a time, though only up to the last whole multiple of 8 bytes.

Unfortunately, if size_t len isn't a multiple of eight then AFAICT it's possible that up to 7 bytes at the end of the hunk won't be checked. (len%8==0) is not asserted and, from a brief look around, isn't necessarily true.

The upshot of this could be that an abd which is completely zero apart from a few bytes at the end of one its hunks will be incorrectly detected as all-zero and written as a zero-compressed block, effectively zeroing-out valid data before committing to disk.

Can anyone please reassure me that I'm actually crazy. :D

[adamdmoss]

I've exercised this enough with a bunch of crafted data that I'm pretty sure non-int64-multiple input sizes don't happen in practice. Would rather like to have an assert (or better yet, test-and-return) in there though, personally.

[stale[bot]]

This issue has been automatically marked as "stale" because it has not had any activity for a while. It will be closed in 90 days if no further activity occurs. Thank you for your contributions.

[behlendorf]

Adding an assert to make it clean this case can never happen sounds reasonable to me. Feel free to open a PR.