{"thread":{"id":"64233","subject":"[PATCH 0/1] files-backend: check symref name before update","startedAt":"2025-10-01T15:08:17Z","lastAt":"2025-10-05T08:19:23Z","messageCount":11,"participants":["Han Young","Junio C Hamano","Karthik Nayak","Patrick Steinhardt","shejialuo"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"527681","messageId":"20251001150805.9652-1-hanyang.tony@bytedance.com","threadId":"64233","inReplyTo":null,"subject":"[PATCH 0/1] files-backend: check symref name before update","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2025-10-01T15:08:04Z","receivedAt":"2025-10-01T15:08:17Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"From: Han Young \n\nIn the ref files backend, the symbolic reference name is not checked\nbefore an update. This could cause reference and lock files to be created\noutside the refs/ directory.\n\nBelow are the original bug report by Sigma:\n\n $ echo ref: refs/../HEAD > .git/HEAD\n $ git commit -m \"test\" --allow-empty\n fatal: cannot lock ref 'HEAD': Unable to create '/home/sigma/headtest/.git/refs/../HEAD.lock': File exists.\n\n Another git process seems to be running in this repository, e.g.\n an editor opened by 'git commit'. Please make sure all processes\n are terminated then try again. If it still fails, a git process\n may have crashed in this repository earlier:\n remove the file manually to continue.\n\nIn this case, while trying to update the symbolic reference refs/../HEAD,\nthe lock file conflicts with the ./git/HEAD.lock.\n\nIf the HEAD points to refs/../foo, a reference file named foo will be\ncreated under ./git directory.\n\nHan Young (1):\n files-backend: check symref name before update\n\n refs/files-backend.c | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\n-- \n2.51.0.373.gaf4ee0e35.dirty\n\n"},{"id":"527682","messageId":"20251001150805.9652-2-hanyang.tony@bytedance.com","threadId":"64233","inReplyTo":"20251001150805.9652-1-hanyang.tony@bytedance.com","subject":"[PATCH 1/1] files-backend: check symref name before update","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2025-10-01T15:08:05Z","receivedAt":"2025-10-01T15:08:23Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"From: Han Young \n\nIn the ref files backend, the symbolic reference name is not checked\nbefore an update. This could cause reference and lock files to be created\noutside the refs/ directory. Validate the reference before adding it to\nthe ref update transaction.\n\nReported-by: Sigma \nSigned-off-by: Han Young \n---\n refs/files-backend.c | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex bc3347d18..d47a8c392 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2516,6 +2516,16 @@ static enum ref_transaction_error split_symref_update(struct ref_update *update,\n \tstruct ref_update *new_update;\n \tunsigned int new_flags;\n \n+\t/*\n+\t * Check the referent is valid before adding it to the transaction.\n+\t */\n+\tif (!refname_is_safe(referent)) {\n+\t\tstrbuf_addf(err,\n+\t\t\t \"reference '%s' appears to be broken\",\n+\t\t\t update->refname);\n+\t\treturn -1;\n+\t}\n+\n \t/*\n \t * First make sure that referent is not already in the\n \t * transaction. This check is O(lg N) in the transaction\n-- \n2.51.0.373.gaf4ee0e35.dirty\n\n"},{"id":"527717","messageId":"xmqqv7ky1l70.fsf@gitster.g","threadId":"64233","inReplyTo":"20251001150805.9652-2-hanyang.tony@bytedance.com","subject":"Re: [PATCH 1/1] files-backend: check symref name before update","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-01T19:22:27Z","receivedAt":"2025-10-01T19:22:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han Young writes:\n\n> From: Han Young \n>\n> In the ref files backend, the symbolic reference name is not checked\n> before an update. This could cause reference and lock files to be created\n> outside the refs/ directory. Validate the reference before adding it to\n> the ref update transaction.\n>\n> Reported-by: Sigma \n> Signed-off-by: Han Young \n> ---\n> refs/files-backend.c | 10 ++++++++++\n> 1 file changed, 10 insertions(+)\n>\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index bc3347d18..d47a8c392 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2516,6 +2516,16 @@ static enum ref_transaction_error split_symref_update(struct ref_update *update,\n> \tstruct ref_update *new_update;\n> \tunsigned int new_flags;\n> \n> +\t/*\n> +\t * Check the referent is valid before adding it to the transaction.\n> +\t */\n> +\tif (!refname_is_safe(referent)) {\n\nShouldn't this new condition share the logic with what is done by\nfsck? IOW, after doing this\n\n $ echo ref: refs/../HEAD > .git/HEAD\n\n\"git fsck\" or \"git refs verify\" should barf (if not, we should make\nthem barf), and this code should use the same logic to notice that\nthe target of the symbolic ref is bogus.\n\n> +\t\tstrbuf_addf(err,\n> +\t\t\t \"reference '%s' appears to be broken\",\n> +\t\t\t update->refname);\n> +\t\treturn -1;\n> +\t}\n> +\n> \t/*\n> \t * First make sure that referent is not already in the\n> \t * transaction. This check is O(lg N) in the transaction\n\nCan we also have some tests?\n\nThanks.\n"},{"id":"527776","messageId":"CAOLa=ZSboPeTNSSh1fsaKc+Ef5DhaKGX+mNiRzyYfvFERa=JLQ@mail.gmail.com","threadId":"64233","inReplyTo":"20251001150805.9652-1-hanyang.tony@bytedance.com","subject":"Re: [PATCH 0/1] files-backend: check symref name before update","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-10-02T09:34:53Z","receivedAt":"2025-10-02T09:34:55Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Han Young writes:\n\n> From: Han Young \n>\n> In the ref files backend, the symbolic reference name is not checked\n> before an update. This could cause reference and lock files to be created\n> outside the refs/ directory.\n>\n> Below are the original bug report by Sigma:\n>\n> $ echo ref: refs/../HEAD > .git/HEAD\n> $ git commit -m \"test\" --allow-empty\n> fatal: cannot lock ref 'HEAD': Unable to create '/home/sigma/headtest/.git/refs/../HEAD.lock': File exists.\n>\n> Another git process seems to be running in this repository, e.g.\n> an editor opened by 'git commit'. Please make sure all processes\n> are terminated then try again. If it still fails, a git process\n> may have crashed in this repository earlier:\n> remove the file manually to continue.\n>\n> In this case, while trying to update the symbolic reference refs/../HEAD,\n> the lock file conflicts with the ./git/HEAD.lock.\n>\n> If the HEAD points to refs/../foo, a reference file named foo will be\n> created under ./git directory.\n>\n\nI quickly checked if this can also be done by using 'git-update-ref(1)'.\nBut the command calls on 'check_refname_format()' to check the new ref\nfor the symref update and fails:\n\n $ git update-ref --stdin\n symref-update HEAD refs/../HEAD\n fatal: invalid ref format: refs/../HEAD\n\nSo this is only possible by manually editing the .git/HEAD file, right?\n\nIn that case, isn't the repository already broken?\n\nIn other words, the fix seem to only stop us from creating files outside\nthe $GIT_DIR, but this seems like something that the user would have to\norchestrate intentionally.\n\nThe bigger question for me is if there is an instance that you'd want to\nmodify the HEAD file manually. Or is there a way this can be done via\nany of the existing Git commands. Otherwise, I'm not sure I would call\nthis a bug.\n\n> Han Young (1):\n> files-backend: check symref name before update\n>\n> refs/files-backend.c | 10 ++++++++++\n> 1 file changed, 10 insertions(+)\n>\n> --\n> 2.51.0.373.gaf4ee0e35.dirty\n"},{"id":"527777","messageId":"CAOLa=ZTnHQbg9ocdA1omqER6CJH-w30G14-F2JAQMtueXENWew@mail.gmail.com","threadId":"64233","inReplyTo":"xmqqv7ky1l70.fsf@gitster.g","subject":"Re: [PATCH 1/1] files-backend: check symref name before update","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-10-02T09:54:54Z","receivedAt":"2025-10-02T09:54:56Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano writes:\n\n> Han Young writes:\n>\n>> From: Han Young \n>>\n>> In the ref files backend, the symbolic reference name is not checked\n>> before an update. This could cause reference and lock files to be created\n>> outside the refs/ directory. Validate the reference before adding it to\n>> the ref update transaction.\n>>\n>> Reported-by: Sigma \n>> Signed-off-by: Han Young \n>> ---\n>> refs/files-backend.c | 10 ++++++++++\n>> 1 file changed, 10 insertions(+)\n>>\n>> diff --git a/refs/files-backend.c b/refs/files-backend.c\n>> index bc3347d18..d47a8c392 100644\n>> --- a/refs/files-backend.c\n>> +++ b/refs/files-backend.c\n>> @@ -2516,6 +2516,16 @@ static enum ref_transaction_error split_symref_update(struct ref_update *update,\n>> \tstruct ref_update *new_update;\n>> \tunsigned int new_flags;\n>>\n>> +\t/*\n>> +\t * Check the referent is valid before adding it to the transaction.\n>> +\t */\n>> +\tif (!refname_is_safe(referent)) {\n>\n> Shouldn't this new condition share the logic with what is done by\n> fsck? IOW, after doing this\n>\n> $ echo ref: refs/../HEAD > .git/HEAD\n>\n> \"git fsck\" or \"git refs verify\" should barf (if not, we should make\n> them barf), and this code should use the same logic to notice that\n> the target of the symbolic ref is bogus.\n>\n\nGood point. I see that 'git fsck' does complain about this:\n\n $ git fsck\n Checking ref database: 100% (1/1), done.\n Checking object directories: 100% (256/256), done.\n error: invalid HEAD\n dangling commit ccd1771e44a18887197d3ee26ca37c2e892b9fb6\n dangling commit f99d68ea2c378218e2360dee4e24115c404f6a66\n\nHowever 'git refs verify' doesn't...\n\n $ git refs verify --verbose\n Checking references consistency\n Checking refs/heads/master\n Checking packed-refs file .git/packed-refs\n\nOkay, so this seems like because fsck also parses all references to mark\nreachability and also parses 'HEAD' via `refs_resolve_ref_unsafe()`\nwhich fails.\n\nThis symref checks and checking root refs is definitely something we\nshould consider adding to 'git refs verify'.\n\n[snip]\n"},{"id":"527794","messageId":"aN5mOTbGBcr355E6@pks.im","threadId":"64233","inReplyTo":"CAOLa=ZTnHQbg9ocdA1omqER6CJH-w30G14-F2JAQMtueXENWew@mail.gmail.com","subject":"Re: [PATCH 1/1] files-backend: check symref name before update","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-02T11:47:05Z","receivedAt":"2025-10-02T11:47:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Oct 02, 2025 at 02:54:54AM -0700, Karthik Nayak wrote:\n> Junio C Hamano writes:\n> \n> > Han Young writes:\n> >\n> >> From: Han Young \n> >>\n> >> In the ref files backend, the symbolic reference name is not checked\n> >> before an update. This could cause reference and lock files to be created\n> >> outside the refs/ directory. Validate the reference before adding it to\n> >> the ref update transaction.\n> >>\n> >> Reported-by: Sigma \n> >> Signed-off-by: Han Young \n> >> ---\n> >> refs/files-backend.c | 10 ++++++++++\n> >> 1 file changed, 10 insertions(+)\n> >>\n> >> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> >> index bc3347d18..d47a8c392 100644\n> >> --- a/refs/files-backend.c\n> >> +++ b/refs/files-backend.c\n> >> @@ -2516,6 +2516,16 @@ static enum ref_transaction_error split_symref_update(struct ref_update *update,\n> >> \tstruct ref_update *new_update;\n> >> \tunsigned int new_flags;\n> >>\n> >> +\t/*\n> >> +\t * Check the referent is valid before adding it to the transaction.\n> >> +\t */\n> >> +\tif (!refname_is_safe(referent)) {\n> >\n> > Shouldn't this new condition share the logic with what is done by\n> > fsck? IOW, after doing this\n> >\n> > $ echo ref: refs/../HEAD > .git/HEAD\n> >\n> > \"git fsck\" or \"git refs verify\" should barf (if not, we should make\n> > them barf), and this code should use the same logic to notice that\n> > the target of the symbolic ref is bogus.\n> >\n> \n> Good point. I see that 'git fsck' does complain about this:\n> \n> $ git fsck\n> Checking ref database: 100% (1/1), done.\n> Checking object directories: 100% (256/256), done.\n> error: invalid HEAD\n> dangling commit ccd1771e44a18887197d3ee26ca37c2e892b9fb6\n> dangling commit f99d68ea2c378218e2360dee4e24115c404f6a66\n> \n> However 'git refs verify' doesn't...\n> \n> $ git refs verify --verbose\n> Checking references consistency\n> Checking refs/heads/master\n> Checking packed-refs file .git/packed-refs\n> \n> Okay, so this seems like because fsck also parses all references to mark\n> reachability and also parses 'HEAD' via `refs_resolve_ref_unsafe()`\n> which fails.\n> \n> This symref checks and checking root refs is definitely something we\n> should consider adding to 'git refs verify'.\n\nAgreed! Overall, the goal is that all logic to verify references should\nbe contained in `git refs verify`, so that git-fsck(1) only needs to\nshell out to that command to perform the full check.\n\nSo if this logic isn't yet part of `git refs verify`, we should migrate\nit over.\n\nPatrick\n"},{"id":"527801","messageId":"xmqqo6qpxw6w.fsf@gitster.g","threadId":"64233","inReplyTo":"aN5mOTbGBcr355E6@pks.im","subject":"Re: [PATCH 1/1] files-backend: check symref name before update","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-02T13:36:07Z","receivedAt":"2025-10-02T13:36:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Thu, Oct 02, 2025 at 02:54:54AM -0700, Karthik Nayak wrote:\n>> Junio C Hamano writes:\n>> \n>> > Han Young writes:\n>> >\n>> >> From: Han Young \n>> >>\n>> >> In the ref files backend, the symbolic reference name is not checked\n>> >> before an update. This could cause reference and lock files to be created\n>> >> outside the refs/ directory. Validate the reference before adding it to\n>> >> the ref update transaction.\n>> >>\n>> >> Reported-by: Sigma \n>> >> Signed-off-by: Han Young \n>> >> ---\n>> >> refs/files-backend.c | 10 ++++++++++\n>> >> 1 file changed, 10 insertions(+)\n>> >>\n>> >> diff --git a/refs/files-backend.c b/refs/files-backend.c\n>> >> index bc3347d18..d47a8c392 100644\n>> >> --- a/refs/files-backend.c\n>> >> +++ b/refs/files-backend.c\n>> >> @@ -2516,6 +2516,16 @@ static enum ref_transaction_error split_symref_update(struct ref_update *update,\n>> >> \tstruct ref_update *new_update;\n>> >> \tunsigned int new_flags;\n>> >>\n>> >> +\t/*\n>> >> +\t * Check the referent is valid before adding it to the transaction.\n>> >> +\t */\n>> >> +\tif (!refname_is_safe(referent)) {\n>> >\n>> > Shouldn't this new condition share the logic with what is done by\n>> > fsck? IOW, after doing this\n>> >\n>> > $ echo ref: refs/../HEAD > .git/HEAD\n>> >\n>> > \"git fsck\" or \"git refs verify\" should barf (if not, we should make\n>> > them barf), and this code should use the same logic to notice that\n>> > the target of the symbolic ref is bogus.\n>> >\n>> \n>> Good point. I see that 'git fsck' does complain about this:\n>> \n>> $ git fsck\n>> Checking ref database: 100% (1/1), done.\n>> Checking object directories: 100% (256/256), done.\n>> error: invalid HEAD\n>> dangling commit ccd1771e44a18887197d3ee26ca37c2e892b9fb6\n>> dangling commit f99d68ea2c378218e2360dee4e24115c404f6a66\n>> \n>> However 'git refs verify' doesn't...\n>> \n>> $ git refs verify --verbose\n>> Checking references consistency\n>> Checking refs/heads/master\n>> Checking packed-refs file .git/packed-refs\n>> \n>> Okay, so this seems like because fsck also parses all references to mark\n>> reachability and also parses 'HEAD' via `refs_resolve_ref_unsafe()`\n>> which fails.\n>> \n>> This symref checks and checking root refs is definitely something we\n>> should consider adding to 'git refs verify'.\n>\n> Agreed! Overall, the goal is that all logic to verify references should\n> be contained in `git refs verify`, so that git-fsck(1) only needs to\n> shell out to that command to perform the full check.\n>\n> So if this logic isn't yet part of `git refs verify`, we should migrate\n> it over.\n\nAbsolutely. As \"git refs verify\" is a way to do the sanity check of\nthe ref part (presumably without incurring cost to sanity check\nother aspect, like fsck does? why is it a separate command in the\nfirst place?), it should learn how to do so. \"git fsck\" should keep\ncomplaining about the failure as before, whether it is done natively\nor by delegating to \"git refs verify\".\n\nThanks.\n"},{"id":"527802","messageId":"xmqqjz1dxszl.fsf@gitster.g","threadId":"64233","inReplyTo":"CAOLa=ZSboPeTNSSh1fsaKc+Ef5DhaKGX+mNiRzyYfvFERa=JLQ@mail.gmail.com","subject":"Re: [PATCH 0/1] files-backend: check symref name before update","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-02T14:45:18Z","receivedAt":"2025-10-02T14:45:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> The bigger question for me is if there is an instance that you'd want to\n> modify the HEAD file manually. Or is there a way this can be done via\n> any of the existing Git commands. Otherwise, I'm not sure I would call\n> this a bug.\n\nAs we discussed downthread, I tend to agree with you here that this\nis a corrupt repository leading the tool to do a nonsensical thing,\nand the true bug here is that \"fsck\" is not catching it. But I do\nnot mind if we add a check at runtime, probably at the location the\npatch under discussion identified, that makes sure a symbolic ref\npoints at a valid target the same way \"git fsck\" does.\n\nThanks.\n"},{"id":"527803","messageId":"aN6amIG2Sp3W500K@pks.im","threadId":"64233","inReplyTo":"xmqqo6qpxw6w.fsf@gitster.g","subject":"Re: [PATCH 1/1] files-backend: check symref name before update","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-02T15:30:32Z","receivedAt":"2025-10-02T15:30:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Oct 02, 2025 at 06:36:07AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> > Agreed! Overall, the goal is that all logic to verify references should\n> > be contained in `git refs verify`, so that git-fsck(1) only needs to\n> > shell out to that command to perform the full check.\n> >\n> > So if this logic isn't yet part of `git refs verify`, we should migrate\n> > it over.\n> \n> Absolutely. As \"git refs verify\" is a way to do the sanity check of\n> the ref part (presumably without incurring cost to sanity check\n> other aspect, like fsck does? why is it a separate command in the\n> first place?), it should learn how to do so.\n\nWe have the same pattern in other command:\n\n - git commit-graph verify\n - git multi-pack-index verify\n - git bundle verify\n\nSo `git refs verify` is following the same direction.\n\nI think it's a nice pattern to have this encapsulated functionality so\nthat it's easy to exercise certain subsystems in isolation. git-fsck(1)\nthen becomes a thin wrapper around these commands and is the one that\nties it all together, if desired.\n\n> \"git fsck\" should keep complaining about the failure as before,\n> whether it is done natively or by delegating to \"git refs verify\".\n\nYup.\n\nPatrick\n"},{"id":"527815","messageId":"xmqqseg1w6ki.fsf@gitster.g","threadId":"64233","inReplyTo":"aN6amIG2Sp3W500K@pks.im","subject":"Re: [PATCH 1/1] files-backend: check symref name before update","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-02T17:34:53Z","receivedAt":"2025-10-02T17:34:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> ... As \"git refs verify\" is a way to do the sanity check of\n>> the ref part (presumably without incurring cost to sanity check\n>> other aspect, like fsck does? why is it a separate command in the\n>> first place?), ...\n>\n> We have the same pattern in other command:\n>\n> - git commit-graph verify\n> - git multi-pack-index verify\n> - git bundle verify\n>\n> So `git refs verify` is following the same direction.\n\nWell, bundle falls into a searate category, though.\n\nA bundle file is a thing on its own and wants to be independently\nverifiable. A packfile (.pack alone without .idx) is also a thing\nthat may want to be independently verifiable. For that they need\nto be accessible by end-users in a form of some command.\n\nBut everything else, ...\n\n> I think it's a nice pattern to have this encapsulated functionality so\n> that it's easy to exercise certain subsystems in isolation. git-fsck(1)\n> then becomes a thin wrapper around these commands and is the one that\n> ties it all together, if desired.\n\n... including refs, commit-graphs, multi-pack-index do not have life\non their own outside the repository they originate in, so there is\nno reason to expose them as separate commands to end-users.\n\nI do agree that having a separate entry point for exercising them\nand them alone would help debugging and development, but such an\nentry point does not have to be a separate binary. It could have\nbeen \"git fsck --refs-only\" instead, for example.\n\n"},{"id":"527926","messageId":"aOIqCY4vC0Eqiz_M@ArchLinux","threadId":"64233","inReplyTo":"aN5mOTbGBcr355E6@pks.im","subject":"Re: [PATCH 1/1] files-backend: check symref name before update","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2025-10-05T08:19:21Z","receivedAt":"2025-10-05T08:19:23Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Thu, Oct 02, 2025 at 01:47:05PM +0200, Patrick Steinhardt wrote:\n> On Thu, Oct 02, 2025 at 02:54:54AM -0700, Karthik Nayak wrote:\n> > Junio C Hamano writes:\n> > \n> > > Han Young writes:\n> > >\n> > >> From: Han Young \n> > >>\n> > >> In the ref files backend, the symbolic reference name is not checked\n> > >> before an update. This could cause reference and lock files to be created\n> > >> outside the refs/ directory. Validate the reference before adding it to\n> > >> the ref update transaction.\n> > >>\n> > >> Reported-by: Sigma \n> > >> Signed-off-by: Han Young \n> > >> ---\n> > >> refs/files-backend.c | 10 ++++++++++\n> > >> 1 file changed, 10 insertions(+)\n> > >>\n> > >> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> > >> index bc3347d18..d47a8c392 100644\n> > >> --- a/refs/files-backend.c\n> > >> +++ b/refs/files-backend.c\n> > >> @@ -2516,6 +2516,16 @@ static enum ref_transaction_error split_symref_update(struct ref_update *update,\n> > >> \tstruct ref_update *new_update;\n> > >> \tunsigned int new_flags;\n> > >>\n> > >> +\t/*\n> > >> +\t * Check the referent is valid before adding it to the transaction.\n> > >> +\t */\n> > >> +\tif (!refname_is_safe(referent)) {\n> > >\n> > > Shouldn't this new condition share the logic with what is done by\n> > > fsck? IOW, after doing this\n> > >\n> > > $ echo ref: refs/../HEAD > .git/HEAD\n> > >\n> > > \"git fsck\" or \"git refs verify\" should barf (if not, we should make\n> > > them barf), and this code should use the same logic to notice that\n> > > the target of the symbolic ref is bogus.\n> > >\n> > \n> > Good point. I see that 'git fsck' does complain about this:\n> > \n> > $ git fsck\n> > Checking ref database: 100% (1/1), done.\n> > Checking object directories: 100% (256/256), done.\n> > error: invalid HEAD\n> > dangling commit ccd1771e44a18887197d3ee26ca37c2e892b9fb6\n> > dangling commit f99d68ea2c378218e2360dee4e24115c404f6a66\n> > \n> > However 'git refs verify' doesn't...\n> > \n> > $ git refs verify --verbose\n> > Checking references consistency\n> > Checking refs/heads/master\n> > Checking packed-refs file .git/packed-refs\n> > \n> > Okay, so this seems like because fsck also parses all references to mark\n> > reachability and also parses 'HEAD' via `refs_resolve_ref_unsafe()`\n> > which fails.\n> > \n> > This symref checks and checking root refs is definitely something we\n> > should consider adding to 'git refs verify'.\n> \n> Agreed! Overall, the goal is that all logic to verify references should\n> be contained in `git refs verify`, so that git-fsck(1) only needs to\n> shell out to that command to perform the full check.\n> \n> So if this logic isn't yet part of `git refs verify`, we should migrate\n> it over.\n\nIIRC, I intentionally didn't implement the code to check the HEAD. HEAD\nis so special as its correctness also indicates whether the current\ndirectory is a Git directory. We would check whether the content of HEAD\nstarts with \"refs: \". If not, we would error the user \"fatal: not a git\nrepository\".\n\nSo, the only check we should do for HEAD is check whether the symref is\nvalid. When I implemented the code, I think I wrongly forgot to add such\ncheck.\n\nThanks,\nJialuo\n"}]}