Thread (32 messages) 32 messages, 3 authors, 2024-02-15

Re: [PATCH v3 1/8] reftable/stack: do not overwrite errors when compacting

From: Han-Wen Nienhuys <hidden>
Date: 2024-02-14 15:13:06

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 [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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 <redacted>
---
 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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help