Re: [PATCH v3 1/8] reftable/stack: do not overwrite errors when compacting
- From
Han-Wen Nienhuys <hanwenn@gmail.com>
- Date
- Feb 14, 2024, 15:12 UTC
- Message-ID
- <CAOw_e7b72HVQob_hiV0gtRhGWsb=rz40WL=oaV63t7oOmEA-mw@mail.gmail.com>
- In-Reply-To
- <1dc8ddf04a112c38f41d573a48dac3f99b4b51e9.1704262787.git.ps@pks.im>
Good catch!
Sorry for messing this up.
> In the worst case, > this can lead to a compacted stack that is missing records.
Yeah, that would be an insidious corruption. Have you considered writing a test to reproduce this (and thus verify that the fix really fixes the problem?)
I think it wouldn't be too difficult: you could create a custom blocksource wrapper that returns I/O error on the Nth read, and then create a reftable with two ref blocks (could just be 2 records if you use a small blocksize and a large refname) and two log blocks. Merge that with an empty table, and see if the compacted result is what you got in. Loop over N to get coverage for all error paths.
On Wed, Jan 3, 2024 at 7:22 AM Patrick Steinhardt <ps@pks.im> wrote:
Show 71 quoted lines
>
> In order to compact multiple stacks we iterate through the merged ref
> and log records. When there is any error either when reading the records
> from the old merged table or when writing the records to the new table
> then we break out of the respective loops. When breaking out of the loop
> for the ref records though the error code will be overwritten, which may
> cause us to inadvertently skip over bad ref records. In the worst case,
> this can lead to a compacted stack that is missing records.
>
> Fix the code by using `goto done` instead so that any potential error
> codes are properly returned to the caller.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> reftable/stack.c | 20 ++++++++------------
> 1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/reftable/stack.c b/reftable/stack.c
> index 16bab82063..8729508dc3 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -801,18 +801,16 @@ static int stack_write_compact(struct reftable_stack *st,
> err = 0;
> break;
> }
> - if (err < 0) {
> - break;
> - }
> + if (err < 0)
> + goto done;
>
> if (first == 0 && reftable_ref_record_is_deletion(&ref)) {
> continue;
> }
>
> err = reftable_writer_add_ref(wr, &ref);
> - if (err < 0) {
> - break;
> - }
> + if (err < 0)
> + goto done;
> entries++;
> }
> reftable_iterator_destroy(&it);
> @@ -827,9 +825,8 @@ static int stack_write_compact(struct reftable_stack *st,
> err = 0;
> break;
> }
> - if (err < 0) {
> - break;
> - }
> + if (err < 0)
> + goto done;
> if (first == 0 && reftable_log_record_is_deletion(&log)) {
> continue;
> }
> @@ -845,9 +842,8 @@ static int stack_write_compact(struct reftable_stack *st,
> }
>
> err = reftable_writer_add_log(wr, &log);
> - if (err < 0) {
> - break;
> - }
> + if (err < 0)
> + goto done;
> entries++;
> }
>
> --
> 2.43.GIT
>-- Han-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen