{"thread":{"id":"62856","subject":"[PATCH v3 4/4] commit.c: Fix type conversation warnings from msvc","startedAt":"2025-01-26T13:00:42Z","lastAt":"2025-01-29T16:53:13Z","messageCount":3,"participants":["Sören Krecker","Patrick Steinhardt","Phillip Wood"],"isPatch":true,"patchVersion":3,"patchTotal":4},"messages":[{"id":"511209","messageId":"20250126130038.3277-1-soekkle@freenet.de","threadId":"62856","inReplyTo":null,"subject":"[PATCH v3 4/4] commit.c: Fix type conversation warnings from msvc","fromName":"Sören Krecker","fromEmail":"soekkle@freenet.de","sentAt":"2025-01-26T13:00:38Z","receivedAt":"2025-01-26T13:00:42Z","isPatch":true,"sender":{"key":"soekkle@freenet.de","avatar":"https://avatars.githubusercontent.com/u/6253399?v=4"},"body":"Fix compiler warnings from msvc in commit.c for value truncation from 64\nbit to 32 bit integers.\n\nAlso switch from int to size_t for all variables with result of strlen()\nwhich cannot become negative.\n\nSigned-off-by: Sören Krecker <soekkle@freenet.de>\n---\n commit.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 540660359d..c9cc56bd9f 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -466,8 +466,8 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n \tstruct object_id parent;\n \tstruct commit_list **pptr;\n \tstruct commit_graft *graft;\n-\tconst int tree_entry_len = the_hash_algo->hexsz + 5;\n-\tconst int parent_entry_len = the_hash_algo->hexsz + 7;\n+\tconst size_t tree_entry_len = the_hash_algo->hexsz + 5;\n+\tconst size_t parent_entry_len = the_hash_algo->hexsz + 7;\n \tstruct tree *tree;\n \n \tif (item->object.parsed)\n@@ -1114,10 +1114,10 @@ static const char *gpg_sig_headers[] = {\n \n int add_header_signature(struct strbuf *buf, struct strbuf *sig, const struct git_hash_algo *algo)\n {\n-\tint inspos, copypos;\n+\tssize_t inspos, copypos;\n \tconst char *eoh;\n \tconst char *gpg_sig_header = gpg_sig_headers[hash_algo_by_ptr(algo)];\n-\tint gpg_sig_header_len = strlen(gpg_sig_header);\n+\tsize_t gpg_sig_header_len = strlen(gpg_sig_header);\n \n \t/* find the end of the header */\n \teoh = strstr(buf->buf, \"\\n\\n\");\n@@ -1530,7 +1530,7 @@ int commit_tree(const char *msg, size_t msg_len, const struct object_id *tree,\n \treturn result;\n }\n \n-static int find_invalid_utf8(const char *buf, int len)\n+static int find_invalid_utf8(const char *buf, size_t len)\n {\n \tint offset = 0;\n \tstatic const unsigned int max_codepoint[] = {\n@@ -1539,7 +1539,7 @@ static int find_invalid_utf8(const char *buf, int len)\n \n \twhile (len) {\n \t\tunsigned char c = *buf++;\n-\t\tint bytes, bad_offset;\n+\t\tsize_t bytes, bad_offset;\n \t\tunsigned int codepoint;\n \t\tunsigned int min_val, max_val;\n \n-- \n2.39.5\n\n"},{"id":"511225","messageId":"Z5c1ITUkNqB7CeHW@pks.im","threadId":"62856","inReplyTo":"20250126130038.3277-1-soekkle@freenet.de","subject":"Re: [PATCH v3 4/4] commit.c: Fix type conversation warnings from msvc","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-27T07:26:25Z","receivedAt":"2025-01-27T07:26:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Jan 26, 2025 at 02:00:38PM +0100, Sören Krecker wrote:\n> diff --git a/commit.c b/commit.c\n> index 540660359d..c9cc56bd9f 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -1530,7 +1530,7 @@ int commit_tree(const char *msg, size_t msg_len, const struct object_id *tree,\n>  \treturn result;\n>  }\n>  \n> -static int find_invalid_utf8(const char *buf, int len)\n> +static int find_invalid_utf8(const char *buf, size_t len)\n>  {\n>  \tint offset = 0;\n\nShouldn't the type of `offset` be adjusted, as well?\n\nPatrick\n"},{"id":"511421","messageId":"a3f49105-e66b-4bfc-9fa5-902c77765e4a@gmail.com","threadId":"62856","inReplyTo":"20250126130038.3277-1-soekkle@freenet.de","subject":"Re: [PATCH v3 4/4] commit.c: Fix type conversation warnings from msvc","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-01-29T16:53:08Z","receivedAt":"2025-01-29T16:53:13Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Sören\n\nOn 26/01/2025 13:00, Sören Krecker wrote:\n\n> -\tconst int tree_entry_len = the_hash_algo->hexsz + 5;\n> -\tconst int parent_entry_len = the_hash_algo->hexsz + 7;\n> +\tconst size_t tree_entry_len = the_hash_algo->hexsz + 5;\n> +\tconst size_t parent_entry_len = the_hash_algo->hexsz + 7;\n\nAs Junio has previously pointed out it might make more sense to change \nthe type of the_hash_algo->hexsz. What is the advantage of using size_t \nhere?\n\n>   int add_header_signature(struct strbuf *buf, struct strbuf *sig, const struct git_hash_algo *algo)\n>   {\n> -\tint inspos, copypos;\n> +\tssize_t inspos, copypos;\n\nNote that POSIX allows \"ssize_t\" to be narrower than \"int\". We have at \nleast one platform where that is the case [see c14e5a1a501 \n(transport-helper: use xread instead of read, 2019-01-03)] so I'm not \nsure this change is a good idea.\n\n>   \tconst char *eoh;\n>   \tconst char *gpg_sig_header = gpg_sig_headers[hash_algo_by_ptr(algo)];\n> -\tint gpg_sig_header_len = strlen(gpg_sig_header);\n> +\tsize_t gpg_sig_header_len = strlen(gpg_sig_header);\n\nIt is really unfortunate that the compiler cannot deduce that there is \nnot any truncation here as the longest string is \"gpgsig-sha256\". I \nwonder if we should add a helper for cases like this something like\n\n\tint strlen_int(const char *s) {\n\t\treturn cast_size_t_to_int(strlen(s));\n\t}\n\nrather than having to deal with the fallout from changing \"int\" to \"size_t\"\n\n> -static int find_invalid_utf8(const char *buf, int len)\n> +static int find_invalid_utf8(const char *buf, size_t len)\n\nI think changing the type of \"len\" is an improvement (even if it hard to \nsee anyone creating such a long commit message) as it matches the \nbuf->len in verify_utf8() which is the only caller of this function. \nHowever the conversion is incomplete. If we are to accommodate buffers \nlonger than INT_MAX we need to change the return type as well. As it \nstands bad_offset is changed to size_t but truncated to int when the \nfunction returns. Patrick has already pointed out that the type of \n\"offset\" needs to match \"len\" as the loop does\n\n\tlen--;\n\toffset++;\n\nand\n\n\tbad_offset = offset - 1;\n\nverify_utf8() uses a variable of type long to track the position so that \nshould be changed as well if we're really going to support size_t length \nbuffers.\n\nI think that a good approach to fixing these warnings would be to ask \n\"does it make sense to use size_t here?\". If the answer is \"yes\" then we \nshould focus on converting the function to work correctly with size_t \nrather than on fixing the compiler warnings. That way we are more likely \nto avoid subtle bugs like this as our focus is on the conversion rather \nthan the compiler warnings which will be fixed as a by-product of the \nconversion. If the answer to the question is \"no\" then we should look to \nchange the types of the other variable in the assignment or use \nsomething like cast_size_t_to_int() or case_size_t_to_ulong() as \nappropriate. There is a danger that we adopt an approach of \"change the \ntype to size_t to silence the warnings\" which leads to code that looks \nlike it handles size_t correctly but in fact contains subtle bugs.\n\nBest Wishes\n\nPhillip\n\n\n>   {\n>   \tint offset = 0;\n>   \tstatic const unsigned int max_codepoint[] = {\n> @@ -1539,7 +1539,7 @@ static int find_invalid_utf8(const char *buf, int len)\n>   \n>   \twhile (len) {\n>   \t\tunsigned char c = *buf++;\n> -\t\tint bytes, bad_offset;\n> +\t\tsize_t bytes, bad_offset;\n>   \t\tunsigned int codepoint;\n>   \t\tunsigned int min_val, max_val;\n>   \n\n"}]}