Re: [PATCH 12/12] refs.c: fix handling of badly named refs
Ronnie Sahlberg sahlb...@google.com writes: We currently do not handle badly named refs well : $ cp .git/refs/heads/master .git/refs/heads/master.@\*@\\. $ git branch fatal: Reference has invalid format: 'refs/heads/master.@*@\.' $ git branch -D master.@\*@\\. error: branch 'master.@*@\.' not found. But we can not really recover from a badly named ref with less than manually deleting the .git/refs/heads/refname file. Change resolve_ref_unsafe to take a flags field instead of a 'reading' boolean and update all callers that used a non-zero value for reading to pass the flag RESOLVE_REF_READING instead. Add another flag RESOLVE_REF_ALLOW_BAD_NAME that will make resolve_ref_unsafe skip checking the refname for sanity and use this from branch.c so that we will be able to call resolve_ref_unsafe on such refs when trying to delete it. Makes sense. Add checks for refname sanity when updating (not deleting) a ref in ref_transaction_update and in ref_transaction_create to make the transaction fail if an attempt is made to create/update a badly named ref. Since all ref changes will later go through the transaction layer this means we no longer need to check for and fail for bad refnames in lock_ref_sha1_basic. Change lock_ref_sha1_basic to not fail for bad refnames. Just check the refname, and print an error, and remember that the refname is bad so that we can skip calling verify_lock(). This is somewhat puzzling, though. @@ -2180,6 +2196,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, else unable_to_lock_index_die(ref_file, errno); } + if (bad_refname) + return lock; Hmph. If the only offence was that the ref was named badly due to historically loose code, does the caller not still benefit from the verify-lock check to make sure that other people did not muck with the ref while we were planning to update it? return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock; error_return: -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/12] refs.c: fix handling of badly named refs
On Tue, Jul 22, 2014 at 1:41 PM, Junio C Hamano gits...@pobox.com wrote: Ronnie Sahlberg sahlb...@google.com writes: We currently do not handle badly named refs well : $ cp .git/refs/heads/master .git/refs/heads/master.@\*@\\. $ git branch fatal: Reference has invalid format: 'refs/heads/master.@*@\.' $ git branch -D master.@\*@\\. error: branch 'master.@*@\.' not found. But we can not really recover from a badly named ref with less than manually deleting the .git/refs/heads/refname file. Change resolve_ref_unsafe to take a flags field instead of a 'reading' boolean and update all callers that used a non-zero value for reading to pass the flag RESOLVE_REF_READING instead. Add another flag RESOLVE_REF_ALLOW_BAD_NAME that will make resolve_ref_unsafe skip checking the refname for sanity and use this from branch.c so that we will be able to call resolve_ref_unsafe on such refs when trying to delete it. Makes sense. Add checks for refname sanity when updating (not deleting) a ref in ref_transaction_update and in ref_transaction_create to make the transaction fail if an attempt is made to create/update a badly named ref. Since all ref changes will later go through the transaction layer this means we no longer need to check for and fail for bad refnames in lock_ref_sha1_basic. Change lock_ref_sha1_basic to not fail for bad refnames. Just check the refname, and print an error, and remember that the refname is bad so that we can skip calling verify_lock(). This is somewhat puzzling, though. @@ -2180,6 +2196,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, else unable_to_lock_index_die(ref_file, errno); } + if (bad_refname) + return lock; Hmph. If the only offence was that the ref was named badly due to historically loose code, does the caller not still benefit from the verify-lock check to make sure that other people did not muck with the ref while we were planning to update it? I don't think we need to do that. That would imply that we would need to be able to also allow reading the content of a badly named ref. Currently a badly named ref can not be accessed by any function except git branch -D badlynamedref which contains the special flag that allows locking it eventhough the ref has an illegal name. So no one else should be able to read or modify the ref at all. I think it is sufficient for this case to just have the semantics just delete it, I don't care what it used to point to. for this special case git branch -D badrefname so therefore since it is not the content of the ref but the name of the ref itself we have a problem with I don't think we need to read the old value or verify it. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/12] refs.c: fix handling of badly named refs
On Tue, Jul 22, 2014 at 2:30 PM, Ronnie Sahlberg sahlb...@google.com wrote: On Tue, Jul 22, 2014 at 1:41 PM, Junio C Hamano gits...@pobox.com wrote: Ronnie Sahlberg sahlb...@google.com writes: We currently do not handle badly named refs well : $ cp .git/refs/heads/master .git/refs/heads/master.@\*@\\. $ git branch fatal: Reference has invalid format: 'refs/heads/master.@*@\.' $ git branch -D master.@\*@\\. error: branch 'master.@*@\.' not found. But we can not really recover from a badly named ref with less than manually deleting the .git/refs/heads/refname file. Change resolve_ref_unsafe to take a flags field instead of a 'reading' boolean and update all callers that used a non-zero value for reading to pass the flag RESOLVE_REF_READING instead. Add another flag RESOLVE_REF_ALLOW_BAD_NAME that will make resolve_ref_unsafe skip checking the refname for sanity and use this from branch.c so that we will be able to call resolve_ref_unsafe on such refs when trying to delete it. Makes sense. Add checks for refname sanity when updating (not deleting) a ref in ref_transaction_update and in ref_transaction_create to make the transaction fail if an attempt is made to create/update a badly named ref. Since all ref changes will later go through the transaction layer this means we no longer need to check for and fail for bad refnames in lock_ref_sha1_basic. Change lock_ref_sha1_basic to not fail for bad refnames. Just check the refname, and print an error, and remember that the refname is bad so that we can skip calling verify_lock(). This is somewhat puzzling, though. @@ -2180,6 +2196,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, else unable_to_lock_index_die(ref_file, errno); } + if (bad_refname) + return lock; Hmph. If the only offence was that the ref was named badly due to historically loose code, does the caller not still benefit from the verify-lock check to make sure that other people did not muck with the ref while we were planning to update it? I don't think we need to do that. That would imply that we would need to be able to also allow reading the content of a badly named ref. Currently a badly named ref can not be accessed by any function except git branch -D badlynamedref which contains the special flag that allows locking it eventhough the ref has an illegal name. So no one else should be able to read or modify the ref at all. I think it is sufficient for this case to just have the semantics just delete it, I don't care what it used to point to. for this special case git branch -D badrefname so therefore since it is not the content of the ref but the name of the ref itself we have a problem with I don't think we need to read the old value or verify it. It is also that prior to this change we could not access these badly named refs at all. This change tries to be careful to not open up too much as it tries to only allow git branch -D and nothing else to start working for such refs. (To avoid accidentally opening things up so that it becomes possible to start using/depending on such refs) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/12] refs.c: fix handling of badly named refs
Ronnie Sahlberg sahlb...@google.com writes: I don't think we need to do that. That would imply that we would need to be able to also allow reading the content of a badly named ref. Currently a badly named ref can not be accessed by any function except git branch -D badlynamedref which contains the special flag that allows locking it eventhough the ref has an illegal name. So no one else should be able to read or modify the ref at all. OK. I think it is sufficient for this case to just have the semantics just delete it, I don't care what it used to point to. for this special case git branch -D badrefname so therefore since it is not the content of the ref but the name of the ref itself we have a problem with I don't think we need to read the old value or verify it. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
