{"thread":{"id":"53396","subject":"[PATCH] builtin/receive-pack: avoid generic function name hmac","startedAt":"2020-05-05T05:46:53Z","lastAt":"2020-05-05T11:48:45Z","messageCount":8,"participants":["Carlo Marcelo Arenas Belón","Eric Sunshine","Junio C Hamano","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"397052","messageId":"20200505054630.5821-1-carenas@gmail.com","threadId":"53396","inReplyTo":null,"subject":"[PATCH] builtin/receive-pack: avoid generic function name hmac","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-05T05:46:30Z","receivedAt":"2020-05-05T05:46:53Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"fabec2c5c3 (builtin/receive-pack: switch to use the_hash_algo, 2019-08-18)\nrenames hmac_sha1 to hmac, as it was updated to (optionally) use the hash\nfunction used by git (which won't be sha1 in the future).\n\nhmac() is provided by NetBSD > 8 libc and conflicts as shown by :\n\nbuiltin/receive-pack.c:421:13: error: conflicting types for 'hmac'\n static void hmac(unsigned char *out,\n ^~~~\nIn file included from ./git-compat-util.h:172:0,\n from ./builtin.h:4,\n from builtin/receive-pack.c:1:\n/usr/include/stdlib.h:305:10: note: previous declaration of 'hmac' was here\n ssize_t hmac(const char *, const void *, size_t, const void *, size_t, void *,\n ^~~~\n\nWhile the conflict, posses the question of why are we even implementing our\nown RFC 2104 complaint HMAC while alternatives are readily available, the\nsimplest solution is to make sure the name is not as generic so it would\nconflict with an equivalent one from the system (or linked libraries); so\nrename it again to hmac_hash to reflect it will use the git's defined hash\nfunction.\n---\n builtin/receive-pack.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 2cc18bbffd..b1d939e7ed 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -418,7 +418,7 @@ static int copy_to_sideband(int in, int out, void *arg)\n \treturn 0;\n }\n \n-static void hmac(unsigned char *out,\n+static void hmac_hash(unsigned char *out,\n \t\t const char *key_in, size_t key_len,\n \t\t const char *text, size_t text_len)\n {\n@@ -463,7 +463,7 @@ static char *prepare_push_cert_nonce(const char *path, timestamp_t stamp)\n \tunsigned char hash[GIT_MAX_RAWSZ];\n \n \tstrbuf_addf(&buf, \"%s:%\"PRItime, path, stamp);\n-\thmac(hash, buf.buf, buf.len, cert_nonce_seed, strlen(cert_nonce_seed));\n+\thmac_hash(hash, buf.buf, buf.len, cert_nonce_seed, strlen(cert_nonce_seed));\n \tstrbuf_release(&buf);\n \n \t/* RFC 2104 5. HMAC-SHA1-80 */\n-- \n2.26.2.686.gfaf46a9ccd\n\n"},{"id":"397053","messageId":"CAPig+cRE60kw-PeF=Di7O9Kw1P1CctCX0drDzGX3diVQ2-Fu1Q@mail.gmail.com","threadId":"53396","inReplyTo":"20200505054630.5821-1-carenas@gmail.com","subject":"Re: [PATCH] builtin/receive-pack: avoid generic function name hmac","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-05-05T05:52:23Z","receivedAt":"2020-05-05T05:52:49Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, May 5, 2020 at 1:46 AM Carlo Marcelo Arenas Belón\n wrote:\n> [...]\n> While the conflict, posses the question of why are we even implementing our\n> own RFC 2104 complaint HMAC while alternatives are readily available, the\n> simplest solution is to make sure the name is not as generic so it would\n> conflict with an equivalent one from the system (or linked libraries); so\n> rename it again to hmac_hash to reflect it will use the git's defined hash\n> function.\n> ---\n\nMissing sign-off.\n"},{"id":"397055","messageId":"xmqqsggec2g1.fsf@gitster.c.googlers.com","threadId":"53396","inReplyTo":"20200505054630.5821-1-carenas@gmail.com","subject":"Re: [PATCH] builtin/receive-pack: avoid generic function name hmac","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-05T06:37:18Z","receivedAt":"2020-05-05T06:37:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> fabec2c5c3 (builtin/receive-pack: switch to use the_hash_algo, 2019-08-18)\n> renames hmac_sha1 to hmac, as it was updated to (optionally) use the hash\n> function used by git (which won't be sha1 in the future).\n>\n> hmac() is provided by NetBSD > 8 libc and conflicts as shown by :\n>\n> builtin/receive-pack.c:421:13: error: conflicting types for 'hmac'\n> static void hmac(unsigned char *out,\n> ^~~~\n> In file included from ./git-compat-util.h:172:0,\n> from ./builtin.h:4,\n> from builtin/receive-pack.c:1:\n> /usr/include/stdlib.h:305:10: note: previous declaration of 'hmac' was here\n> ssize_t hmac(const char *, const void *, size_t, const void *, size_t, void *,\n> ^~~~\n\nIncluding is allowed to contaminate the namespace this way?\n\nAt least\nhttps://pubs.opengroup.org/onlinepubs/9699919799/basedefs/stdlib.h.html\ndoes not seem to list hmac() there.\n\n> While the conflict, posses the question of why are we even implementing our\n> own RFC 2104 complaint HMAC while alternatives are readily available, the\n> simplest solution is to make sure the name is not as generic so it would\n> conflict with an equivalent one from the system (or linked libraries); so\n> rename it again to hmac_hash to reflect it will use the git's defined hash\n> function.\n\nI do not mind renaming ours, as it is harder to get the \nfixed on the NetBSD, but I would phrase s/equivalent/unrelated/ (or\neven \"irrelevant\"), as it is clearly not an \"alternative\" that is\nreadily available outside NetBSD. Otherwise we would have found\nthis a lot sooner (it used to be called hmac_sha1() and renamed to\nhmac() in August last year).\n\n> ---\n> builtin/receive-pack.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n\nMissing sign-off. The patch text is trivially correct.\n\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 2cc18bbffd..b1d939e7ed 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -418,7 +418,7 @@ static int copy_to_sideband(int in, int out, void *arg)\n> \treturn 0;\n> }\n> \n> -static void hmac(unsigned char *out,\n> +static void hmac_hash(unsigned char *out,\n> \t\t const char *key_in, size_t key_len,\n> \t\t const char *text, size_t text_len)\n> {\n> @@ -463,7 +463,7 @@ static char *prepare_push_cert_nonce(const char *path, timestamp_t stamp)\n> \tunsigned char hash[GIT_MAX_RAWSZ];\n> \n> \tstrbuf_addf(&buf, \"%s:%\"PRItime, path, stamp);\n> -\thmac(hash, buf.buf, buf.len, cert_nonce_seed, strlen(cert_nonce_seed));\n> +\thmac_hash(hash, buf.buf, buf.len, cert_nonce_seed, strlen(cert_nonce_seed));\n> \tstrbuf_release(&buf);\n> \n> \t/* RFC 2104 5. HMAC-SHA1-80 */\n"},{"id":"397058","messageId":"20200505071834.GB1218@Carlos-MBP","threadId":"53396","inReplyTo":"xmqqsggec2g1.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] builtin/receive-pack: avoid generic function name hmac","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-05T07:18:34Z","receivedAt":"2020-05-05T07:18:39Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, May 04, 2020 at 11:37:18PM -0700, Junio C Hamano wrote:\n> Carlo Marcelo Arenas Belón writes:\n> > While the conflict, posses the question of why are we even implementing our\n> > own RFC 2104 complaint HMAC while alternatives are readily available, the\n> > simplest solution is to make sure the name is not as generic so it would\n> > conflict with an equivalent one from the system (or linked libraries); so\n> > rename it again to hmac_hash to reflect it will use the git's defined hash\n> > function.\n> \n> I do not mind renaming ours, as it is harder to get the \n> fixed on the NetBSD, but I would phrase s/equivalent/unrelated/ (or\n> even \"irrelevant\"), as it is clearly not an \"alternative\" that is\n> readily available outside NetBSD. Otherwise we would have found\n> this a lot sooner (it used to be called hmac_sha1() and renamed to\n> hmac() in August last year).\n\nfair enough, would probably better drop this paragraph; but I wasn't\nreferring to hmac() in NetBSD, but the fact that we are linking against\nOpenSSL (or their equivalent in macOS and Windows) that provide HMAC,\nand even using it (but with MD5) already for git-imap-send.\n\nthe SHA256 feature will even bring (alternatively) libgcrypt to the mix\nso it seemed strange that we didn't abstract this hmac function as part\nof that interface instead since we seemed to have plenty of alternatives\nto chose from and would had make IMHO more sense as a fallback inside\nthere.\n\nCarlo\n"},{"id":"397066","messageId":"20200505092421.GF6530@camp.crustytoothpaste.net","threadId":"53396","inReplyTo":"20200505054630.5821-1-carenas@gmail.com","subject":"Re: [PATCH] builtin/receive-pack: avoid generic function name hmac","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-05-05T09:24:21Z","receivedAt":"2020-05-05T09:24:30Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-05-05 at 05:46:30, Carlo Marcelo Arenas Belón wrote:\n> fabec2c5c3 (builtin/receive-pack: switch to use the_hash_algo, 2019-08-18)\n> renames hmac_sha1 to hmac, as it was updated to (optionally) use the hash\n> function used by git (which won't be sha1 in the future).\n> \n> hmac() is provided by NetBSD > 8 libc and conflicts as shown by :\n> \n> builtin/receive-pack.c:421:13: error: conflicting types for 'hmac'\n> static void hmac(unsigned char *out,\n> ^~~~\n> In file included from ./git-compat-util.h:172:0,\n> from ./builtin.h:4,\n> from builtin/receive-pack.c:1:\n> /usr/include/stdlib.h:305:10: note: previous declaration of 'hmac' was here\n> ssize_t hmac(const char *, const void *, size_t, const void *, size_t, void *,\n> ^~~~\n> \n> While the conflict, posses the question of why are we even implementing our\n> own RFC 2104 complaint HMAC while alternatives are readily available, the\n> simplest solution is to make sure the name is not as generic so it would\n> conflict with an equivalent one from the system (or linked libraries); so\n> rename it again to hmac_hash to reflect it will use the git's defined hash\n> function.\n\nThis is fine, although as others mentioned, there's a missing sign-off\nhere. While it may seem that we can use OpenSSL's version here, we\ncannot, since not all versions build with OpenSSL (for example, Debian\ndoes not). In fact, it's possible to build core Git without any crypto\nlibrary at all if you don't need HTTPS support.\n\nI appreciate you pointing this out, since it was a surprise to me that\nthis would be in stdlib.h without further guards, although perhaps it\ndoes have guards and we coax NetBSD to provide more than standard\nfunctionality (as we do with glibc).\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"397067","messageId":"20200505095326.36374-1-carenas@gmail.com","threadId":"53396","inReplyTo":"20200505054630.5821-1-carenas@gmail.com","subject":"[PATCH v2] builtin/receive-pack: avoid generic function name hmac()","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-05T09:53:26Z","receivedAt":"2020-05-05T09:53:35Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"fabec2c5c3 (builtin/receive-pack: switch to use the_hash_algo, 2019-08-18)\nrenames hmac_sha1 to hmac, as it was updated to use the hash function used\nby git (which won't be sha1 in the future).\n\nhmac() is provided by NetBSD >= 8 libc and therefore conflicts as shown by :\n\nbuiltin/receive-pack.c:421:13: error: conflicting types for 'hmac'\n static void hmac(unsigned char *out,\n ^~~~\nIn file included from ./git-compat-util.h:172:0,\n from ./builtin.h:4,\n from builtin/receive-pack.c:1:\n/usr/include/stdlib.h:305:10: note: previous declaration of 'hmac' was here\n ssize_t hmac(const char *, const void *, size_t, const void *, size_t, void *,\n ^~~~\n\nRename it again to hmac_hash to reflect it will use the git's defined hash\nfunction and avoid the conflict, while at it update a comment to better\ndescribe the HMAC function that was used.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\nv2\n* cleaner commit message that even has an SOB\n\n builtin/receive-pack.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 2cc18bbffd..39b0c3d70b 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -418,7 +418,7 @@ static int copy_to_sideband(int in, int out, void *arg)\n \treturn 0;\n }\n \n-static void hmac(unsigned char *out,\n+static void hmac_hash(unsigned char *out,\n \t\t const char *key_in, size_t key_len,\n \t\t const char *text, size_t text_len)\n {\n@@ -463,10 +463,10 @@ static char *prepare_push_cert_nonce(const char *path, timestamp_t stamp)\n \tunsigned char hash[GIT_MAX_RAWSZ];\n \n \tstrbuf_addf(&buf, \"%s:%\"PRItime, path, stamp);\n-\thmac(hash, buf.buf, buf.len, cert_nonce_seed, strlen(cert_nonce_seed));\n+\thmac_hash(hash, buf.buf, buf.len, cert_nonce_seed, strlen(cert_nonce_seed));\n \tstrbuf_release(&buf);\n \n-\t/* RFC 2104 5. HMAC-SHA1-80 */\n+\t/* RFC 2104 5. HMAC-SHA1 or HMAC-SHA256 */\n \tstrbuf_addf(&buf, \"%\"PRItime\"-%.*s\", stamp, (int)the_hash_algo->hexsz, hash_to_hex(hash));\n \treturn strbuf_detach(&buf, NULL);\n }\n\nbase-commit: af6b65d45ef179ed52087e80cb089f6b2349f4ec\n-- \n2.26.2.686.gfaf46a9ccd\n\n"},{"id":"397068","messageId":"20200505101200.GC1218@Carlos-MBP","threadId":"53396","inReplyTo":"20200505092421.GF6530@camp.crustytoothpaste.net","subject":"Re: [PATCH] builtin/receive-pack: avoid generic function name hmac","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-05T10:12:00Z","receivedAt":"2020-05-05T10:12:08Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, May 05, 2020 at 09:24:21AM +0000, brian m. carlson wrote:\n> I appreciate you pointing this out, since it was a surprise to me that\n> this would be in stdlib.h without further guards, although perhaps it\n> does have guards and we coax NetBSD to provide more than standard\n> functionality (as we do with glibc).\n\nwe define NETBSD_SOURCE since 9a695fbf38 (NetBSD compilation fix, 2009-04-26)\n\nCarlo\n"},{"id":"397078","messageId":"20200505114837.GG6530@camp.crustytoothpaste.net","threadId":"53396","inReplyTo":"20200505095326.36374-1-carenas@gmail.com","subject":"Re: [PATCH v2] builtin/receive-pack: avoid generic function name hmac()","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-05-05T11:48:37Z","receivedAt":"2020-05-05T11:48:45Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-05-05 at 09:53:26, Carlo Marcelo Arenas Belón wrote:\n> fabec2c5c3 (builtin/receive-pack: switch to use the_hash_algo, 2019-08-18)\n> renames hmac_sha1 to hmac, as it was updated to use the hash function used\n> by git (which won't be sha1 in the future).\n> \n> hmac() is provided by NetBSD >= 8 libc and therefore conflicts as shown by :\n> \n> builtin/receive-pack.c:421:13: error: conflicting types for 'hmac'\n> static void hmac(unsigned char *out,\n> ^~~~\n> In file included from ./git-compat-util.h:172:0,\n> from ./builtin.h:4,\n> from builtin/receive-pack.c:1:\n> /usr/include/stdlib.h:305:10: note: previous declaration of 'hmac' was here\n> ssize_t hmac(const char *, const void *, size_t, const void *, size_t, void *,\n> ^~~~\n> \n> Rename it again to hmac_hash to reflect it will use the git's defined hash\n> function and avoid the conflict, while at it update a comment to better\n> describe the HMAC function that was used.\n> \n> Signed-off-by: Carlo Marcelo Arenas Belón \n\nLooks good. Thanks again for the patch.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"}]}