{"thread":{"id":"15122","subject":"[PATCH] compat/snprintf.c: handle snprintf's that always return the # chars transmitted","startedAt":"2008-08-21T01:53:50Z","lastAt":"2008-08-21T08:06:31Z","messageCount":2,"participants":["Brandon Casey","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"87919","messageId":"nYV6zI97uDEOlIEFjH5oAZW1lTFm0mOVlTrIxzRyOY5CCrUJw7ROgA@cipher.nrlssc.navy.mil","threadId":"15122","inReplyTo":null,"subject":"[PATCH] compat/snprintf.c: handle snprintf's that always return the # chars transmitted","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2008-08-21T01:53:50Z","receivedAt":"2008-08-21T01:53:50Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Some platforms provide a horribly broken snprintf. More broken than the\nplatforms that return -1 when there is too little space in the target buffer\nfor the formatted string. Some platforms provide an snprintf which _always_\nreturns the number of characters transmitted to the buffer, regardless of\nwhether there was enough space or not.\n\nIRIX 6.5 is such a platform. IRIX does have a working snprintf(), but it\nis only provided when _NO_XOPEN5 evaluates to zero, and this only happens\nif _XOPEN_SOURCE is defined, but definition of _XOPEN_SOURCE prevents\ninclusion of many other common functions and defines. So it must be avoided.\n\nWork around these horribly broken snprintf implementations by detecting an\nsnprintf call which results in the number of transmitted characters exactly\nequal to the length of our buffer and retrying with a larger buffer just to\nbe safe.\n\nSigned-off-by: Brandon Casey \n---\n\nI was cautious about this change and how it would affect the calling sequence\nin strbuf_addf where after an initial failure of vsnprintf, strbuf_grow is\nused to increase the strbuf by exactly the number of characters needed and\nthen vsnprintf is called again.\n\nIf strbuf_grow often allocated exactly the number of characters requested,\nthen the failure code path of git_vsnprintf would be triggered after each\nsuch strbuf_grow. The result of git_vsnprintf would still be successful,\nbut an unnecessary malloc and vsnprintf would be called.\n\nThanks to ALLOC_GROW, I don't think it is common for strbuf_grow to allocate\nexactly the number of characters that are requested. That is if the test\nsuite is any indication.\n\nI ran the test suite and counted how many times git_vsnprintf was called,\nand how many times the return of the first vsnprintf was equal to maxsize-1.\n\n 379056: git_vsnprintf called\n 2954: ret == maxsize-1 happened (meaning possible failure, but possibly\n a buffer exactly the right size)\n\n failure code path .779%\n\nOut of the 379056 times that git_vsnprintf was called during the tests, it\nwas only 2954 of those times that the initial vsnprintf returned maxsize-1.\n\nThat compares to this count on linux (more tests enabled):\n\n 405135: git_vsnprintf called\n 3609: ret >= maxsize (meaning called with buffer too small)\n 68: ret == maxsize-1 happened (buffer exactly the right size)\n\n failure code path .891%\n\nSo, it doesn't seem like it is necessary to complicate this change with more\nMakefile options like SNPRINTF_RETURNS_CRAP and then protect the additions\nbelow with preprocessor tests. But that can be done.\n\n-brandon\n\n\n compat/snprintf.c | 4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/compat/snprintf.c b/compat/snprintf.c\nindex 580966e..357e733 100644\n--- a/compat/snprintf.c\n+++ b/compat/snprintf.c\n@@ -17,6 +17,8 @@ int git_vsnprintf(char *str, size_t maxsize, const char *format, va_list ap)\n \n \tif (maxsize > 0) {\n \t\tret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, ap);\n+\t\tif (ret == maxsize-1)\n+\t\t\tret = -1;\n \t\t/* Windows does not NUL-terminate if result fills buffer */\n \t\tstr[maxsize-1] = 0;\n \t}\n@@ -34,6 +36,8 @@ int git_vsnprintf(char *str, size_t maxsize, const char *format, va_list ap)\n \t\t\tbreak;\n \t\ts = str;\n \t\tret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, ap);\n+\t\tif (ret == maxsize-1)\n+\t\t\tret = -1;\n \t}\n \tfree(s);\n \treturn ret;\n-- \n1.6.0.13.ge1c8\n"},{"id":"87950","messageId":"48AD2207.903@viscovery.net","threadId":"15122","inReplyTo":"nYV6zI97uDEOlIEFjH5oAZW1lTFm0mOVlTrIxzRyOY5CCrUJw7ROgA@cipher.nrlssc.navy.mil","subject":"Re: [PATCH] compat/snprintf.c: handle snprintf's that always return the # chars transmitted","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-08-21T08:06:31Z","receivedAt":"2008-08-21T08:06:31Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Brandon Casey schrieb:\n> Some platforms provide a horribly broken snprintf. More broken than the\n> platforms that return -1 when there is too little space in the target buffer\n> for the formatted string. Some platforms provide an snprintf which _always_\n> returns the number of characters transmitted to the buffer, regardless of\n> whether there was enough space or not.\n...\n> diff --git a/compat/snprintf.c b/compat/snprintf.c\n> index 580966e..357e733 100644\n> --- a/compat/snprintf.c\n> +++ b/compat/snprintf.c\n> @@ -17,6 +17,8 @@ int git_vsnprintf(char *str, size_t maxsize, const char *format, va_list ap)\n> \n> \tif (maxsize > 0) {\n> \t\tret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, ap);\n> +\t\tif (ret == maxsize-1)\n> +\t\t\tret = -1;\n> \t\t/* Windows does not NUL-terminate if result fills buffer */\n> \t\tstr[maxsize-1] = 0;\n> \t}\n> @@ -34,6 +36,8 @@ int git_vsnprintf(char *str, size_t maxsize, const char *format, va_list ap)\n> \t\t\tbreak;\n> \t\ts = str;\n> \t\tret = vsnprintf(str, maxsize-SNPRINTF_SIZE_CORR, format, ap);\n> +\t\tif (ret == maxsize-1)\n> +\t\t\tret = -1;\n> \t}\n> \tfree(s);\n> \treturn ret;\n\nThis looks good, and it passes the test suite on MinGW. Hence:\n\nAcked-by: Johannes Sixt \n\n-- Hannes\n"}]}