{"thread":{"id":"37609","subject":"[PATCH] sha1_file: don't convert off_t to size_t too early to avoid potential die()","startedAt":"2014-09-21T10:03:26Z","lastAt":"2014-09-22T19:41:32Z","messageCount":3,"participants":["Steffen Prohaska","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"249721","messageId":"1411293806-3087-1-git-send-email-prohaska@zib.de","threadId":"37609","inReplyTo":null,"subject":"[PATCH] sha1_file: don't convert off_t to size_t too early to avoid potential die()","fromName":"Steffen Prohaska","fromEmail":"prohaska@zib.de","sentAt":"2014-09-21T10:03:26Z","receivedAt":"2014-09-21T10:03:26Z","isPatch":true,"sender":{"key":"prohaska@zib.de","avatar":"https://avatars.githubusercontent.com/u/217580?v=4"},"body":"xsize_t() checks if an off_t argument can be safely converted to\na size_t return value. If the check is executed too early, it could\nfail for large files on 32-bit architectures even if the size_t code\npath is not taken. Other paths might be able to handle the large file.\nSpecifically, index_stream_convert_blob() is able to handle a large file\nif a filter is configured that returns a small result.\n\nSigned-off-by: Steffen Prohaska \n---\n\nThis patch should be applied on top of sp/stream-clean-filter.\n\nindex_stream() might internally also be able to handle large files to\nsome extent. But it uses size_t for its third argument, and we must\nalready die() when calling it. It might be a good idea to convert its\ninterface to use off_t and push the size checks further down the stack.\nIn general, it might be good idea to carefully consider whether to use\noff_t or size_t when passing file-related sizes around. To me it looks\nlike a separate issue for a separate patch series (I have no specific\nplans to prepare one).\n\n sha1_file.c | 13 +++++++++----\n 1 file changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 5b0e67a..6f18c22 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -3180,17 +3180,22 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st,\n \t enum object_type type, const char *path, unsigned flags)\n {\n \tint ret;\n-\tsize_t size = xsize_t(st->st_size);\n \n+\t/*\n+\t * Call xsize_t() only when needed to avoid potentially unnecessary\n+\t * die() for large files.\n+\t */\n \tif (type == OBJ_BLOB && path && would_convert_to_git_filter_fd(path))\n \t\tret = index_stream_convert_blob(sha1, fd, path, flags);\n \telse if (!S_ISREG(st->st_mode))\n \t\tret = index_pipe(sha1, fd, type, path, flags);\n-\telse if (size <= big_file_threshold || type != OBJ_BLOB ||\n+\telse if (st->st_size <= big_file_threshold || type != OBJ_BLOB ||\n \t\t (path && would_convert_to_git(path)))\n-\t\tret = index_core(sha1, fd, size, type, path, flags);\n+\t\tret = index_core(sha1, fd, xsize_t(st->st_size), type, path,\n+\t\t\t\t flags);\n \telse\n-\t\tret = index_stream(sha1, fd, size, type, path, flags);\n+\t\tret = index_stream(sha1, fd, xsize_t(st->st_size), type, path,\n+\t\t\t\t flags);\n \tclose(fd);\n \treturn ret;\n }\n-- \n2.1.0.139.g351b19f\n"},{"id":"249743","messageId":"xmqqfvfjlf4k.fsf@gitster.dls.corp.google.com","threadId":"37609","inReplyTo":"1411293806-3087-1-git-send-email-prohaska@zib.de","subject":"Re: [PATCH] sha1_file: don't convert off_t to size_t too early to avoid potential die()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-22T17:49:31Z","receivedAt":"2014-09-22T17:49:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Steffen Prohaska writes:\n\n> xsize_t() checks if an off_t argument can be safely converted to\n> a size_t return value. If the check is executed too early, it could\n> fail for large files on 32-bit architectures even if the size_t code\n> path is not taken. Other paths might be able to handle the large file.\n> Specifically, index_stream_convert_blob() is able to handle a large file\n> if a filter is configured that returns a small result.\n>\n> Signed-off-by: Steffen Prohaska \n> ---\n>\n> This patch should be applied on top of sp/stream-clean-filter.\n>\n> index_stream() might internally also be able to handle large files to\n> some extent. But it uses size_t for its third argument, and we must\n> already die() when calling it. It might be a good idea to convert its\n> interface to use off_t and push the size checks further down the stack.\n\nYes, if we want to futz in this area, I think that would be the\nright approach.\n"},{"id":"249750","messageId":"xmqqvbofjvdf.fsf@gitster.dls.corp.google.com","threadId":"37609","inReplyTo":"1411293806-3087-1-git-send-email-prohaska@zib.de","subject":"Re: [PATCH] sha1_file: don't convert off_t to size_t too early to avoid potential die()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-22T19:41:32Z","receivedAt":"2014-09-22T19:41:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Steffen Prohaska writes:\n\n> This patch should be applied on top of sp/stream-clean-filter.\n\n... or it can be squashed in as a fix, as the topic is not yet in\n'next'.\n\n> index_stream() might internally also be able to handle large files to\n> some extent. But it uses size_t for its third argument, and we must\n> already die() when calling it. It might be a good idea to convert its\n> interface to use off_t and push the size checks further down the stack.\n> In general, it might be good idea to carefully consider whether to use\n> off_t or size_t when passing file-related sizes around. To me it looks\n> like a separate issue for a separate patch series (I have no specific\n> plans to prepare one).\n>\n> sha1_file.c | 13 +++++++++----\n> 1 file changed, 9 insertions(+), 4 deletions(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 5b0e67a..6f18c22 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -3180,17 +3180,22 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st,\n> \t enum object_type type, const char *path, unsigned flags)\n> {\n> \tint ret;\n> -\tsize_t size = xsize_t(st->st_size);\n> \n> +\t/*\n> +\t * Call xsize_t() only when needed to avoid potentially unnecessary\n> +\t * die() for large files.\n> +\t */\n> \tif (type == OBJ_BLOB && path && would_convert_to_git_filter_fd(path))\n> \t\tret = index_stream_convert_blob(sha1, fd, path, flags);\n> \telse if (!S_ISREG(st->st_mode))\n> \t\tret = index_pipe(sha1, fd, type, path, flags);\n> -\telse if (size <= big_file_threshold || type != OBJ_BLOB ||\n> +\telse if (st->st_size <= big_file_threshold || type != OBJ_BLOB ||\n> \t\t (path && would_convert_to_git(path)))\n> -\t\tret = index_core(sha1, fd, size, type, path, flags);\n> +\t\tret = index_core(sha1, fd, xsize_t(st->st_size), type, path,\n> +\t\t\t\t flags);\n> \telse\n> -\t\tret = index_stream(sha1, fd, size, type, path, flags);\n> +\t\tret = index_stream(sha1, fd, xsize_t(st->st_size), type, path,\n> +\t\t\t\t flags);\n> \tclose(fd);\n> \treturn ret;\n> }\n"}]}