{"thread":{"id":"62820","subject":"[PATCH 0/3] refs: small followups to the migration corruption fix","startedAt":"2025-01-17T07:59:20Z","lastAt":"2025-02-03T15:38:23Z","messageCount":30,"participants":["Karthik Nayak","Patrick Steinhardt","Junio C Hamano","Jeff King","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"510760","messageId":"20250117-461-corrupted-reftable-followup-v1-0-70ee605ae3fe@gmail.com","threadId":"62820","inReplyTo":null,"subject":"[PATCH 0/3] refs: small followups to the migration corruption fix","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-17T07:59:11Z","receivedAt":"2025-01-17T07:59:20Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"This is a follow up to the bug that was reported [1] around `git refs\nmigrate --ref-format=reftable` where the migration would fail for\nrepositories with reflogs with lots of entries. This was caused due to a\nmismatch in the reftable's header and footer, specifically WRT the\n'max_update_index'.\n\nWhile there was a fix posted. This series is a small followup to fix\nsome of the topics discussed there:\n1. To mark `ref_transaction_update_reflog()` as static since it is only\nused internally within 'refs.c'.\n2. To change the type of 'max_index' from 'unsigned int' to 'uint64_t'.\nThis would be much safer for large repositories with millions of files\nand on 32bit systems.\n3. To add a safeguard to prevent 'update_index' changes after first\nblock is written. This is a preventive measure to ensure such bugs don't\narise in the future.\n\nThis is based on top of master 757161efcc (Sync with Git 2.48.1,\n2025-01-13) with 'kn/reflog-migration-fix' merged in.\n\n[1]: https://lore.kernel.org/r/Z4UbkcmJAU1MT-Rs@tapette.crustytoothpaste.net\n\nSigned-off-by: Karthik Nayak \n---\nKarthik Nayak (3):\n refs: mark `ref_transaction_update_reflog()` as static\n refs: use 'uint64_t' for 'ref_update.index'\n reftable: prevent 'update_index' changes after header write\n\n refs.c | 18 ++++++++++--------\n refs.h | 14 --------------\n refs/refs-internal.h | 4 ++--\n refs/reftable-backend.c | 2 +-\n reftable/writer.c | 7 +++++++\n 5 files changed, 20 insertions(+), 25 deletions(-)\n---\n\n\n\n---\n\nbase-commit: a5aa44e7930761cb900813d971b4105f901818fb\nchange-id: 20250117-461-corrupted-reftable-followup-eb0e4fd1a723\n\nThanks\n- Karthik\n\n"},{"id":"510761","messageId":"20250117-461-corrupted-reftable-followup-v1-1-70ee605ae3fe@gmail.com","threadId":"62820","inReplyTo":"20250117-461-corrupted-reftable-followup-v1-0-70ee605ae3fe@gmail.com","subject":"[PATCH 1/3] refs: mark `ref_transaction_update_reflog()` as static","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-17T07:59:12Z","receivedAt":"2025-01-17T07:59:22Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `ref_transaction_update_reflog()` function is only used within\n'refs.c', so mark it as static.\n\nReported-by: Junio C Hamano \nSigned-off-by: Karthik Nayak \n---\n refs.c | 16 +++++++++-------\n refs.h | 14 --------------\n 2 files changed, 9 insertions(+), 21 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex f7b6f0f897eb58665e10a2efd3eb53c3f72abe61..1bb6f0356d5c5cae8bea9d6f4d5ff35164a03c64 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1318,13 +1318,15 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \treturn 0;\n }\n \n-int ref_transaction_update_reflog(struct ref_transaction *transaction,\n-\t\t\t\t const char *refname,\n-\t\t\t\t const struct object_id *new_oid,\n-\t\t\t\t const struct object_id *old_oid,\n-\t\t\t\t const char *committer_info, unsigned int flags,\n-\t\t\t\t const char *msg, unsigned int index,\n-\t\t\t\t struct strbuf *err)\n+static int ref_transaction_update_reflog(struct ref_transaction *transaction,\n+\t\t\t\t\t const char *refname,\n+\t\t\t\t\t const struct object_id *new_oid,\n+\t\t\t\t\t const struct object_id *old_oid,\n+\t\t\t\t\t const char *committer_info,\n+\t\t\t\t\t unsigned int flags,\n+\t\t\t\t\t const char *msg,\n+\t\t\t\t\t unsigned int index,\n+\t\t\t\t\t struct strbuf *err)\n {\n \tstruct ref_update *update;\n \ndiff --git a/refs.h b/refs.h\nindex a0cdd99250e8286b55808b697b0a94afac5d8319..09be47afbee51e99f4ae49588cd65596ccfcb07e 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -771,20 +771,6 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \t\t\t unsigned int flags, const char *msg,\n \t\t\t struct strbuf *err);\n \n-/*\n- * Similar to`ref_transaction_update`, but this function is only for adding\n- * a reflog update. Supports providing custom committer information. The index\n- * field can be utiltized to order updates as desired. When not used, the\n- * updates default to being ordered by refname.\n- */\n-int ref_transaction_update_reflog(struct ref_transaction *transaction,\n-\t\t\t\t const char *refname,\n-\t\t\t\t const struct object_id *new_oid,\n-\t\t\t\t const struct object_id *old_oid,\n-\t\t\t\t const char *committer_info, unsigned int flags,\n-\t\t\t\t const char *msg, unsigned int index,\n-\t\t\t\t struct strbuf *err);\n-\n /*\n * Add a reference creation to transaction. new_oid is the value that\n * the reference should have after the update; it must not be\n\n-- \n2.47.0\n\n"},{"id":"510762","messageId":"20250117-461-corrupted-reftable-followup-v1-2-70ee605ae3fe@gmail.com","threadId":"62820","inReplyTo":"20250117-461-corrupted-reftable-followup-v1-0-70ee605ae3fe@gmail.com","subject":"[PATCH 2/3] refs: use 'uint64_t' for 'ref_update.index'","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-17T07:59:13Z","receivedAt":"2025-01-17T07:59:25Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The 'ref_update.index' variable is used to store an index for a given\nreference update. This index is used to order the updates in a\npredetermined order, while the default ordering is alphabetical as per\nthe refname.\n\nFor large repositories with millions of references, it should be safer\nto use 'uint64_t'. Let's do that. This also is applied for all other\ncode sections where we store 'index' and pass it around.\n\nReported-by: brian m. carlson \nSigned-off-by: Karthik Nayak \n---\n refs.c | 4 ++--\n refs/refs-internal.h | 4 ++--\n refs/reftable-backend.c | 2 +-\n 3 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 1bb6f0356d5c5cae8bea9d6f4d5ff35164a03c64..0de23f91b2e4401e93aee6940d4aa2abea4312cf 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1325,7 +1325,7 @@ static int ref_transaction_update_reflog(struct ref_transaction *transaction,\n \t\t\t\t\t const char *committer_info,\n \t\t\t\t\t unsigned int flags,\n \t\t\t\t\t const char *msg,\n-\t\t\t\t\t unsigned int index,\n+\t\t\t\t\t uint64_t index,\n \t\t\t\t\t struct strbuf *err)\n {\n \tstruct ref_update *update;\n@@ -2807,7 +2807,7 @@ static int migrate_one_ref(const char *refname, const char *referent UNUSED, con\n }\n \n struct reflog_migration_data {\n-\tunsigned int index;\n+\tuint64_t index;\n \tconst char *refname;\n \tstruct ref_store *old_refs;\n \tstruct ref_transaction *transaction;\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex aaab711bb96844755dfa600d37efdb25b30a0765..8894b43d1d1a327d404d3923c507d2d39649de19 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -120,7 +120,7 @@ struct ref_update {\n \t * when migrating reflogs and we want to ensure we carry over the\n \t * same order.\n \t */\n-\tunsigned int index;\n+\tuint64_t index;\n \n \t/*\n \t * If this ref_update was split off of a symref update via\n@@ -203,7 +203,7 @@ struct ref_transaction {\n \tenum ref_transaction_state state;\n \tvoid *backend_data;\n \tunsigned int flags;\n-\tunsigned int max_index;\n+\tuint64_t max_index;\n };\n \n /*\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 289496058e6eb4d3e3aef96ca70219cd4ff78eae..6814c87bc618229ac8a70b904be3f850371ad876 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -942,7 +942,7 @@ struct write_transaction_table_arg {\n \tsize_t updates_nr;\n \tsize_t updates_alloc;\n \tsize_t updates_expected;\n-\tunsigned int max_index;\n+\tuint64_t max_index;\n };\n \n struct reftable_transaction_data {\n\n-- \n2.47.0\n\n"},{"id":"510763","messageId":"20250117-461-corrupted-reftable-followup-v1-3-70ee605ae3fe@gmail.com","threadId":"62820","inReplyTo":"20250117-461-corrupted-reftable-followup-v1-0-70ee605ae3fe@gmail.com","subject":"[PATCH 3/3] reftable: prevent 'update_index' changes after header write","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-17T07:59:14Z","receivedAt":"2025-01-17T07:59:28Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The function `reftable_writer_set_limits()` allows updating the\n'min_update_index' and 'max_update_index' of a reftable writer. These\nvalues are written to both the writer's header and footer.\n\nSince the header is written during the first block write, any subsequent\nchanges to the update index would create a mismatch between the header\nand footer values. The footer would contain the newer values while the\nheader retained the original ones. To fix this bug, we now prevent\ncallers from updating these values after the header has been written.\n\nSigned-off-by: Karthik Nayak \n---\n reftable/writer.c | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 740c98038eaf883258bef4988f78977ac7e4a75a..c602b873543790e36178f797ed9f98112671f97f 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -182,6 +182,13 @@ int reftable_writer_new(struct reftable_writer **out,\n void reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,\n \t\t\t\tuint64_t max)\n {\n+\t/*\n+\t * The limits shouldn't be modified post writing the first block, else\n+\t * it would cause a mismatch between the header and the footer.\n+\t */\n+\tif (w->next)\n+\t\tBUG(\"update index modified after writing first block\");\n+\n \tw->min_update_index = min;\n \tw->max_update_index = max;\n }\n\n-- \n2.47.0\n\n"},{"id":"510775","messageId":"Z4oi55XewgX2deTm@pks.im","threadId":"62820","inReplyTo":"20250117-461-corrupted-reftable-followup-v1-3-70ee605ae3fe@gmail.com","subject":"Re: [PATCH 3/3] reftable: prevent 'update_index' changes after header write","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-17T09:29:21Z","receivedAt":"2025-01-17T09:29:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jan 17, 2025 at 08:59:14AM +0100, Karthik Nayak wrote:\n> diff --git a/reftable/writer.c b/reftable/writer.c\n> index 740c98038eaf883258bef4988f78977ac7e4a75a..c602b873543790e36178f797ed9f98112671f97f 100644\n> --- a/reftable/writer.c\n> +++ b/reftable/writer.c\n> @@ -182,6 +182,13 @@ int reftable_writer_new(struct reftable_writer **out,\n> void reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,\n> \t\t\t\tuint64_t max)\n> {\n> +\t/*\n> +\t * The limits shouldn't be modified post writing the first block, else\n> +\t * it would cause a mismatch between the header and the footer.\n> +\t */\n\nCan we make this *even* stricter? I think that this is something that is\neasy to do wrong, and the fact that it only triggers in some situations\nof misuse may easily make tests miss this issue. So ideally, we should\nassert that `set_limits()` is always called before queueing any records\nto the writer. This would make us error out in all situations where the\ncalling order is wrong.\n\nThere are two ways I can see us doing that:\n\n - Detect any state written by `writer_add_record()` and error out if\n it's set when `reftable_writer_set_limits()` is called.\n\n - Adapt `reftable_writer_new()` so that it takes the update indices as\n input and drop `reftable_writer_set_limits()` altogether.\n\nThe latter might be preferable as you basically want to set limits in\nall (most?) situations anyway.\n\n> +\tif (w->next)\n> +\t\tBUG(\"update index modified after writing first block\");\n\nLet's not use BUG, but rather return a `REFTABLE_API_ERROR` error. It\nrequires a bit more plumbing because we'll also hvae to adapt all\ncallers to handle errors. But on the one hand we don't want to die in\nlibrary code. And on the other hand we don't want to keep on adding more\ndependencies on \"git-compat-util.h\".\n\nPatrick\n"},{"id":"510776","messageId":"Z4oi9i_EHLaPjk0g@pks.im","threadId":"62820","inReplyTo":"20250117-461-corrupted-reftable-followup-v1-1-70ee605ae3fe@gmail.com","subject":"Re: [PATCH 1/3] refs: mark `ref_transaction_update_reflog()` as static","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-17T09:29:26Z","receivedAt":"2025-01-17T09:29:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jan 17, 2025 at 08:59:12AM +0100, Karthik Nayak wrote:\n> diff --git a/refs.h b/refs.h\n> index a0cdd99250e8286b55808b697b0a94afac5d8319..09be47afbee51e99f4ae49588cd65596ccfcb07e 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -771,20 +771,6 @@ int ref_transaction_update(struct ref_transaction *transaction,\n> \t\t\t unsigned int flags, const char *msg,\n> \t\t\t struct strbuf *err);\n> \n> -/*\n> - * Similar to`ref_transaction_update`, but this function is only for adding\n> - * a reflog update. Supports providing custom committer information. The index\n> - * field can be utiltized to order updates as desired. When not used, the\n> - * updates default to being ordered by refname.\n> - */\n\nDo we maybe want to move the comment over? The explanation of the index\nfield seems useful to me.\n\nPatrick\n\n> -int ref_transaction_update_reflog(struct ref_transaction *transaction,\n> -\t\t\t\t const char *refname,\n> -\t\t\t\t const struct object_id *new_oid,\n> -\t\t\t\t const struct object_id *old_oid,\n> -\t\t\t\t const char *committer_info, unsigned int flags,\n> -\t\t\t\t const char *msg, unsigned int index,\n> -\t\t\t\t struct strbuf *err);\n> -\n> /*\n> * Add a reference creation to transaction. new_oid is the value that\n> * the reference should have after the update; it must not be\n"},{"id":"510942","messageId":"CAOLa=ZTWxy65o4+=nF8r3+Z44DjcpEvwHhocAcM8L2VqZucR1g@mail.gmail.com","threadId":"62820","inReplyTo":"Z4oi9i_EHLaPjk0g@pks.im","subject":"Re: [PATCH 1/3] refs: mark `ref_transaction_update_reflog()` as static","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-20T11:17:31Z","receivedAt":"2025-01-20T11:17:33Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Fri, Jan 17, 2025 at 08:59:12AM +0100, Karthik Nayak wrote:\n>> diff --git a/refs.h b/refs.h\n>> index a0cdd99250e8286b55808b697b0a94afac5d8319..09be47afbee51e99f4ae49588cd65596ccfcb07e 100644\n>> --- a/refs.h\n>> +++ b/refs.h\n>> @@ -771,20 +771,6 @@ int ref_transaction_update(struct ref_transaction *transaction,\n>> \t\t\t unsigned int flags, const char *msg,\n>> \t\t\t struct strbuf *err);\n>>\n>> -/*\n>> - * Similar to`ref_transaction_update`, but this function is only for adding\n>> - * a reflog update. Supports providing custom committer information. The index\n>> - * field can be utiltized to order updates as desired. When not used, the\n>> - * updates default to being ordered by refname.\n>> - */\n>\n> Do we maybe want to move the comment over? The explanation of the index\n> field seems useful to me.\n>\n\nYeah, that'd be useful indeed, let me move it.\n\n> Patrick\n>\n>> -int ref_transaction_update_reflog(struct ref_transaction *transaction,\n>> -\t\t\t\t const char *refname,\n>> -\t\t\t\t const struct object_id *new_oid,\n>> -\t\t\t\t const struct object_id *old_oid,\n>> -\t\t\t\t const char *committer_info, unsigned int flags,\n>> -\t\t\t\t const char *msg, unsigned int index,\n>> -\t\t\t\t struct strbuf *err);\n>> -\n>> /*\n>> * Add a reference creation to transaction. new_oid is the value that\n>> * the reference should have after the update; it must not be\n"},{"id":"510944","messageId":"CAOLa=ZQpg+8vg+3XMcG4XyoaOMvtMV7KuSkQhJXTkFiPatO7Lw@mail.gmail.com","threadId":"62820","inReplyTo":"Z4oi55XewgX2deTm@pks.im","subject":"Re: [PATCH 3/3] reftable: prevent 'update_index' changes after header write","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-20T11:47:02Z","receivedAt":"2025-01-20T11:47:04Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Fri, Jan 17, 2025 at 08:59:14AM +0100, Karthik Nayak wrote:\n>> diff --git a/reftable/writer.c b/reftable/writer.c\n>> index 740c98038eaf883258bef4988f78977ac7e4a75a..c602b873543790e36178f797ed9f98112671f97f 100644\n>> --- a/reftable/writer.c\n>> +++ b/reftable/writer.c\n>> @@ -182,6 +182,13 @@ int reftable_writer_new(struct reftable_writer **out,\n>> void reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,\n>> \t\t\t\tuint64_t max)\n>> {\n>> +\t/*\n>> +\t * The limits shouldn't be modified post writing the first block, else\n>> +\t * it would cause a mismatch between the header and the footer.\n>> +\t */\n>\n> Can we make this *even* stricter? I think that this is something that is\n> easy to do wrong, and the fact that it only triggers in some situations\n> of misuse may easily make tests miss this issue. So ideally, we should\n> assert that `set_limits()` is always called before queueing any records\n> to the writer. This would make us error out in all situations where the\n> calling order is wrong.\n>\n\nI agree here, it makes sense to make this stricter. Like you mentioned,\ncurrently they are independent. The only way to enforce the limits is to\nensure that they are dependent.\n\n> There are two ways I can see us doing that:\n>\n> - Detect any state written by `writer_add_record()` and error out if\n> it's set when `reftable_writer_set_limits()` is called.\n>\n\nYeah I think this would be simple to do. I guess we can check\n`w->last_key` is set, since any record write would modify that.\n\n> - Adapt `reftable_writer_new()` so that it takes the update indices as\n> input and drop `reftable_writer_set_limits()` altogether.\n>\n\nThis one is a bit harder to do because of our flow. Generally the writer\nis provided to callers via a callback function passed to\n`reftable_addition_add()`. I guess I could simply pass the data:\n\n caller -> reftable_addition_add() -> reftable_writer_new()\n\nAny direct users of `reftable_writer_new()` would simply pass the data\ndirectly.\n\nI'll play around and see if this is doable without too much refactoring\nand have something in the next version.\n\n> The latter might be preferable as you basically want to set limits in\n> all (most?) situations anyway.\n>\n>> +\tif (w->next)\n>> +\t\tBUG(\"update index modified after writing first block\");\n>\n> Let's not use BUG, but rather return a `REFTABLE_API_ERROR` error. It\n> requires a bit more plumbing because we'll also hvae to adapt all\n> callers to handle errors. But on the one hand we don't want to die in\n> library code. And on the other hand we don't want to keep on adding more\n> dependencies on \"git-compat-util.h\".\n>\n\nFair enough, thanks for explaining.\n\n> Patrick\n"},{"id":"510945","messageId":"CAOLa=ZQhcGyAXCHug_5=g8qnRMPJ10xRowHNY7c0RcNH_u5=9g@mail.gmail.com","threadId":"62820","inReplyTo":"CAOLa=ZQpg+8vg+3XMcG4XyoaOMvtMV7KuSkQhJXTkFiPatO7Lw@mail.gmail.com","subject":"Re: [PATCH 3/3] reftable: prevent 'update_index' changes after header write","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-20T12:18:15Z","receivedAt":"2025-01-20T12:18:18Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Karthik Nayak writes:\n\n> Patrick Steinhardt writes:\n>\n>> On Fri, Jan 17, 2025 at 08:59:14AM +0100, Karthik Nayak wrote:\n>>> diff --git a/reftable/writer.c b/reftable/writer.c\n>>> index 740c98038eaf883258bef4988f78977ac7e4a75a..c602b873543790e36178f797ed9f98112671f97f 100644\n>>> --- a/reftable/writer.c\n>>> +++ b/reftable/writer.c\n>>> @@ -182,6 +182,13 @@ int reftable_writer_new(struct reftable_writer **out,\n>>> void reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,\n>>> \t\t\t\tuint64_t max)\n>>> {\n>>> +\t/*\n>>> +\t * The limits shouldn't be modified post writing the first block, else\n>>> +\t * it would cause a mismatch between the header and the footer.\n>>> +\t */\n>>\n>> Can we make this *even* stricter? I think that this is something that is\n>> easy to do wrong, and the fact that it only triggers in some situations\n>> of misuse may easily make tests miss this issue. So ideally, we should\n>> assert that `set_limits()` is always called before queueing any records\n>> to the writer. This would make us error out in all situations where the\n>> calling order is wrong.\n>>\n>\n> I agree here, it makes sense to make this stricter. Like you mentioned,\n> currently they are independent. The only way to enforce the limits is to\n> ensure that they are dependent.\n>\n>> There are two ways I can see us doing that:\n>>\n>> - Detect any state written by `writer_add_record()` and error out if\n>> it's set when `reftable_writer_set_limits()` is called.\n>>\n>\n> Yeah I think this would be simple to do. I guess we can check\n> `w->last_key` is set, since any record write would modify that.\n>\n>> - Adapt `reftable_writer_new()` so that it takes the update indices as\n>> input and drop `reftable_writer_set_limits()` altogether.\n>>\n>\n> This one is a bit harder to do because of our flow. Generally the writer\n> is provided to callers via a callback function passed to\n> `reftable_addition_add()`. I guess I could simply pass the data:\n>\n> caller -> reftable_addition_add() -> reftable_writer_new()\n>\n> Any direct users of `reftable_writer_new()` would simply pass the data\n> directly.\n>\n> I'll play around and see if this is doable without too much refactoring\n> and have something in the next version.\n\nIt seems like `reftable_addition_add()` is internally and externally\ncalled by a lot of functions and as such, modifying them all would be\ntedious. Another idea is to modify the function pointer that\n`reftable_addition_add()` receives so that the `write_table` argument\nreceives a function which would set the limits, but this seems like bad\ndesign to me.\n\nSo the best/easiest way to do this is to error out if any state is set\nbefore calling `reftable_writer_set_limits()`. I also realized that if\n`reftable_writer_set_limits()` isn't called, the writer errors anyways.\nSo just ensuring that `reftable_writer_set_limits()` checks for any\nstate being set would work well.\n\n[snip]\n"},{"id":"510987","messageId":"20250121-461-corrupted-reftable-followup-v2-0-37e26c7a79b4@gmail.com","threadId":"62820","inReplyTo":"20250117-461-corrupted-reftable-followup-v1-0-70ee605ae3fe@gmail.com","subject":"[PATCH v2 0/3] refs: small followups to the migration corruption fix","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-21T03:34:09Z","receivedAt":"2025-01-21T03:34:32Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"This is a follow up to the bug that was reported [1] around `git refs\nmigrate --ref-format=reftable` where the migration would fail for\nrepositories with reflogs with lots of entries. This was caused due to a\nmismatch in the reftable's header and footer, specifically WRT the\n'max_update_index'.\n\nWhile there was a fix posted. This series is a small followup to fix\nsome of the topics discussed there:\n1. To mark `ref_transaction_update_reflog()` as static since it is only\nused internally within 'refs.c'.\n2. To change the type of 'max_index' from 'unsigned int' to 'uint64_t'.\nThis would be much safer for large repositories with millions of files\nand on 32bit systems.\n3. To add a safeguard to prevent 'update_index' changes post any record\naddition. This is a preventive measure to ensure such bugs don't arise\nin the future.\n\nThis is based on top of master 757161efcc (Sync with Git 2.48.1,\n2025-01-13) with 'kn/reflog-migration-fix' merged in.\n\n[1]: https://lore.kernel.org/r/Z4UbkcmJAU1MT-Rs@tapette.crustytoothpaste.net\n\nSigned-off-by: Karthik Nayak \n---\nChanges in v2:\n- Commit 1: Keep the function comments.\n- Commit 3: Modify the requirement to be for any record writes instead\nof the first block write. This ensures that the limits need to be set \nbefore any records being added. Instead of using `BUG()`, return \n`REFTABLE_API_ERROR`. Handle all callers as needed.\n- Link to v1: https://lore.kernel.org/r/20250117-461-corrupted-reftable-followup-v1-0-70ee605ae3fe@gmail.com\n\n---\nKarthik Nayak (3):\n refs: mark `ref_transaction_update_reflog()` as static\n refs: use 'uint64_t' for 'ref_update.index'\n reftable: prevent 'update_index' changes after adding records\n\n refs.c | 24 ++++++++++++++++--------\n refs.h | 14 --------------\n refs/refs-internal.h | 4 ++--\n refs/reftable-backend.c | 22 ++++++++++++++++------\n reftable/reftable-error.h | 1 +\n reftable/reftable-writer.h | 24 ++++++++++++++----------\n reftable/stack.c | 6 ++++--\n reftable/writer.c | 13 +++++++++++--\n t/unit-tests/t-reftable-stack.c | 8 +++++---\n 9 files changed, 69 insertions(+), 47 deletions(-)\n---\n\nRange-diff versus v1:\n\n1: 82a4f950dd ! 1: 5821ddfbe2 refs: mark `ref_transaction_update_reflog()` as static\n @@ refs.c: int ref_transaction_update(struct ref_transaction *transaction,\n -\t\t\t\t const char *committer_info, unsigned int flags,\n -\t\t\t\t const char *msg, unsigned int index,\n -\t\t\t\t struct strbuf *err)\n ++/*\n ++ * Similar to`ref_transaction_update`, but this function is only for adding\n ++ * a reflog update. Supports providing custom committer information. The index\n ++ * field can be utiltized to order updates as desired. When not used, the\n ++ * updates default to being ordered by refname.\n ++ */\n +static int ref_transaction_update_reflog(struct ref_transaction *transaction,\n +\t\t\t\t\t const char *refname,\n +\t\t\t\t\t const struct object_id *new_oid,\n2: 51ed04e95d = 2: 8025cfadf2 refs: use 'uint64_t' for 'ref_update.index'\n3: 34e3001e29 < -: ---------- reftable: prevent 'update_index' changes after header write\n-: ---------- > 3: 4b0416ce20 reftable: prevent 'update_index' changes after adding records\n\n\n---\n\nbase-commit: a5aa44e7930761cb900813d971b4105f901818fb\nchange-id: 20250117-461-corrupted-reftable-followup-eb0e4fd1a723\n\nThanks\n- Karthik\n\n"},{"id":"510988","messageId":"20250121-461-corrupted-reftable-followup-v2-1-37e26c7a79b4@gmail.com","threadId":"62820","inReplyTo":"20250121-461-corrupted-reftable-followup-v2-0-37e26c7a79b4@gmail.com","subject":"[PATCH v2 1/3] refs: mark `ref_transaction_update_reflog()` as static","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-21T03:34:10Z","receivedAt":"2025-01-21T03:34:34Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `ref_transaction_update_reflog()` function is only used within\n'refs.c', so mark it as static.\n\nReported-by: Junio C Hamano \nSigned-off-by: Karthik Nayak \n---\n refs.c | 22 +++++++++++++++-------\n refs.h | 14 --------------\n 2 files changed, 15 insertions(+), 21 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex f7b6f0f897eb58665e10a2efd3eb53c3f72abe61..ad6d774717150f1fe68a59c629e05e49a469693f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1318,13 +1318,21 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \treturn 0;\n }\n \n-int ref_transaction_update_reflog(struct ref_transaction *transaction,\n-\t\t\t\t const char *refname,\n-\t\t\t\t const struct object_id *new_oid,\n-\t\t\t\t const struct object_id *old_oid,\n-\t\t\t\t const char *committer_info, unsigned int flags,\n-\t\t\t\t const char *msg, unsigned int index,\n-\t\t\t\t struct strbuf *err)\n+/*\n+ * Similar to`ref_transaction_update`, but this function is only for adding\n+ * a reflog update. Supports providing custom committer information. The index\n+ * field can be utiltized to order updates as desired. When not used, the\n+ * updates default to being ordered by refname.\n+ */\n+static int ref_transaction_update_reflog(struct ref_transaction *transaction,\n+\t\t\t\t\t const char *refname,\n+\t\t\t\t\t const struct object_id *new_oid,\n+\t\t\t\t\t const struct object_id *old_oid,\n+\t\t\t\t\t const char *committer_info,\n+\t\t\t\t\t unsigned int flags,\n+\t\t\t\t\t const char *msg,\n+\t\t\t\t\t unsigned int index,\n+\t\t\t\t\t struct strbuf *err)\n {\n \tstruct ref_update *update;\n \ndiff --git a/refs.h b/refs.h\nindex a0cdd99250e8286b55808b697b0a94afac5d8319..09be47afbee51e99f4ae49588cd65596ccfcb07e 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -771,20 +771,6 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \t\t\t unsigned int flags, const char *msg,\n \t\t\t struct strbuf *err);\n \n-/*\n- * Similar to`ref_transaction_update`, but this function is only for adding\n- * a reflog update. Supports providing custom committer information. The index\n- * field can be utiltized to order updates as desired. When not used, the\n- * updates default to being ordered by refname.\n- */\n-int ref_transaction_update_reflog(struct ref_transaction *transaction,\n-\t\t\t\t const char *refname,\n-\t\t\t\t const struct object_id *new_oid,\n-\t\t\t\t const struct object_id *old_oid,\n-\t\t\t\t const char *committer_info, unsigned int flags,\n-\t\t\t\t const char *msg, unsigned int index,\n-\t\t\t\t struct strbuf *err);\n-\n /*\n * Add a reference creation to transaction. new_oid is the value that\n * the reference should have after the update; it must not be\n\n-- \n2.47.0\n\n"},{"id":"510989","messageId":"20250121-461-corrupted-reftable-followup-v2-2-37e26c7a79b4@gmail.com","threadId":"62820","inReplyTo":"20250121-461-corrupted-reftable-followup-v2-0-37e26c7a79b4@gmail.com","subject":"[PATCH v2 2/3] refs: use 'uint64_t' for 'ref_update.index'","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-21T03:34:11Z","receivedAt":"2025-01-21T03:34:36Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The 'ref_update.index' variable is used to store an index for a given\nreference update. This index is used to order the updates in a\npredetermined order, while the default ordering is alphabetical as per\nthe refname.\n\nFor large repositories with millions of references, it should be safer\nto use 'uint64_t'. Let's do that. This also is applied for all other\ncode sections where we store 'index' and pass it around.\n\nReported-by: brian m. carlson \nSigned-off-by: Karthik Nayak \n---\n refs.c | 4 ++--\n refs/refs-internal.h | 4 ++--\n refs/reftable-backend.c | 2 +-\n 3 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex ad6d774717150f1fe68a59c629e05e49a469693f..ef04f403a6a3a34f9156b4cf68c3daa29c9cbad6 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1331,7 +1331,7 @@ static int ref_transaction_update_reflog(struct ref_transaction *transaction,\n \t\t\t\t\t const char *committer_info,\n \t\t\t\t\t unsigned int flags,\n \t\t\t\t\t const char *msg,\n-\t\t\t\t\t unsigned int index,\n+\t\t\t\t\t uint64_t index,\n \t\t\t\t\t struct strbuf *err)\n {\n \tstruct ref_update *update;\n@@ -2813,7 +2813,7 @@ static int migrate_one_ref(const char *refname, const char *referent UNUSED, con\n }\n \n struct reflog_migration_data {\n-\tunsigned int index;\n+\tuint64_t index;\n \tconst char *refname;\n \tstruct ref_store *old_refs;\n \tstruct ref_transaction *transaction;\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex aaab711bb96844755dfa600d37efdb25b30a0765..8894b43d1d1a327d404d3923c507d2d39649de19 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -120,7 +120,7 @@ struct ref_update {\n \t * when migrating reflogs and we want to ensure we carry over the\n \t * same order.\n \t */\n-\tunsigned int index;\n+\tuint64_t index;\n \n \t/*\n \t * If this ref_update was split off of a symref update via\n@@ -203,7 +203,7 @@ struct ref_transaction {\n \tenum ref_transaction_state state;\n \tvoid *backend_data;\n \tunsigned int flags;\n-\tunsigned int max_index;\n+\tuint64_t max_index;\n };\n \n /*\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 289496058e6eb4d3e3aef96ca70219cd4ff78eae..6814c87bc618229ac8a70b904be3f850371ad876 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -942,7 +942,7 @@ struct write_transaction_table_arg {\n \tsize_t updates_nr;\n \tsize_t updates_alloc;\n \tsize_t updates_expected;\n-\tunsigned int max_index;\n+\tuint64_t max_index;\n };\n \n struct reftable_transaction_data {\n\n-- \n2.47.0\n\n"},{"id":"510990","messageId":"20250121-461-corrupted-reftable-followup-v2-3-37e26c7a79b4@gmail.com","threadId":"62820","inReplyTo":"20250121-461-corrupted-reftable-followup-v2-0-37e26c7a79b4@gmail.com","subject":"[PATCH v2 3/3] reftable: prevent 'update_index' changes after adding records","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-21T03:34:12Z","receivedAt":"2025-01-21T03:34:38Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The function `reftable_writer_set_limits()` allows updating the\n'min_update_index' and 'max_update_index' of a reftable writer. These\nvalues are written to both the writer's header and footer.\n\nSince the header is written during the first block write, any subsequent\nchanges to the update index would create a mismatch between the header\nand footer values. The footer would contain the newer values while the\nheader retained the original ones.\n\nTo fix this bug, prevent callers from updating these values after any\nrecord is written. To do this, modify the function to return an error\nwhenever the limits are modified after any record adds. Check for record\nadds within `reftable_writer_set_limits()` by checking the `last_key`\nvariable, which is set whenever a new record is added.\n\nModify all callers of the function to anticipate a return type and\nhandle it accordingly.\n\nHelped-by: Patrick Steinhardt \nSigned-off-by: Karthik Nayak \n---\n refs/reftable-backend.c | 20 +++++++++++++++-----\n reftable/reftable-error.h | 1 +\n reftable/reftable-writer.h | 24 ++++++++++++++----------\n reftable/stack.c | 6 ++++--\n reftable/writer.c | 13 +++++++++++--\n t/unit-tests/t-reftable-stack.c | 8 +++++---\n 6 files changed, 50 insertions(+), 22 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 6814c87bc618229ac8a70b904be3f850371ad876..9cfb0cb26721a9425c3b4a374f7b41e192037315 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1443,7 +1443,9 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n \t * multiple entries. Each entry will contain a different update_index,\n \t * so set the limits accordingly.\n \t */\n-\treftable_writer_set_limits(writer, ts, ts + arg->max_index);\n+\tret = reftable_writer_set_limits(writer, ts, ts + arg->max_index);\n+\tif (ret < 0)\n+\t\tgoto done;\n \n \tfor (i = 0; i < arg->updates_nr; i++) {\n \t\tstruct reftable_transaction_update *tx_update = &arg->updates[i];\n@@ -1766,7 +1768,9 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)\n \tdeletion_ts = creation_ts = reftable_stack_next_update_index(arg->be->stack);\n \tif (arg->delete_old)\n \t\tcreation_ts++;\n-\treftable_writer_set_limits(writer, deletion_ts, creation_ts);\n+\tret = reftable_writer_set_limits(writer, deletion_ts, creation_ts);\n+\tif (ret < 0)\n+\t\tgoto done;\n \n \t/*\n \t * Add the new reference. If this is a rename then we also delete the\n@@ -2298,7 +2302,9 @@ static int write_reflog_existence_table(struct reftable_writer *writer,\n \tif (ret <= 0)\n \t\tgoto done;\n \n-\treftable_writer_set_limits(writer, ts, ts);\n+\tret = reftable_writer_set_limits(writer, ts, ts);\n+\tif (ret < 0)\n+\t\tgoto done;\n \n \t/*\n \t * The existence entry has both old and new object ID set to the\n@@ -2357,7 +2363,9 @@ static int write_reflog_delete_table(struct reftable_writer *writer, void *cb_da\n \tuint64_t ts = reftable_stack_next_update_index(arg->stack);\n \tint ret;\n \n-\treftable_writer_set_limits(writer, ts, ts);\n+\tret = reftable_writer_set_limits(writer, ts, ts);\n+\tif (ret < 0)\n+\t\tgoto out;\n \n \tret = reftable_stack_init_log_iterator(arg->stack, &it);\n \tif (ret < 0)\n@@ -2434,7 +2442,9 @@ static int write_reflog_expiry_table(struct reftable_writer *writer, void *cb_da\n \t\tif (arg->records[i].value_type == REFTABLE_LOG_UPDATE)\n \t\t\tlive_records++;\n \n-\treftable_writer_set_limits(writer, ts, ts);\n+\tret = reftable_writer_set_limits(writer, ts, ts);\n+\tif (ret < 0)\n+\t\treturn ret;\n \n \tif (!is_null_oid(&arg->update_oid)) {\n \t\tstruct reftable_ref_record ref = {0};\ndiff --git a/reftable/reftable-error.h b/reftable/reftable-error.h\nindex f4048265629fe456207b88620658193f770a84f0..a7e33d964d0cfe5546f588d26c0fcb66ab326828 100644\n--- a/reftable/reftable-error.h\n+++ b/reftable/reftable-error.h\n@@ -30,6 +30,7 @@ enum reftable_error {\n \n \t/* Misuse of the API:\n \t * - on writing a record with NULL refname.\n+\t * - on writing a record before setting the writer limits.\n \t * - on writing a reftable_ref_record outside the table limits\n \t * - on writing a ref or log record before the stack's\n \t * next_update_inde*x\ndiff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h\nindex 5f9afa620bb00de66c311765fb0ae8c6f56401ae..1ea014d389cc47f173279e3234a82f3fcbc807a0 100644\n--- a/reftable/reftable-writer.h\n+++ b/reftable/reftable-writer.h\n@@ -124,17 +124,21 @@ int reftable_writer_new(struct reftable_writer **out,\n \t\t\tint (*flush_func)(void *),\n \t\t\tvoid *writer_arg, const struct reftable_write_options *opts);\n \n-/* Set the range of update indices for the records we will add. When writing a\n- table into a stack, the min should be at least\n- reftable_stack_next_update_index(), or REFTABLE_API_ERROR is returned.\n-\n- For transactional updates to a stack, typically min==max, and the\n- update_index can be obtained by inspeciting the stack. When converting an\n- existing ref database into a single reftable, this would be a range of\n- update-index timestamps.\n+/*\n+ * Set the range of update indices for the records we will add. When writing a\n+ * table into a stack, the min should be at least\n+ * reftable_stack_next_update_index(), or REFTABLE_API_ERROR is returned.\n+ *\n+ * For transactional updates to a stack, typically min==max, and the\n+ * update_index can be obtained by inspeciting the stack. When converting an\n+ * existing ref database into a single reftable, this would be a range of\n+ * update-index timestamps.\n+ *\n+ * The function should be called before adding any records to the writer. If not\n+ * it will fail with REFTABLE_API_ERROR.\n */\n-void reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,\n-\t\t\t\tuint64_t max);\n+int reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,\n+\t\t\t uint64_t max);\n \n /*\n Add a reftable_ref_record. The record should have names that come after\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 531660a49f0948c33041831ee0d740feacb22b2f..9649dbbb04c51e106ee752f14481bbad381cb348 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -1058,8 +1058,10 @@ static int stack_write_compact(struct reftable_stack *st,\n \n \tfor (size_t i = first; i <= last; i++)\n \t\tst->stats.bytes += st->readers[i]->size;\n-\treftable_writer_set_limits(wr, st->readers[first]->min_update_index,\n-\t\t\t\t st->readers[last]->max_update_index);\n+\terr = reftable_writer_set_limits(wr, st->readers[first]->min_update_index,\n+\t\t\t\t\t st->readers[last]->max_update_index);\n+\tif (err < 0)\n+\t\tgoto done;\n \n \terr = reftable_merged_table_new(&mt, st->readers + first, subtabs_len,\n \t\t\t\t\tst->opts.hash_id);\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 740c98038eaf883258bef4988f78977ac7e4a75a..03acbdbcce75fd51820c5fb016bd94f0f7f4914a 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -179,11 +179,20 @@ int reftable_writer_new(struct reftable_writer **out,\n \treturn 0;\n }\n \n-void reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,\n-\t\t\t\tuint64_t max)\n+int reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,\n+\t\t\t uint64_t max)\n {\n+\t/*\n+\t * The limits should be set before any records are added to the writer.\n+\t * Check if any records were added by checking if `last_key` was set.\n+\t */\n+\tif (w->last_key.len)\n+\t\treturn REFTABLE_API_ERROR;\n+\n \tw->min_update_index = min;\n \tw->max_update_index = max;\n+\n+\treturn 0;\n }\n \n static void writer_release(struct reftable_writer *w)\ndiff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c\nindex aeec195b2b1014445d71c5db39a9795017fd8ff2..b23edf18a7d75b0c2292490ad06d4dfaaa571e79 100644\n--- a/t/unit-tests/t-reftable-stack.c\n+++ b/t/unit-tests/t-reftable-stack.c\n@@ -103,7 +103,8 @@ static void t_read_file(void)\n static int write_test_ref(struct reftable_writer *wr, void *arg)\n {\n \tstruct reftable_ref_record *ref = arg;\n-\treftable_writer_set_limits(wr, ref->update_index, ref->update_index);\n+\tcheck(!reftable_writer_set_limits(wr, ref->update_index,\n+\t\t\t\t\t ref->update_index));\n \treturn reftable_writer_add_ref(wr, ref);\n }\n \n@@ -143,7 +144,8 @@ static int write_test_log(struct reftable_writer *wr, void *arg)\n {\n \tstruct write_log_arg *wla = arg;\n \n-\treftable_writer_set_limits(wr, wla->update_index, wla->update_index);\n+\tcheck(!reftable_writer_set_limits(wr, wla->update_index,\n+\t\t\t\t\t wla->update_index));\n \treturn reftable_writer_add_log(wr, wla->log);\n }\n \n@@ -961,7 +963,7 @@ static void t_reflog_expire(void)\n \n static int write_nothing(struct reftable_writer *wr, void *arg UNUSED)\n {\n-\treftable_writer_set_limits(wr, 1, 1);\n+\tcheck(!reftable_writer_set_limits(wr, 1, 1));\n \treturn 0;\n }\n \n\n-- \n2.47.0\n\n"},{"id":"510993","messageId":"Z49FHQgsFSX6sTxu@pks.im","threadId":"62820","inReplyTo":"20250121-461-corrupted-reftable-followup-v2-3-37e26c7a79b4@gmail.com","subject":"Re: [PATCH v2 3/3] reftable: prevent 'update_index' changes after adding records","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-21T06:56:29Z","receivedAt":"2025-01-21T06:56:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jan 21, 2025 at 04:34:12AM +0100, Karthik Nayak wrote:\n> The function `reftable_writer_set_limits()` allows updating the\n> 'min_update_index' and 'max_update_index' of a reftable writer. These\n> values are written to both the writer's header and footer.\n> \n> Since the header is written during the first block write, any subsequent\n> changes to the update index would create a mismatch between the header\n> and footer values. The footer would contain the newer values while the\n> header retained the original ones.\n> \n> To fix this bug, prevent callers from updating these values after any\n\nNit: it's not really fixing a bug, but protecting us against it. Not\nworth a reroll though, from my point of view.\n\n> diff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h\n> index 5f9afa620bb00de66c311765fb0ae8c6f56401ae..1ea014d389cc47f173279e3234a82f3fcbc807a0 100644\n> --- a/reftable/reftable-writer.h\n> +++ b/reftable/reftable-writer.h\n> @@ -124,17 +124,21 @@ int reftable_writer_new(struct reftable_writer **out,\n> \t\t\tint (*flush_func)(void *),\n> \t\t\tvoid *writer_arg, const struct reftable_write_options *opts);\n> \n> -/* Set the range of update indices for the records we will add. When writing a\n> - table into a stack, the min should be at least\n> - reftable_stack_next_update_index(), or REFTABLE_API_ERROR is returned.\n> -\n> - For transactional updates to a stack, typically min==max, and the\n> - update_index can be obtained by inspeciting the stack. When converting an\n> - existing ref database into a single reftable, this would be a range of\n> - update-index timestamps.\n> +/*\n> + * Set the range of update indices for the records we will add. When writing a\n> + * table into a stack, the min should be at least\n> + * reftable_stack_next_update_index(), or REFTABLE_API_ERROR is returned.\n> + *\n> + * For transactional updates to a stack, typically min==max, and the\n> + * update_index can be obtained by inspeciting the stack. When converting an\n> + * existing ref database into a single reftable, this would be a range of\n> + * update-index timestamps.\n> + *\n> + * The function should be called before adding any records to the writer. If not\n> + * it will fail with REFTABLE_API_ERROR.\n> */\n\nThanks for updating this. I think the reftable library is one of those\ncode areas where it makes sense to sneak in a formatting fix every now\nand then because its coding style is quite alien to Git's own in some\nplaces. We could also do it all in one go, but I strongly doubt that it\nwould be worth the churn.\n\n> -void reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,\n> -\t\t\t\tuint64_t max);\n> +int reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,\n> +\t\t\t uint64_t max);\n> \n> /*\n> Add a reftable_ref_record. The record should have names that come after\n\n> diff --git a/reftable/writer.c b/reftable/writer.c\n> index 740c98038eaf883258bef4988f78977ac7e4a75a..03acbdbcce75fd51820c5fb016bd94f0f7f4914a 100644\n> --- a/reftable/writer.c\n> +++ b/reftable/writer.c\n> @@ -179,11 +179,20 @@ int reftable_writer_new(struct reftable_writer **out,\n> \treturn 0;\n> }\n> \n> -void reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,\n> -\t\t\t\tuint64_t max)\n> +int reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,\n> +\t\t\t uint64_t max)\n> {\n> +\t/*\n> +\t * The limits should be set before any records are added to the writer.\n> +\t * Check if any records were added by checking if `last_key` was set.\n> +\t */\n> +\tif (w->last_key.len)\n> +\t\treturn REFTABLE_API_ERROR;\n\nHm. Using the last key feels somewhat dangerous to me as it does get\nreset at times, e.g. when finishing writing the current section. It\n_should_ work, but overall it just feels a tad to disconnected from the\nthing that we actually want to check.\n\nHow about we instead use `next`? This variable records the offset of the\nnext block we're about to write, and `writer_flush_nonempty_block()`\nuses it directly to check whether we're currently writing the first\nblock in order to decide whether it needs to write a header or not. If\nit's 0, we know that we haven't written the first block yet. That feels\nmuch closer aligned with what we're checking.\n\n> diff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c\n> index aeec195b2b1014445d71c5db39a9795017fd8ff2..b23edf18a7d75b0c2292490ad06d4dfaaa571e79 100644\n> --- a/t/unit-tests/t-reftable-stack.c\n> +++ b/t/unit-tests/t-reftable-stack.c\n\nCan we maybe add a unit test that demonstrates the error?\n\nPatrick\n"},{"id":"510996","messageId":"CAOLa=ZQBoSpXiLHtwgaeg8vnChrqtvzcettRQZFvXZQrpr0=8A@mail.gmail.com","threadId":"62820","inReplyTo":"Z49FHQgsFSX6sTxu@pks.im","subject":"Re: [PATCH v2 3/3] reftable: prevent 'update_index' changes after adding records","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-21T11:44:27Z","receivedAt":"2025-01-21T11:44:29Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Tue, Jan 21, 2025 at 04:34:12AM +0100, Karthik Nayak wrote:\n>> The function `reftable_writer_set_limits()` allows updating the\n>> 'min_update_index' and 'max_update_index' of a reftable writer. These\n>> values are written to both the writer's header and footer.\n>>\n>> Since the header is written during the first block write, any subsequent\n>> changes to the update index would create a mismatch between the header\n>> and footer values. The footer would contain the newer values while the\n>> header retained the original ones.\n>>\n>> To fix this bug, prevent callers from updating these values after any\n>\n> Nit: it's not really fixing a bug, but protecting us against it. Not\n> worth a reroll though, from my point of view.\n>\n\nThat's right, I'll add that in.\n\n>> diff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h\n>> index 5f9afa620bb00de66c311765fb0ae8c6f56401ae..1ea014d389cc47f173279e3234a82f3fcbc807a0 100644\n>> --- a/reftable/reftable-writer.h\n>> +++ b/reftable/reftable-writer.h\n>> @@ -124,17 +124,21 @@ int reftable_writer_new(struct reftable_writer **out,\n>> \t\t\tint (*flush_func)(void *),\n>> \t\t\tvoid *writer_arg, const struct reftable_write_options *opts);\n>>\n>> -/* Set the range of update indices for the records we will add. When writing a\n>> - table into a stack, the min should be at least\n>> - reftable_stack_next_update_index(), or REFTABLE_API_ERROR is returned.\n>> -\n>> - For transactional updates to a stack, typically min==max, and the\n>> - update_index can be obtained by inspeciting the stack. When converting an\n>> - existing ref database into a single reftable, this would be a range of\n>> - update-index timestamps.\n>> +/*\n>> + * Set the range of update indices for the records we will add. When writing a\n>> + * table into a stack, the min should be at least\n>> + * reftable_stack_next_update_index(), or REFTABLE_API_ERROR is returned.\n>> + *\n>> + * For transactional updates to a stack, typically min==max, and the\n>> + * update_index can be obtained by inspeciting the stack. When converting an\n>> + * existing ref database into a single reftable, this would be a range of\n>> + * update-index timestamps.\n>> + *\n>> + * The function should be called before adding any records to the writer. If not\n>> + * it will fail with REFTABLE_API_ERROR.\n>> */\n>\n> Thanks for updating this. I think the reftable library is one of those\n> code areas where it makes sense to sneak in a formatting fix every now\n> and then because its coding style is quite alien to Git's own in some\n> places. We could also do it all in one go, but I strongly doubt that it\n> would be worth the churn.\n>\n\nGenerally I try to sneak in small fixes like this around code being\ntouched. I know it is a little more toll on reviewers, but small\nimprovements do add up.\n\n>> -void reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,\n>> -\t\t\t\tuint64_t max);\n>> +int reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,\n>> +\t\t\t uint64_t max);\n>>\n>> /*\n>> Add a reftable_ref_record. The record should have names that come after\n>\n>> diff --git a/reftable/writer.c b/reftable/writer.c\n>> index 740c98038eaf883258bef4988f78977ac7e4a75a..03acbdbcce75fd51820c5fb016bd94f0f7f4914a 100644\n>> --- a/reftable/writer.c\n>> +++ b/reftable/writer.c\n>> @@ -179,11 +179,20 @@ int reftable_writer_new(struct reftable_writer **out,\n>> \treturn 0;\n>> }\n>>\n>> -void reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,\n>> -\t\t\t\tuint64_t max)\n>> +int reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,\n>> +\t\t\t uint64_t max)\n>> {\n>> +\t/*\n>> +\t * The limits should be set before any records are added to the writer.\n>> +\t * Check if any records were added by checking if `last_key` was set.\n>> +\t */\n>> +\tif (w->last_key.len)\n>> +\t\treturn REFTABLE_API_ERROR;\n>\n> Hm. Using the last key feels somewhat dangerous to me as it does get\n> reset at times, e.g. when finishing writing the current section. It\n> _should_ work, but overall it just feels a tad to disconnected from the\n> thing that we actually want to check.\n>\n> How about we instead use `next`? This variable records the offset of the\n> next block we're about to write, and `writer_flush_nonempty_block()`\n> uses it directly to check whether we're currently writing the first\n> block in order to decide whether it needs to write a header or not. If\n> it's 0, we know that we haven't written the first block yet. That feels\n> much closer aligned with what we're checking.\n>\n\nThe last version did use `next`. I changed it because `next` is only\nmodified once the first block has been written. This would still allow\nlimit modification post writing of first few records.\n\nThis should be okay however since we're concerned about header <> footer\nmismatch. But from an ideological point, it makes sense to only allow\nlimit modification before _any_ records have been written.\n\nI'm thinking if we should use both `if (w->next || w->last_key.len)`.\nThis way we capture all modifications.\n\n>> diff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c\n>> index aeec195b2b1014445d71c5db39a9795017fd8ff2..b23edf18a7d75b0c2292490ad06d4dfaaa571e79 100644\n>> --- a/t/unit-tests/t-reftable-stack.c\n>> +++ b/t/unit-tests/t-reftable-stack.c\n>\n> Can we maybe add a unit test that demonstrates the error?\n\nGood suggestion, will add it!\n\n\n\n> Patrick\n"},{"id":"511035","messageId":"20250122-461-corrupted-reftable-followup-v3-0-ae5f88bf04fa@gmail.com","threadId":"62820","inReplyTo":"20250121-461-corrupted-reftable-followup-v2-0-37e26c7a79b4@gmail.com","subject":"[PATCH v3 0/3] refs: small followups to the migration corruption fix","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-22T05:35:46Z","receivedAt":"2025-01-22T05:36:01Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"This is a follow up to the bug that was reported [1] around `git refs\nmigrate --ref-format=reftable` where the migration would fail for\nrepositories with reflogs with lots of entries. This was caused due to a\nmismatch in the reftable's header and footer, specifically WRT the\n'max_update_index'.\n\nWhile there was a fix posted. This series is a small followup to fix\nsome of the topics discussed there:\n1. To mark `ref_transaction_update_reflog()` as static since it is only\nused internally within 'refs.c'.\n2. To change the type of 'max_index' from 'unsigned int' to 'uint64_t'.\nThis would be much safer for large repositories with millions of files\nand on 32bit systems.\n3. To add a safeguard to prevent 'update_index' changes post any record\naddition. This is a preventive measure to ensure such bugs don't arise\nin the future.\n\nThis is based on top of master 757161efcc (Sync with Git 2.48.1,\n2025-01-13) with 'kn/reflog-migration-fix' merged in.\n\n[1]: https://lore.kernel.org/r/Z4UbkcmJAU1MT-Rs@tapette.crustytoothpaste.net\n\nSigned-off-by: Karthik Nayak \n---\nChanges in v3:\n- Commit 3: Modify the commit message to make it clearer that this is a\n preventive measure. Modify the condition to check for both `last_key`\n and `next`. Also add a unit test.\n- Link to v2: https://lore.kernel.org/r/20250121-461-corrupted-reftable-followup-v2-0-37e26c7a79b4@gmail.com\n\nChanges in v2:\n- Commit 1: Keep the function comments.\n- Commit 3: Modify the requirement to be for any record writes instead\nof the first block write. This ensures that the limits need to be set \nbefore any records being added. Instead of using `BUG()`, return \n`REFTABLE_API_ERROR`. Handle all callers as needed.\n- Link to v1: https://lore.kernel.org/r/20250117-461-corrupted-reftable-followup-v1-0-70ee605ae3fe@gmail.com\n\n---\nKarthik Nayak (3):\n refs: mark `ref_transaction_update_reflog()` as static\n refs: use 'uint64_t' for 'ref_update.index'\n reftable: prevent 'update_index' changes after adding records\n\n refs.c | 24 ++++++++++++------\n refs.h | 14 -----------\n refs/refs-internal.h | 4 +--\n refs/reftable-backend.c | 22 ++++++++++++-----\n reftable/reftable-error.h | 1 +\n reftable/reftable-writer.h | 24 ++++++++++--------\n reftable/stack.c | 6 +++--\n reftable/writer.c | 15 +++++++++++-\n t/unit-tests/t-reftable-stack.c | 54 ++++++++++++++++++++++++++++++++++++++---\n 9 files changed, 118 insertions(+), 46 deletions(-)\n---\n\nRange-diff versus v2:\n\n1: 76e6da564a = 1: cafede9b10 refs: mark `ref_transaction_update_reflog()` as static\n2: f8e2e81eb4 = 2: 737360d883 refs: use 'uint64_t' for 'ref_update.index'\n3: ca92e29ecc ! 3: 01bf1d765f reftable: prevent 'update_index' changes after adding records\n @@ Commit message\n and footer values. The footer would contain the newer values while the\n header retained the original ones.\n \n - To fix this bug, prevent callers from updating these values after any\n - record is written. To do this, modify the function to return an error\n - whenever the limits are modified after any record adds. Check for record\n - adds within `reftable_writer_set_limits()` by checking the `last_key`\n - variable, which is set whenever a new record is added.\n + To protect against this bug, prevent callers from updating these values\n + after any record is written. To do this, modify the function to return\n + an error whenever the limits are modified after any record adds. Check\n + for record adds within `reftable_writer_set_limits()` by checking the\n + `last_key` and `next` variable. The former is updated after each record\n + added, but is reset at certain points. The latter is set after writing\n + the first block.\n \n Modify all callers of the function to anticipate a return type and\n - handle it accordingly.\n + handle it accordingly. Add a unit test to also ensure the function\n + returns the error as expected.\n \n Helped-by: Patrick Steinhardt \n Signed-off-by: Karthik Nayak \n @@ reftable/writer.c: int reftable_writer_new(struct reftable_writer **out,\n }\n \n -void reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,\n --\t\t\t\tuint64_t max)\n +int reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,\n -+\t\t\t uint64_t max)\n + \t\t\t\tuint64_t max)\n {\n +\t/*\n -+\t * The limits should be set before any records are added to the writer.\n -+\t * Check if any records were added by checking if `last_key` was set.\n ++\t * Set the min/max update index limits for the reftable writer.\n ++\t * This must be called before adding any records, since:\n ++\t * - The 'next' field gets set after writing the first block.\n ++\t * - The 'last_key' field updates with each new record (but resets\n ++\t * after sections).\n ++\t * Returns REFTABLE_API_ERROR if called after writing has begun.\n +\t */\n -+\tif (w->last_key.len)\n ++\tif (w->next || w->last_key.len)\n +\t\treturn REFTABLE_API_ERROR;\n +\n \tw->min_update_index = min;\n @@ t/unit-tests/t-reftable-stack.c: static void t_reflog_expire(void)\n \treturn 0;\n }\n \n +@@ t/unit-tests/t-reftable-stack.c: static void t_reftable_stack_reload_with_missing_table(void)\n + \tclear_dir(dir);\n + }\n + \n ++static int write_limits_after_ref(struct reftable_writer *wr, void *arg)\n ++{\n ++\tstruct reftable_ref_record *ref = arg;\n ++\tcheck(!reftable_writer_set_limits(wr, ref->update_index, ref->update_index));\n ++\tcheck(!reftable_writer_add_ref(wr, ref));\n ++\treturn reftable_writer_set_limits(wr, ref->update_index, ref->update_index);\n ++}\n ++\n ++static void t_reftable_invalid_limit_updates(void)\n ++{\n ++\tstruct reftable_ref_record ref = {\n ++\t\t.refname = (char *) \"HEAD\",\n ++\t\t.update_index = 1,\n ++\t\t.value_type = REFTABLE_REF_SYMREF,\n ++\t\t.value.symref = (char *) \"master\",\n ++\t};\n ++\tstruct reftable_write_options opts = {\n ++\t\t.default_permissions = 0660,\n ++\t};\n ++\tstruct reftable_addition *add = NULL;\n ++\tchar *dir = get_tmp_dir(__LINE__);\n ++\tstruct reftable_stack *st = NULL;\n ++\tint err;\n ++\n ++\terr = reftable_new_stack(&st, dir, &opts);\n ++\tcheck(!err);\n ++\n ++\treftable_addition_destroy(add);\n ++\n ++\terr = reftable_stack_new_addition(&add, st, 0);\n ++\tcheck(!err);\n ++\n ++\t/*\n ++\t * write_limits_after_ref also updates the update indexes after adding\n ++\t * the record. This should cause an err to be returned, since the limits\n ++\t * must be set at the start.\n ++\t */\n ++\terr = reftable_addition_add(add, write_limits_after_ref, &ref);\n ++\tcheck_int(err, ==, REFTABLE_API_ERROR);\n ++\n ++\treftable_addition_destroy(add);\n ++\treftable_stack_destroy(st);\n ++\tclear_dir(dir);\n ++}\n ++\n + int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n + {\n + \tTEST(t_empty_add(), \"empty addition to stack\");\n + \tTEST(t_read_file(), \"read_lines works\");\n + \tTEST(t_reflog_expire(), \"expire reflog entries\");\n ++\tTEST(t_reftable_invalid_limit_updates(), \"prevent limit updates after adding records\");\n + \tTEST(t_reftable_stack_add(), \"add multiple refs and logs to stack\");\n + \tTEST(t_reftable_stack_add_one(), \"add a single ref record to stack\");\n + \tTEST(t_reftable_stack_add_performs_auto_compaction(), \"addition to stack triggers auto-compaction\");\n\n\n---\n\nbase-commit: a5aa44e7930761cb900813d971b4105f901818fb\nchange-id: 20250117-461-corrupted-reftable-followup-eb0e4fd1a723\n\nThanks\n- Karthik\n\n"},{"id":"511036","messageId":"20250122-461-corrupted-reftable-followup-v3-1-ae5f88bf04fa@gmail.com","threadId":"62820","inReplyTo":"20250122-461-corrupted-reftable-followup-v3-0-ae5f88bf04fa@gmail.com","subject":"[PATCH v3 1/3] refs: mark `ref_transaction_update_reflog()` as static","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-22T05:35:47Z","receivedAt":"2025-01-22T05:36:03Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `ref_transaction_update_reflog()` function is only used within\n'refs.c', so mark it as static.\n\nReported-by: Junio C Hamano \nSigned-off-by: Karthik Nayak \n---\n refs.c | 22 +++++++++++++++-------\n refs.h | 14 --------------\n 2 files changed, 15 insertions(+), 21 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex f7b6f0f897eb58665e10a2efd3eb53c3f72abe61..ad6d774717150f1fe68a59c629e05e49a469693f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1318,13 +1318,21 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \treturn 0;\n }\n \n-int ref_transaction_update_reflog(struct ref_transaction *transaction,\n-\t\t\t\t const char *refname,\n-\t\t\t\t const struct object_id *new_oid,\n-\t\t\t\t const struct object_id *old_oid,\n-\t\t\t\t const char *committer_info, unsigned int flags,\n-\t\t\t\t const char *msg, unsigned int index,\n-\t\t\t\t struct strbuf *err)\n+/*\n+ * Similar to`ref_transaction_update`, but this function is only for adding\n+ * a reflog update. Supports providing custom committer information. The index\n+ * field can be utiltized to order updates as desired. When not used, the\n+ * updates default to being ordered by refname.\n+ */\n+static int ref_transaction_update_reflog(struct ref_transaction *transaction,\n+\t\t\t\t\t const char *refname,\n+\t\t\t\t\t const struct object_id *new_oid,\n+\t\t\t\t\t const struct object_id *old_oid,\n+\t\t\t\t\t const char *committer_info,\n+\t\t\t\t\t unsigned int flags,\n+\t\t\t\t\t const char *msg,\n+\t\t\t\t\t unsigned int index,\n+\t\t\t\t\t struct strbuf *err)\n {\n \tstruct ref_update *update;\n \ndiff --git a/refs.h b/refs.h\nindex a0cdd99250e8286b55808b697b0a94afac5d8319..09be47afbee51e99f4ae49588cd65596ccfcb07e 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -771,20 +771,6 @@ int ref_transaction_update(struct ref_transaction *transaction,\n \t\t\t unsigned int flags, const char *msg,\n \t\t\t struct strbuf *err);\n \n-/*\n- * Similar to`ref_transaction_update`, but this function is only for adding\n- * a reflog update. Supports providing custom committer information. The index\n- * field can be utiltized to order updates as desired. When not used, the\n- * updates default to being ordered by refname.\n- */\n-int ref_transaction_update_reflog(struct ref_transaction *transaction,\n-\t\t\t\t const char *refname,\n-\t\t\t\t const struct object_id *new_oid,\n-\t\t\t\t const struct object_id *old_oid,\n-\t\t\t\t const char *committer_info, unsigned int flags,\n-\t\t\t\t const char *msg, unsigned int index,\n-\t\t\t\t struct strbuf *err);\n-\n /*\n * Add a reference creation to transaction. new_oid is the value that\n * the reference should have after the update; it must not be\n\n-- \n2.47.0\n\n"},{"id":"511037","messageId":"20250122-461-corrupted-reftable-followup-v3-2-ae5f88bf04fa@gmail.com","threadId":"62820","inReplyTo":"20250122-461-corrupted-reftable-followup-v3-0-ae5f88bf04fa@gmail.com","subject":"[PATCH v3 2/3] refs: use 'uint64_t' for 'ref_update.index'","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-22T05:35:48Z","receivedAt":"2025-01-22T05:36:06Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The 'ref_update.index' variable is used to store an index for a given\nreference update. This index is used to order the updates in a\npredetermined order, while the default ordering is alphabetical as per\nthe refname.\n\nFor large repositories with millions of references, it should be safer\nto use 'uint64_t'. Let's do that. This also is applied for all other\ncode sections where we store 'index' and pass it around.\n\nReported-by: brian m. carlson \nSigned-off-by: Karthik Nayak \n---\n refs.c | 4 ++--\n refs/refs-internal.h | 4 ++--\n refs/reftable-backend.c | 2 +-\n 3 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex ad6d774717150f1fe68a59c629e05e49a469693f..ef04f403a6a3a34f9156b4cf68c3daa29c9cbad6 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1331,7 +1331,7 @@ static int ref_transaction_update_reflog(struct ref_transaction *transaction,\n \t\t\t\t\t const char *committer_info,\n \t\t\t\t\t unsigned int flags,\n \t\t\t\t\t const char *msg,\n-\t\t\t\t\t unsigned int index,\n+\t\t\t\t\t uint64_t index,\n \t\t\t\t\t struct strbuf *err)\n {\n \tstruct ref_update *update;\n@@ -2813,7 +2813,7 @@ static int migrate_one_ref(const char *refname, const char *referent UNUSED, con\n }\n \n struct reflog_migration_data {\n-\tunsigned int index;\n+\tuint64_t index;\n \tconst char *refname;\n \tstruct ref_store *old_refs;\n \tstruct ref_transaction *transaction;\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex aaab711bb96844755dfa600d37efdb25b30a0765..8894b43d1d1a327d404d3923c507d2d39649de19 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -120,7 +120,7 @@ struct ref_update {\n \t * when migrating reflogs and we want to ensure we carry over the\n \t * same order.\n \t */\n-\tunsigned int index;\n+\tuint64_t index;\n \n \t/*\n \t * If this ref_update was split off of a symref update via\n@@ -203,7 +203,7 @@ struct ref_transaction {\n \tenum ref_transaction_state state;\n \tvoid *backend_data;\n \tunsigned int flags;\n-\tunsigned int max_index;\n+\tuint64_t max_index;\n };\n \n /*\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 289496058e6eb4d3e3aef96ca70219cd4ff78eae..6814c87bc618229ac8a70b904be3f850371ad876 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -942,7 +942,7 @@ struct write_transaction_table_arg {\n \tsize_t updates_nr;\n \tsize_t updates_alloc;\n \tsize_t updates_expected;\n-\tunsigned int max_index;\n+\tuint64_t max_index;\n };\n \n struct reftable_transaction_data {\n\n-- \n2.47.0\n\n"},{"id":"511038","messageId":"20250122-461-corrupted-reftable-followup-v3-3-ae5f88bf04fa@gmail.com","threadId":"62820","inReplyTo":"20250122-461-corrupted-reftable-followup-v3-0-ae5f88bf04fa@gmail.com","subject":"[PATCH v3 3/3] reftable: prevent 'update_index' changes after adding records","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-01-22T05:35:49Z","receivedAt":"2025-01-22T05:36:09Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The function `reftable_writer_set_limits()` allows updating the\n'min_update_index' and 'max_update_index' of a reftable writer. These\nvalues are written to both the writer's header and footer.\n\nSince the header is written during the first block write, any subsequent\nchanges to the update index would create a mismatch between the header\nand footer values. The footer would contain the newer values while the\nheader retained the original ones.\n\nTo protect against this bug, prevent callers from updating these values\nafter any record is written. To do this, modify the function to return\nan error whenever the limits are modified after any record adds. Check\nfor record adds within `reftable_writer_set_limits()` by checking the\n`last_key` and `next` variable. The former is updated after each record\nadded, but is reset at certain points. The latter is set after writing\nthe first block.\n\nModify all callers of the function to anticipate a return type and\nhandle it accordingly. Add a unit test to also ensure the function\nreturns the error as expected.\n\nHelped-by: Patrick Steinhardt \nSigned-off-by: Karthik Nayak \n---\n refs/reftable-backend.c | 20 +++++++++++----\n reftable/reftable-error.h | 1 +\n reftable/reftable-writer.h | 24 ++++++++++--------\n reftable/stack.c | 6 +++--\n reftable/writer.c | 15 +++++++++++-\n t/unit-tests/t-reftable-stack.c | 54 ++++++++++++++++++++++++++++++++++++++---\n 6 files changed, 99 insertions(+), 21 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 6814c87bc618229ac8a70b904be3f850371ad876..9cfb0cb26721a9425c3b4a374f7b41e192037315 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1443,7 +1443,9 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n \t * multiple entries. Each entry will contain a different update_index,\n \t * so set the limits accordingly.\n \t */\n-\treftable_writer_set_limits(writer, ts, ts + arg->max_index);\n+\tret = reftable_writer_set_limits(writer, ts, ts + arg->max_index);\n+\tif (ret < 0)\n+\t\tgoto done;\n \n \tfor (i = 0; i < arg->updates_nr; i++) {\n \t\tstruct reftable_transaction_update *tx_update = &arg->updates[i];\n@@ -1766,7 +1768,9 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)\n \tdeletion_ts = creation_ts = reftable_stack_next_update_index(arg->be->stack);\n \tif (arg->delete_old)\n \t\tcreation_ts++;\n-\treftable_writer_set_limits(writer, deletion_ts, creation_ts);\n+\tret = reftable_writer_set_limits(writer, deletion_ts, creation_ts);\n+\tif (ret < 0)\n+\t\tgoto done;\n \n \t/*\n \t * Add the new reference. If this is a rename then we also delete the\n@@ -2298,7 +2302,9 @@ static int write_reflog_existence_table(struct reftable_writer *writer,\n \tif (ret <= 0)\n \t\tgoto done;\n \n-\treftable_writer_set_limits(writer, ts, ts);\n+\tret = reftable_writer_set_limits(writer, ts, ts);\n+\tif (ret < 0)\n+\t\tgoto done;\n \n \t/*\n \t * The existence entry has both old and new object ID set to the\n@@ -2357,7 +2363,9 @@ static int write_reflog_delete_table(struct reftable_writer *writer, void *cb_da\n \tuint64_t ts = reftable_stack_next_update_index(arg->stack);\n \tint ret;\n \n-\treftable_writer_set_limits(writer, ts, ts);\n+\tret = reftable_writer_set_limits(writer, ts, ts);\n+\tif (ret < 0)\n+\t\tgoto out;\n \n \tret = reftable_stack_init_log_iterator(arg->stack, &it);\n \tif (ret < 0)\n@@ -2434,7 +2442,9 @@ static int write_reflog_expiry_table(struct reftable_writer *writer, void *cb_da\n \t\tif (arg->records[i].value_type == REFTABLE_LOG_UPDATE)\n \t\t\tlive_records++;\n \n-\treftable_writer_set_limits(writer, ts, ts);\n+\tret = reftable_writer_set_limits(writer, ts, ts);\n+\tif (ret < 0)\n+\t\treturn ret;\n \n \tif (!is_null_oid(&arg->update_oid)) {\n \t\tstruct reftable_ref_record ref = {0};\ndiff --git a/reftable/reftable-error.h b/reftable/reftable-error.h\nindex f4048265629fe456207b88620658193f770a84f0..a7e33d964d0cfe5546f588d26c0fcb66ab326828 100644\n--- a/reftable/reftable-error.h\n+++ b/reftable/reftable-error.h\n@@ -30,6 +30,7 @@ enum reftable_error {\n \n \t/* Misuse of the API:\n \t * - on writing a record with NULL refname.\n+\t * - on writing a record before setting the writer limits.\n \t * - on writing a reftable_ref_record outside the table limits\n \t * - on writing a ref or log record before the stack's\n \t * next_update_inde*x\ndiff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h\nindex 5f9afa620bb00de66c311765fb0ae8c6f56401ae..1ea014d389cc47f173279e3234a82f3fcbc807a0 100644\n--- a/reftable/reftable-writer.h\n+++ b/reftable/reftable-writer.h\n@@ -124,17 +124,21 @@ int reftable_writer_new(struct reftable_writer **out,\n \t\t\tint (*flush_func)(void *),\n \t\t\tvoid *writer_arg, const struct reftable_write_options *opts);\n \n-/* Set the range of update indices for the records we will add. When writing a\n- table into a stack, the min should be at least\n- reftable_stack_next_update_index(), or REFTABLE_API_ERROR is returned.\n-\n- For transactional updates to a stack, typically min==max, and the\n- update_index can be obtained by inspeciting the stack. When converting an\n- existing ref database into a single reftable, this would be a range of\n- update-index timestamps.\n+/*\n+ * Set the range of update indices for the records we will add. When writing a\n+ * table into a stack, the min should be at least\n+ * reftable_stack_next_update_index(), or REFTABLE_API_ERROR is returned.\n+ *\n+ * For transactional updates to a stack, typically min==max, and the\n+ * update_index can be obtained by inspeciting the stack. When converting an\n+ * existing ref database into a single reftable, this would be a range of\n+ * update-index timestamps.\n+ *\n+ * The function should be called before adding any records to the writer. If not\n+ * it will fail with REFTABLE_API_ERROR.\n */\n-void reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,\n-\t\t\t\tuint64_t max);\n+int reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,\n+\t\t\t uint64_t max);\n \n /*\n Add a reftable_ref_record. The record should have names that come after\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 531660a49f0948c33041831ee0d740feacb22b2f..9649dbbb04c51e106ee752f14481bbad381cb348 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -1058,8 +1058,10 @@ static int stack_write_compact(struct reftable_stack *st,\n \n \tfor (size_t i = first; i <= last; i++)\n \t\tst->stats.bytes += st->readers[i]->size;\n-\treftable_writer_set_limits(wr, st->readers[first]->min_update_index,\n-\t\t\t\t st->readers[last]->max_update_index);\n+\terr = reftable_writer_set_limits(wr, st->readers[first]->min_update_index,\n+\t\t\t\t\t st->readers[last]->max_update_index);\n+\tif (err < 0)\n+\t\tgoto done;\n \n \terr = reftable_merged_table_new(&mt, st->readers + first, subtabs_len,\n \t\t\t\t\tst->opts.hash_id);\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 740c98038eaf883258bef4988f78977ac7e4a75a..76e24018172fc1d80a6535698757979e53b0e213 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -179,11 +179,24 @@ int reftable_writer_new(struct reftable_writer **out,\n \treturn 0;\n }\n \n-void reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,\n+int reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,\n \t\t\t\tuint64_t max)\n {\n+\t/*\n+\t * Set the min/max update index limits for the reftable writer.\n+\t * This must be called before adding any records, since:\n+\t * - The 'next' field gets set after writing the first block.\n+\t * - The 'last_key' field updates with each new record (but resets\n+\t * after sections).\n+\t * Returns REFTABLE_API_ERROR if called after writing has begun.\n+\t */\n+\tif (w->next || w->last_key.len)\n+\t\treturn REFTABLE_API_ERROR;\n+\n \tw->min_update_index = min;\n \tw->max_update_index = max;\n+\n+\treturn 0;\n }\n \n static void writer_release(struct reftable_writer *w)\ndiff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c\nindex aeec195b2b1014445d71c5db39a9795017fd8ff2..c3f0059c346edbe1ad543c9832959c6fc0aa9180 100644\n--- a/t/unit-tests/t-reftable-stack.c\n+++ b/t/unit-tests/t-reftable-stack.c\n@@ -103,7 +103,8 @@ static void t_read_file(void)\n static int write_test_ref(struct reftable_writer *wr, void *arg)\n {\n \tstruct reftable_ref_record *ref = arg;\n-\treftable_writer_set_limits(wr, ref->update_index, ref->update_index);\n+\tcheck(!reftable_writer_set_limits(wr, ref->update_index,\n+\t\t\t\t\t ref->update_index));\n \treturn reftable_writer_add_ref(wr, ref);\n }\n \n@@ -143,7 +144,8 @@ static int write_test_log(struct reftable_writer *wr, void *arg)\n {\n \tstruct write_log_arg *wla = arg;\n \n-\treftable_writer_set_limits(wr, wla->update_index, wla->update_index);\n+\tcheck(!reftable_writer_set_limits(wr, wla->update_index,\n+\t\t\t\t\t wla->update_index));\n \treturn reftable_writer_add_log(wr, wla->log);\n }\n \n@@ -961,7 +963,7 @@ static void t_reflog_expire(void)\n \n static int write_nothing(struct reftable_writer *wr, void *arg UNUSED)\n {\n-\treftable_writer_set_limits(wr, 1, 1);\n+\tcheck(!reftable_writer_set_limits(wr, 1, 1));\n \treturn 0;\n }\n \n@@ -1369,11 +1371,57 @@ static void t_reftable_stack_reload_with_missing_table(void)\n \tclear_dir(dir);\n }\n \n+static int write_limits_after_ref(struct reftable_writer *wr, void *arg)\n+{\n+\tstruct reftable_ref_record *ref = arg;\n+\tcheck(!reftable_writer_set_limits(wr, ref->update_index, ref->update_index));\n+\tcheck(!reftable_writer_add_ref(wr, ref));\n+\treturn reftable_writer_set_limits(wr, ref->update_index, ref->update_index);\n+}\n+\n+static void t_reftable_invalid_limit_updates(void)\n+{\n+\tstruct reftable_ref_record ref = {\n+\t\t.refname = (char *) \"HEAD\",\n+\t\t.update_index = 1,\n+\t\t.value_type = REFTABLE_REF_SYMREF,\n+\t\t.value.symref = (char *) \"master\",\n+\t};\n+\tstruct reftable_write_options opts = {\n+\t\t.default_permissions = 0660,\n+\t};\n+\tstruct reftable_addition *add = NULL;\n+\tchar *dir = get_tmp_dir(__LINE__);\n+\tstruct reftable_stack *st = NULL;\n+\tint err;\n+\n+\terr = reftable_new_stack(&st, dir, &opts);\n+\tcheck(!err);\n+\n+\treftable_addition_destroy(add);\n+\n+\terr = reftable_stack_new_addition(&add, st, 0);\n+\tcheck(!err);\n+\n+\t/*\n+\t * write_limits_after_ref also updates the update indexes after adding\n+\t * the record. This should cause an err to be returned, since the limits\n+\t * must be set at the start.\n+\t */\n+\terr = reftable_addition_add(add, write_limits_after_ref, &ref);\n+\tcheck_int(err, ==, REFTABLE_API_ERROR);\n+\n+\treftable_addition_destroy(add);\n+\treftable_stack_destroy(st);\n+\tclear_dir(dir);\n+}\n+\n int cmd_main(int argc UNUSED, const char *argv[] UNUSED)\n {\n \tTEST(t_empty_add(), \"empty addition to stack\");\n \tTEST(t_read_file(), \"read_lines works\");\n \tTEST(t_reflog_expire(), \"expire reflog entries\");\n+\tTEST(t_reftable_invalid_limit_updates(), \"prevent limit updates after adding records\");\n \tTEST(t_reftable_stack_add(), \"add multiple refs and logs to stack\");\n \tTEST(t_reftable_stack_add_one(), \"add a single ref record to stack\");\n \tTEST(t_reftable_stack_add_performs_auto_compaction(), \"addition to stack triggers auto-compaction\");\n\n-- \n2.47.0\n\n"},{"id":"511070","messageId":"Z5DgxQuc2j_-5GHg@pks.im","threadId":"62820","inReplyTo":"20250122-461-corrupted-reftable-followup-v3-3-ae5f88bf04fa@gmail.com","subject":"Re: [PATCH v3 3/3] reftable: prevent 'update_index' changes after adding records","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-22T12:12:53Z","receivedAt":"2025-01-22T12:12:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jan 22, 2025 at 06:35:49AM +0100, Karthik Nayak wrote:\n> diff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c\n> index aeec195b2b1014445d71c5db39a9795017fd8ff2..c3f0059c346edbe1ad543c9832959c6fc0aa9180 100644\n> --- a/t/unit-tests/t-reftable-stack.c\n> +++ b/t/unit-tests/t-reftable-stack.c\n> @@ -1369,11 +1371,57 @@ static void t_reftable_stack_reload_with_missing_table(void)\n> \tclear_dir(dir);\n> }\n> \n> +static int write_limits_after_ref(struct reftable_writer *wr, void *arg)\n> +{\n> +\tstruct reftable_ref_record *ref = arg;\n> +\tcheck(!reftable_writer_set_limits(wr, ref->update_index, ref->update_index));\n> +\tcheck(!reftable_writer_add_ref(wr, ref));\n> +\treturn reftable_writer_set_limits(wr, ref->update_index, ref->update_index);\n> +}\n\nNice.\n\n> +static void t_reftable_invalid_limit_updates(void)\n> +{\n> +\tstruct reftable_ref_record ref = {\n> +\t\t.refname = (char *) \"HEAD\",\n> +\t\t.update_index = 1,\n> +\t\t.value_type = REFTABLE_REF_SYMREF,\n> +\t\t.value.symref = (char *) \"master\",\n> +\t};\n> +\tstruct reftable_write_options opts = {\n> +\t\t.default_permissions = 0660,\n> +\t};\n\nNit: it's unnecessary to pass write options. Other than that the test\nlooks good to me, and this nit isn't worth a reroll.\n\nThanks for working on this!\n\nPatrick\n"},{"id":"511078","messageId":"xmqqzfjiwxei.fsf@gitster.g","threadId":"62820","inReplyTo":"Z5DgxQuc2j_-5GHg@pks.im","subject":"Re: [PATCH v3 3/3] reftable: prevent 'update_index' changes after adding records","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-22T17:50:29Z","receivedAt":"2025-01-22T17:50:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> +static void t_reftable_invalid_limit_updates(void)\n>> +{\n>> +\tstruct reftable_ref_record ref = {\n>> +\t\t.refname = (char *) \"HEAD\",\n>> +\t\t.update_index = 1,\n>> +\t\t.value_type = REFTABLE_REF_SYMREF,\n>> +\t\t.value.symref = (char *) \"master\",\n>> +\t};\n>> +\tstruct reftable_write_options opts = {\n>> +\t\t.default_permissions = 0660,\n>> +\t};\n>\n> Nit: it's unnecessary to pass write options. Other than that the test\n> looks good to me, and this nit isn't worth a reroll.\n\nThis write_options opts is used later in this call.\n\n\terr = reftable_new_stack(&st, dir, &opts);\n\nand \"git grep reftable_new_stack\" finds many hits, almost all in\nt/unit-tests/ hierarchy, only two among many of them passing NULL as\nthe \"use the default set of options\" signal. And majority of them\ninitialize their opts like so:\n\n\tstruct reftable_write_options opts = { 0 };\n\nSo I agree that this one should pass NULL to be more explicit that\nwe do not exercise any special features from the API, but so should\nmany other existing callers that pass such meaningless &opts, I\nwould think.\n\nThanks.\n\n\n"},{"id":"511089","messageId":"xmqqo6zyv7e9.fsf@gitster.g","threadId":"62820","inReplyTo":"xmqqzfjiwxei.fsf@gitster.g","subject":"Re: [PATCH v3 3/3] reftable: prevent 'update_index' changes after adding records","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-22T21:57:34Z","receivedAt":"2025-01-22T21:57:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> Nit: it's unnecessary to pass write options. Other than that the test\n>> looks good to me, and this nit isn't worth a reroll.\n>\n> This write_options opts is used later in this call.\n>\n> \terr = reftable_new_stack(&st, dir, &opts);\n>\n> and \"git grep reftable_new_stack\" finds many hits, almost all in\n> t/unit-tests/ hierarchy, only two among many of them passing NULL as\n> the \"use the default set of options\" signal. And majority of them\n> initialize their opts like so:\n>\n> \tstruct reftable_write_options opts = { 0 };\n>\n> So I agree that this one should pass NULL to be more explicit that\n> we do not exercise any special features from the API, but so should\n> many other existing callers that pass such meaningless &opts, I\n> would think.\n\nI may have been vague, but what I meant was that cleaning it up to\npass NULL when &opts is unnecessary can and probably should be left\noutside the scope of this miniseries, and done with a clean-up patch\nthat is separate.\n\nThanks.\n"},{"id":"511615","messageId":"20250201022409.GA4082344@coredump.intra.peff.net","threadId":"62820","inReplyTo":"20250122-461-corrupted-reftable-followup-v3-3-ae5f88bf04fa@gmail.com","subject":"undefined behavior in unit tests, was Re: [PATCH v3 3/3] reftable: prevent 'update_index' changes after adding records","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-02-01T02:24:09Z","receivedAt":"2025-02-01T02:24:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 22, 2025 at 06:35:49AM +0100, Karthik Nayak wrote:\n\n> +static void t_reftable_invalid_limit_updates(void)\n> +{\n> +\tstruct reftable_ref_record ref = {\n> +\t\t.refname = (char *) \"HEAD\",\n> +\t\t.update_index = 1,\n> +\t\t.value_type = REFTABLE_REF_SYMREF,\n> +\t\t.value.symref = (char *) \"master\",\n> +\t};\n> +\tstruct reftable_write_options opts = {\n> +\t\t.default_permissions = 0660,\n> +\t};\n> +\tstruct reftable_addition *add = NULL;\n> +\tchar *dir = get_tmp_dir(__LINE__);\n> +\tstruct reftable_stack *st = NULL;\n> +\tint err;\n> +\n> +\terr = reftable_new_stack(&st, dir, &opts);\n> +\tcheck(!err);\n> +\n> +\treftable_addition_destroy(add);\n> +\n> +\terr = reftable_stack_new_addition(&add, st, 0);\n> +\tcheck(!err);\n\nCoverity complains that this function may have undefined behavior. It's\nan issue we have in a lot of other tests that have moved to the\nunit-test framework. I've mostly been ignoring it, but this is a pretty\nstraight-forward example, so I thought I'd write a note.\n\nThe issue is that reftable_new_stack() might fail, leaving \"st\" as NULL.\nAnd then we feed it to reftable_stack_new_addition(), which dereferences\nit.\n\nIn normal production code, we'd expect something like:\n\n if (err)\n\treturn -1;\n\nto avoid running the rest of the function after the first error. But the\ntest harness check() function doesn't return. It just complains to\nstdout and keeps running! So you'll get something like[1]:\n\n $ t/unit-tests/bin/t-reftable-stack\n ok 1 - empty addition to stack\n ok 2 - read_lines works\n ok 3 - expire reflog entries\n # check \"!err\" failed at t/unit-tests/t-reftable-stack.c:1404\n Segmentation fault\n\nSo...yes, we will probably notice that the test failed from the exit\ncode. But it's not great when the harness itself barfs so had. Plus a\ncompiler may be free to reorder things in a confusing way if it can see\nthat \"st\" must never be NULL.\n\nIt feels like we probably ought to return as soon as a check() fails.\nThat does create other headaches, though. E.g., we'd potentially leak\nfrom an early return (which our LSan builds will complain about),\nmeaning that test code needs to start doing the usual \"goto out\" type of\ncleanup.\n\nSo I dunno. Maybe we just live with it. But it feels pretty ugly.\n\n-Peff\n\n[1] This would happen in practice if malloc() failed, but you can\n simulate it yourself like this, which is what I used to create the\n output above:\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 026a9f9742..fe77132102 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -861,6 +861,11 @@ int reftable_stack_new_addition(struct reftable_addition **dest,\n \tint err = 0;\n \tstruct reftable_addition empty = REFTABLE_ADDITION_INIT;\n \n+\tif (flags & (1 << 16)) {\n+\t\t*dest = NULL;\n+\t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n+\t}\n+\n \tREFTABLE_CALLOC_ARRAY(*dest, 1);\n \tif (!*dest)\n \t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\ndiff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c\nindex c3f0059c34..73ed9792a5 100644\n--- a/t/unit-tests/t-reftable-stack.c\n+++ b/t/unit-tests/t-reftable-stack.c\n@@ -1400,7 +1400,7 @@ static void t_reftable_invalid_limit_updates(void)\n \n \treftable_addition_destroy(add);\n \n-\terr = reftable_stack_new_addition(&add, st, 0);\n+\terr = reftable_stack_new_addition(&add, st, (1 << 16));\n \tcheck(!err);\n \n \t/*\n"},{"id":"511632","messageId":"425859d1-d42e-42ee-b59c-723a519f0ad8@gmail.com","threadId":"62820","inReplyTo":"20250201022409.GA4082344@coredump.intra.peff.net","subject":"Re: undefined behavior in unit tests, was Re: [PATCH v3 3/3] reftable: prevent 'update_index' changes after adding records","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-02-01T10:33:13Z","receivedAt":"2025-02-01T10:33:17Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Peff\n\nOn 01/02/2025 02:24, Jeff King wrote:\n> On Wed, Jan 22, 2025 at 06:35:49AM +0100, Karthik Nayak wrote:\n> \n> Coverity complains that this function may have undefined behavior. It's\n> an issue we have in a lot of other tests that have moved to the\n> unit-test framework. I've mostly been ignoring it, but this is a pretty\n> straight-forward example, so I thought I'd write a note.\n> \n> The issue is that reftable_new_stack() might fail, leaving \"st\" as NULL.\n> And then we feed it to reftable_stack_new_addition(), which dereferences\n> it.\n> \n> In normal production code, we'd expect something like:\n> \n> if (err)\n> \treturn -1;\n> \n> to avoid running the rest of the function after the first error. But the\n> test harness check() function doesn't return. It just complains to\n> stdout and keeps running! \n\nThat is to allow the test to add more context with test_msg() or do \nthings like check all the members of a struct before returning. It is a \nbug in the test if it does not return after finding a NULL pointer, the \ncorrect usage is\n\n\tif (!check(ptr))\n\t\treturn;\n\nAs we're in the process of switching to using clar which does exit the \ntext function if a check fails (that means there may be leaks on failure \nbut if the test is failing then I don't think we should be worrying \nabout leaks) I don't know if it is worth fixing these or not. I guess it \ndepends if there are the list of targets for Seyi's Outreachy project.\n\nBest Wishes\n\nPhillip\n\n So you'll get something like[1]:\n> \n> $ t/unit-tests/bin/t-reftable-stack\n> ok 1 - empty addition to stack\n> ok 2 - read_lines works\n> ok 3 - expire reflog entries\n> # check \"!err\" failed at t/unit-tests/t-reftable-stack.c:1404\n> Segmentation fault\n> \n> So...yes, we will probably notice that the test failed from the exit\n> code. But it's not great when the harness itself barfs so had. Plus a\n> compiler may be free to reorder things in a confusing way if it can see\n> that \"st\" must never be NULL.\n> \n> It feels like we probably ought to return as soon as a check() fails.\n> That does create other headaches, though. E.g., we'd potentially leak\n> from an early return (which our LSan builds will complain about),\n> meaning that test code needs to start doing the usual \"goto out\" type of\n> cleanup.\n> \n> So I dunno. Maybe we just live with it. But it feels pretty ugly.\n> \n> -Peff\n> \n> [1] This would happen in practice if malloc() failed, but you can\n> simulate it yourself like this, which is what I used to create the\n> output above:\n> \n> diff --git a/reftable/stack.c b/reftable/stack.c\n> index 026a9f9742..fe77132102 100644\n> --- a/reftable/stack.c\n> +++ b/reftable/stack.c\n> @@ -861,6 +861,11 @@ int reftable_stack_new_addition(struct reftable_addition **dest,\n> \tint err = 0;\n> \tstruct reftable_addition empty = REFTABLE_ADDITION_INIT;\n> \n> +\tif (flags & (1 << 16)) {\n> +\t\t*dest = NULL;\n> +\t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n> +\t}\n> +\n> \tREFTABLE_CALLOC_ARRAY(*dest, 1);\n> \tif (!*dest)\n> \t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n> diff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c\n> index c3f0059c34..73ed9792a5 100644\n> --- a/t/unit-tests/t-reftable-stack.c\n> +++ b/t/unit-tests/t-reftable-stack.c\n> @@ -1400,7 +1400,7 @@ static void t_reftable_invalid_limit_updates(void)\n> \n> \treftable_addition_destroy(add);\n> \n> -\terr = reftable_stack_new_addition(&add, st, 0);\n> +\terr = reftable_stack_new_addition(&add, st, (1 << 16));\n> \tcheck(!err);\n> \n> \t/*\n> \n\n"},{"id":"511674","messageId":"Z6BWw0PIANPAcVA6@pks.im","threadId":"62820","inReplyTo":"20250201022409.GA4082344@coredump.intra.peff.net","subject":"Re: undefined behavior in unit tests, was Re: [PATCH v3 3/3] reftable: prevent 'update_index' changes after adding records","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-03T05:40:19Z","receivedAt":"2025-02-03T05:40:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jan 31, 2025 at 09:24:09PM -0500, Jeff King wrote:\n> On Wed, Jan 22, 2025 at 06:35:49AM +0100, Karthik Nayak wrote:\n> \n> > +static void t_reftable_invalid_limit_updates(void)\n> > +{\n> > +\tstruct reftable_ref_record ref = {\n> > +\t\t.refname = (char *) \"HEAD\",\n> > +\t\t.update_index = 1,\n> > +\t\t.value_type = REFTABLE_REF_SYMREF,\n> > +\t\t.value.symref = (char *) \"master\",\n> > +\t};\n> > +\tstruct reftable_write_options opts = {\n> > +\t\t.default_permissions = 0660,\n> > +\t};\n> > +\tstruct reftable_addition *add = NULL;\n> > +\tchar *dir = get_tmp_dir(__LINE__);\n> > +\tstruct reftable_stack *st = NULL;\n> > +\tint err;\n> > +\n> > +\terr = reftable_new_stack(&st, dir, &opts);\n> > +\tcheck(!err);\n> > +\n> > +\treftable_addition_destroy(add);\n> > +\n> > +\terr = reftable_stack_new_addition(&add, st, 0);\n> > +\tcheck(!err);\n> \n> Coverity complains that this function may have undefined behavior. It's\n> an issue we have in a lot of other tests that have moved to the\n> unit-test framework. I've mostly been ignoring it, but this is a pretty\n> straight-forward example, so I thought I'd write a note.\n> \n> The issue is that reftable_new_stack() might fail, leaving \"st\" as NULL.\n> And then we feed it to reftable_stack_new_addition(), which dereferences\n> it.\n> \n> In normal production code, we'd expect something like:\n> \n> if (err)\n> \treturn -1;\n> \n> to avoid running the rest of the function after the first error. But the\n> test harness check() function doesn't return. It just complains to\n> stdout and keeps running! So you'll get something like[1]:\n> \n> $ t/unit-tests/bin/t-reftable-stack\n> ok 1 - empty addition to stack\n> ok 2 - read_lines works\n> ok 3 - expire reflog entries\n> # check \"!err\" failed at t/unit-tests/t-reftable-stack.c:1404\n> Segmentation fault\n> \n> So...yes, we will probably notice that the test failed from the exit\n> code. But it's not great when the harness itself barfs so had. Plus a\n> compiler may be free to reorder things in a confusing way if it can see\n> that \"st\" must never be NULL.\n> \n> It feels like we probably ought to return as soon as a check() fails.\n> That does create other headaches, though. E.g., we'd potentially leak\n> from an early return (which our LSan builds will complain about),\n> meaning that test code needs to start doing the usual \"goto out\" type of\n> cleanup.\n> \n> So I dunno. Maybe we just live with it. But it feels pretty ugly.\n\nIt's one of the pitfalls of our own testing framework, from my point of\nview. It's somewhat unexpected that the test would just continue running\nas this is the wrong thing to do in almost all cases. When assumptions\nfail, nothing good will come of it.\n\nIn any case, issues like this will be fixed once we migrate those tests\nto the clar unit test framework. The default there is to abort the tests\nin case an assertion fails, which is much saner from my point of view.\nSo I'm not sure if it is worth fixing this now, or whether refactoring\nthe tests to use clar is the more sensible fix.\n\nPatrick\n"},{"id":"511675","messageId":"Z6BW7_aj4C7B-g9P@pks.im","threadId":"62820","inReplyTo":"425859d1-d42e-42ee-b59c-723a519f0ad8@gmail.com","subject":"Re: undefined behavior in unit tests, was Re: [PATCH v3 3/3] reftable: prevent 'update_index' changes after adding records","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-03T05:41:03Z","receivedAt":"2025-02-03T05:41:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Feb 01, 2025 at 10:33:13AM +0000, Phillip Wood wrote:\n> Hi Peff\n> \n> On 01/02/2025 02:24, Jeff King wrote:\n> > On Wed, Jan 22, 2025 at 06:35:49AM +0100, Karthik Nayak wrote:\n> > \n> > Coverity complains that this function may have undefined behavior. It's\n> > an issue we have in a lot of other tests that have moved to the\n> > unit-test framework. I've mostly been ignoring it, but this is a pretty\n> > straight-forward example, so I thought I'd write a note.\n> > \n> > The issue is that reftable_new_stack() might fail, leaving \"st\" as NULL.\n> > And then we feed it to reftable_stack_new_addition(), which dereferences\n> > it.\n> > \n> > In normal production code, we'd expect something like:\n> > \n> > if (err)\n> > \treturn -1;\n> > \n> > to avoid running the rest of the function after the first error. But the\n> > test harness check() function doesn't return. It just complains to\n> > stdout and keeps running!\n> \n> That is to allow the test to add more context with test_msg() or do things\n> like check all the members of a struct before returning. It is a bug in the\n> test if it does not return after finding a NULL pointer, the correct usage\n> is\n> \n> \tif (!check(ptr))\n> \t\treturn;\n> \n> As we're in the process of switching to using clar which does exit the text\n> function if a check fails (that means there may be leaks on failure but if\n> the test is failing then I don't think we should be worrying about leaks) I\n> don't know if it is worth fixing these or not. I guess it depends if there\n> are the list of targets for Seyi's Outreachy project.\n\nAh, yes, should've read your mail first, as you're saying basically the\nsame as I did :)\n\nPatrick\n"},{"id":"511735","messageId":"xmqqcyfzyv6y.fsf@gitster.g","threadId":"62820","inReplyTo":"Z6BW7_aj4C7B-g9P@pks.im","subject":"Re: undefined behavior in unit tests, was Re: [PATCH v3 3/3] reftable: prevent 'update_index' changes after adding records","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-02-03T14:11:33Z","receivedAt":"2025-02-03T14:11:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> \tif (!check(ptr))\n>> \t\treturn;\n>> \n>> As we're in the process of switching to using clar which does exit the text\n>> function if a check fails (that means there may be leaks on failure but if\n>> the test is failing then I don't think we should be worrying about leaks) I\n>> don't know if it is worth fixing these or not. I guess it depends if there\n>> are the list of targets for Seyi's Outreachy project.\n>\n> Ah, yes, should've read your mail first, as you're saying basically the\n> same as I did :)\n\nYup, I 100% agree with both of you. Thanks.\n"},{"id":"511741","messageId":"CAOLa=ZRCb4KHKNkW=kuGhiR7B-aV73KV32DqjX4RM0fDP1gMww@mail.gmail.com","threadId":"62820","inReplyTo":"20250201022409.GA4082344@coredump.intra.peff.net","subject":"Re: undefined behavior in unit tests, was Re: [PATCH v3 3/3] reftable: prevent 'update_index' changes after adding records","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-02-03T15:20:16Z","receivedAt":"2025-02-03T15:20:17Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Jeff King writes:\n\n> On Wed, Jan 22, 2025 at 06:35:49AM +0100, Karthik Nayak wrote:\n>\n>> +static void t_reftable_invalid_limit_updates(void)\n>> +{\n>> +\tstruct reftable_ref_record ref = {\n>> +\t\t.refname = (char *) \"HEAD\",\n>> +\t\t.update_index = 1,\n>> +\t\t.value_type = REFTABLE_REF_SYMREF,\n>> +\t\t.value.symref = (char *) \"master\",\n>> +\t};\n>> +\tstruct reftable_write_options opts = {\n>> +\t\t.default_permissions = 0660,\n>> +\t};\n>> +\tstruct reftable_addition *add = NULL;\n>> +\tchar *dir = get_tmp_dir(__LINE__);\n>> +\tstruct reftable_stack *st = NULL;\n>> +\tint err;\n>> +\n>> +\terr = reftable_new_stack(&st, dir, &opts);\n>> +\tcheck(!err);\n>> +\n>> +\treftable_addition_destroy(add);\n>> +\n>> +\terr = reftable_stack_new_addition(&add, st, 0);\n>> +\tcheck(!err);\n>\n> Coverity complains that this function may have undefined behavior. It's\n> an issue we have in a lot of other tests that have moved to the\n> unit-test framework. I've mostly been ignoring it, but this is a pretty\n> straight-forward example, so I thought I'd write a note.\n>\n> The issue is that reftable_new_stack() might fail, leaving \"st\" as NULL.\n> And then we feed it to reftable_stack_new_addition(), which dereferences\n> it.\n>\n> In normal production code, we'd expect something like:\n>\n> if (err)\n> \treturn -1;\n>\n> to avoid running the rest of the function after the first error. But the\n> test harness check() function doesn't return. It just complains to\n> stdout and keeps running! So you'll get something like[1]:\n>\n> $ t/unit-tests/bin/t-reftable-stack\n> ok 1 - empty addition to stack\n> ok 2 - read_lines works\n> ok 3 - expire reflog entries\n> # check \"!err\" failed at t/unit-tests/t-reftable-stack.c:1404\n> Segmentation fault\n>\n> So...yes, we will probably notice that the test failed from the exit\n> code. But it's not great when the harness itself barfs so had. Plus a\n> compiler may be free to reorder things in a confusing way if it can see\n> that \"st\" must never be NULL.\n>\n> It feels like we probably ought to return as soon as a check() fails.\n> That does create other headaches, though. E.g., we'd potentially leak\n> from an early return (which our LSan builds will complain about),\n> meaning that test code needs to start doing the usual \"goto out\" type of\n> cleanup.\n>\n> So I dunno. Maybe we just live with it. But it feels pretty ugly.\n>\n\nThanks for pointing it out, I didn't notice this, mostly as I was\ncopying from existing test cases and it does seem like this (wrong)\npattern exists in a lot of the tests.\n\nLike Phillip and Patrick mentioned, this should go away since we're\nmoving to using the clar test framework. I think it makes sense to keep\nthis as is to stay consistent with the rest of code in this file for\nnow. It is ugly, but seems like that would be simpler while migrating.\n\n> -Peff\n>\n> [1] This would happen in practice if malloc() failed, but you can\n> simulate it yourself like this, which is what I used to create the\n> output above:\n>\n> diff --git a/reftable/stack.c b/reftable/stack.c\n> index 026a9f9742..fe77132102 100644\n> --- a/reftable/stack.c\n> +++ b/reftable/stack.c\n> @@ -861,6 +861,11 @@ int reftable_stack_new_addition(struct reftable_addition **dest,\n> \tint err = 0;\n> \tstruct reftable_addition empty = REFTABLE_ADDITION_INIT;\n>\n> +\tif (flags & (1 << 16)) {\n> +\t\t*dest = NULL;\n> +\t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n> +\t}\n> +\n> \tREFTABLE_CALLOC_ARRAY(*dest, 1);\n> \tif (!*dest)\n> \t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n> diff --git a/t/unit-tests/t-reftable-stack.c b/t/unit-tests/t-reftable-stack.c\n> index c3f0059c34..73ed9792a5 100644\n> --- a/t/unit-tests/t-reftable-stack.c\n> +++ b/t/unit-tests/t-reftable-stack.c\n> @@ -1400,7 +1400,7 @@ static void t_reftable_invalid_limit_updates(void)\n>\n> \treftable_addition_destroy(add);\n>\n> -\terr = reftable_stack_new_addition(&add, st, 0);\n> +\terr = reftable_stack_new_addition(&add, st, (1 << 16));\n> \tcheck(!err);\n>\n> \t/*\n"},{"id":"511743","messageId":"20250203153724.GB4165842@coredump.intra.peff.net","threadId":"62820","inReplyTo":"425859d1-d42e-42ee-b59c-723a519f0ad8@gmail.com","subject":"Re: undefined behavior in unit tests, was Re: [PATCH v3 3/3] reftable: prevent 'update_index' changes after adding records","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-02-03T15:37:24Z","receivedAt":"2025-02-03T15:37:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 01, 2025 at 10:33:13AM +0000, Phillip Wood wrote:\n\n> > In normal production code, we'd expect something like:\n> > \n> > if (err)\n> > \treturn -1;\n> > \n> > to avoid running the rest of the function after the first error. But the\n> > test harness check() function doesn't return. It just complains to\n> > stdout and keeps running!\n> \n> That is to allow the test to add more context with test_msg() or do things\n> like check all the members of a struct before returning. It is a bug in the\n> test if it does not return after finding a NULL pointer, the correct usage\n> is\n> \n> \tif (!check(ptr))\n> \t\treturn;\n> \n> As we're in the process of switching to using clar which does exit the text\n> function if a check fails (that means there may be leaks on failure but if\n> the test is failing then I don't think we should be worrying about leaks) I\n> don't know if it is worth fixing these or not. I guess it depends if there\n> are the list of targets for Seyi's Outreachy project.\n\nAh, that's good to hear. I don't think there's any urgency here. These\nhave been popping up since people started adding more unit-tests/ last\nsummer. Waiting a few more months to switch to clar is probably not a\nbig deal.\n\nI'm OK with ignoring a leak in a failing test. I do suspect that\nCoverity might still complain about the leaks, because it is doing\nstatic analysis to show that we _can_ leak (rather than the tests, which\nare seeing if we leaked at runtime). But I'm not sure how much effort we\nwant to spend on making tests do cleanup on failure. Especially in a\nlanguage like C.\n\nAnyway, I'll continue to ignore these Coverity results for now, then. ;)\n\n-Peff\n"},{"id":"511744","messageId":"20250203153821.GC4165842@coredump.intra.peff.net","threadId":"62820","inReplyTo":"CAOLa=ZRCb4KHKNkW=kuGhiR7B-aV73KV32DqjX4RM0fDP1gMww@mail.gmail.com","subject":"Re: undefined behavior in unit tests, was Re: [PATCH v3 3/3] reftable: prevent 'update_index' changes after adding records","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-02-03T15:38:21Z","receivedAt":"2025-02-03T15:38:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 03, 2025 at 07:20:16AM -0800, Karthik Nayak wrote:\n\n> Like Phillip and Patrick mentioned, this should go away since we're\n> moving to using the clar test framework. I think it makes sense to keep\n> this as is to stay consistent with the rest of code in this file for\n> now. It is ugly, but seems like that would be simpler while migrating.\n\nYeah, definitely not worth addressing this single case. I was more\ninterested in the overall trend, but it sounds like there are plans\nthere already.\n\n-Peff\n"}]}