| Message ID | 1dc8ddf04a112c38f41d573a48dac3f99b4b51e9.1704262787.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d26c21483d327fdecb52a98be8239f0f224423f9 |
| Headers | show |
| Series | reftable: fixes and optimizations (pt.2) | expand |
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: > > 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 >
On Wed, Feb 14, 2024 at 04:12:54PM +0100, Han-Wen Nienhuys wrote: > Good catch! > > Sorry for messing this up. Nothing to be sorry about, bugs happen to all of us. > > 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. Ah, that's a feasible way to write such a test indeed. I may come back to it in the future and will add it to our backlog. Thanks! Patrick
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++; }
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(-)