{"thread":{"id":"38175","subject":"[PATCH] win32: syslog: prevent potential realloc memory leak","startedAt":"2014-12-13T09:41:16Z","lastAt":"2015-01-26T04:25:29Z","messageCount":4,"participants":["Arjun Sreedharan","Junio C Hamano","Erik Faye-Lund"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"253677","messageId":"1418463676-1753-1-git-send-email-arjun024@gmail.com","threadId":"38175","inReplyTo":null,"subject":"[PATCH] win32: syslog: prevent potential realloc memory leak","fromName":"Arjun Sreedharan","fromEmail":"arjun024@gmail.com","sentAt":"2014-12-13T09:41:16Z","receivedAt":"2014-12-13T09:41:16Z","isPatch":true,"sender":{"key":"arjun024@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2861611?v=4"},"body":"use a temporary variable to free the memory in case\nrealloc() fails.\n\nSigned-off-by: Arjun Sreedharan <arjun024@gmail.com>\n---\n compat/win32/syslog.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/compat/win32/syslog.c b/compat/win32/syslog.c\nindex d015e43..3409e43 100644\n--- a/compat/win32/syslog.c\n+++ b/compat/win32/syslog.c\n@@ -16,7 +16,7 @@ void openlog(const char *ident, int logopt, int facility)\n void syslog(int priority, const char *fmt, ...)\n {\n \tWORD logtype;\n-\tchar *str, *pos;\n+\tchar *str, *str_temp, *pos;\n \tint str_len;\n \tva_list ap;\n \n@@ -43,9 +43,11 @@ void syslog(int priority, const char *fmt, ...)\n \tva_end(ap);\n \n \twhile ((pos = strstr(str, \"%1\")) != NULL) {\n+\t\tstr_temp = str;\n \t\tstr = realloc(str, ++str_len + 1);\n \t\tif (!str) {\n \t\t\twarning(\"realloc failed: '%s'\", strerror(errno));\n+\t\t\tfree(str_temp);\n \t\t\treturn;\n \t\t}\n \t\tmemmove(pos + 2, pos + 1, strlen(pos));\n-- \n1.8.1.msysgit.1\n"},{"id":"253695","messageId":"xmqqbnn4eqay.fsf@gitster.dls.corp.google.com","threadId":"38175","inReplyTo":"1418463676-1753-1-git-send-email-arjun024@gmail.com","subject":"Re: [PATCH] win32: syslog: prevent potential realloc memory leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-15T18:11:33Z","receivedAt":"2014-12-15T18:11:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Arjun Sreedharan <arjun024@gmail.com> writes:\n\n> use a temporary variable to free the memory in case\n> realloc() fails.\n>\n> Signed-off-by: Arjun Sreedharan <arjun024@gmail.com>\n> ---\n>  compat/win32/syslog.c | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c\n> index d015e43..3409e43 100644\n> --- a/compat/win32/syslog.c\n> +++ b/compat/win32/syslog.c\n> @@ -16,7 +16,7 @@ void openlog(const char *ident, int logopt, int facility)\n>  void syslog(int priority, const char *fmt, ...)\n>  {\n>  \tWORD logtype;\n> -\tchar *str, *pos;\n> +\tchar *str, *str_temp, *pos;\n>  \tint str_len;\n>  \tva_list ap;\n>  \n> @@ -43,9 +43,11 @@ void syslog(int priority, const char *fmt, ...)\n>  \tva_end(ap);\n>  \n>  \twhile ((pos = strstr(str, \"%1\")) != NULL) {\n> +\t\tstr_temp = str;\n>  \t\tstr = realloc(str, ++str_len + 1);\n>  \t\tif (!str) {\n>  \t\t\twarning(\"realloc failed: '%s'\", strerror(errno));\n> +\t\t\tfree(str_temp);\n>  \t\t\treturn;\n>  \t\t}\n\nHmm, the original, 088d8802 (mingw: implement syslog, 2010-11-04),\nthat introduced the special casing for %1, says:\n\n    Syslog does not usually exist on Windows, so implement our own\n    using Window's ReportEvent mechanism.\n\n    Strings containing \"%1\" gets expanded into them selves by\n    ReportEvent, resulting in an unreadable string. \"%2\" and above\n    is not a problem.  Unfortunately, on Windows an IPv6 address can\n    contain \"%1\", so expand \"%1\" to \"% 1\" before reporting. \"%%1\" is\n    also a problem for ReportEvent, but that string cannot occur in\n    an IPv6 address.\n\nIt is unclear why it says '\"%2\" and above is not a problem' to me.\nIs that because they expand to something not \"an unreadable string\",\nor is that because in the original developer's testing only \"%1\" was\nobserved?  It also says \"%%1\" is a problem, and it does not occur in\nan IPv6 address, but that would suggest that every time a new caller\nis added to syslog(), this imitation of syslog() can break, as there\nis nothing that says the new caller must be reporting something\nabout an IP address.  Perhaps this loop should cleanse what it\npasses to ReportEvent() a bit more aggressively by expanding all \"%\"\nto \"%-sp\" or something)?\n\nRegardless of that funny %1 business, I notice in\n\n    http://msdn.microsoft.com/en-us/library/windows/desktop/aa363679%28v=vs.85%29.aspx\n\nthat each element of lpStrings array that is passed to ReportEvent()\nis limited to 32k or so.  Wouldn't it make it a lot simpler if we\nremoved the dynamic allocation and use a fixed sized 32k buffer here\n(and truncate the result as necessary)?  That would make the \"leak\"\ndisappear automatically.\n\nHaving said all that, if we were to still go with the current code\nstructure, \"str_temp\" should be scoped inside the loop, as there is\nno need to make it available to the remainder of the function, I\nthink.  Also writing this way may make the intention more clear.\n\n\twhile (...) {\n\t\tchar *new_str = realloc(str, ...);\n                if (!new_str) {\n                \tfree(str);\n                        return;\n\t\t}\n\t\tmemmove(... to shuffle ...);\n\nAnd after starting to write the above, I notice that the current\ncode around realloc may be completely bogus.  It goes like this:\n\n\twhile ((pos = strstr(str, \"...\"))) {\n        \tstr = realloc(str, ...);\n                if (!str) { warn and bail; }\n                memmove(pos + 1, pos + 1, ...);\n\t\tpos[1] = ' ';\n\t}\n\nIf realloc() really allocated a new string, then pos that points\ninto the original str has no relation to the reallocated str, so\nmemmove() is not shuffling the string to make room for the SP in the\nstring that will be given to ReportEvent() at all, no?  This seems\nto be a bug introduced by 2a6b149c (mingw: avoid using strbuf in\nsyslog, 2011-10-06).\n\nIt makes me wonder if this codepath ever triggers in the first\nplace.\n"},{"id":"255281","messageId":"CABPQNSbCF7P23K+uj00PmjJkr=19t=W5wCN0-jweBJYEWgUWRw@mail.gmail.com","threadId":"38175","inReplyTo":"xmqqbnn4eqay.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] win32: syslog: prevent potential realloc memory leak","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2015-01-25T21:38:39Z","receivedAt":"2015-01-25T21:38:39Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"Sorry for very late reply. I had a bug in my mail rules that caused\nthis email to skip my inbox. That should be fixed now.\n\nOn Mon, Dec 15, 2014 at 7:11 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Arjun Sreedharan <arjun024@gmail.com> writes:\n>\n>> use a temporary variable to free the memory in case\n>> realloc() fails.\n>>\n>> Signed-off-by: Arjun Sreedharan <arjun024@gmail.com>\n>> ---\n>>  compat/win32/syslog.c | 4 +++-\n>>  1 file changed, 3 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c\n>> index d015e43..3409e43 100644\n>> --- a/compat/win32/syslog.c\n>> +++ b/compat/win32/syslog.c\n>> @@ -16,7 +16,7 @@ void openlog(const char *ident, int logopt, int facility)\n>>  void syslog(int priority, const char *fmt, ...)\n>>  {\n>>       WORD logtype;\n>> -     char *str, *pos;\n>> +     char *str, *str_temp, *pos;\n>>       int str_len;\n>>       va_list ap;\n>>\n>> @@ -43,9 +43,11 @@ void syslog(int priority, const char *fmt, ...)\n>>       va_end(ap);\n>>\n>>       while ((pos = strstr(str, \"%1\")) != NULL) {\n>> +             str_temp = str;\n>>               str = realloc(str, ++str_len + 1);\n>>               if (!str) {\n>>                       warning(\"realloc failed: '%s'\", strerror(errno));\n>> +                     free(str_temp);\n>>                       return;\n>>               }\n>\n> Hmm, the original, 088d8802 (mingw: implement syslog, 2010-11-04),\n> that introduced the special casing for %1, says:\n>\n>     Syslog does not usually exist on Windows, so implement our own\n>     using Window's ReportEvent mechanism.\n>\n>     Strings containing \"%1\" gets expanded into them selves by\n>     ReportEvent, resulting in an unreadable string. \"%2\" and above\n>     is not a problem.  Unfortunately, on Windows an IPv6 address can\n>     contain \"%1\", so expand \"%1\" to \"% 1\" before reporting. \"%%1\" is\n>     also a problem for ReportEvent, but that string cannot occur in\n>     an IPv6 address.\n>\n> It is unclear why it says '\"%2\" and above is not a problem' to me.\n> Is that because they expand to something not \"an unreadable string\",\n> or is that because in the original developer's testing only \"%1\" was\n> observed?\n\n%2 would expand to the first argument (this is a varargs-type\ninterface), but such an argument does not exist, so no expansion is\nbeing performed.\n\n> It also says \"%%1\" is a problem, and it does not occur in\n> an IPv6 address, but that would suggest that every time a new caller\n> is added to syslog(), this imitation of syslog() can break, as there\n> is nothing that says the new caller must be reporting something\n> about an IP address.  Perhaps this loop should cleanse what it\n> passes to ReportEvent() a bit more aggressively by expanding all \"%\"\n> to \"%-sp\" or something)?\n\nThis suspicion is somewhat justified; I only did %1 because that was\nthe only one that had any theoretical way of being hit at the time the\npatch was written. However, a quick test reveals that we currently\nexpand \"%%1\" to \"%% 1\", which is not problematic. And that makes sense\nfrom looking at the code also.\n\nHowever, removing the hunk and running the code reveals that Windows\nno longer does that ugly recursive expansion. I'm sure it did when I\nwrote the code, because I did test it. So it seems at least Windows\n8.1 is less insane about this.\n\n> Regardless of that funny %1 business, I notice in\n>\n>     http://msdn.microsoft.com/en-us/library/windows/desktop/aa363679%28v=vs.85%29.aspx\n>\n> that each element of lpStrings array that is passed to ReportEvent()\n> is limited to 32k or so.  Wouldn't it make it a lot simpler if we\n> removed the dynamic allocation and use a fixed sized 32k buffer here\n> (and truncate the result as necessary)?  That would make the \"leak\"\n> disappear automatically.\n\nThat's a very good point. Yes, I think that makes more sense.\n\n> Having said all that, if we were to still go with the current code\n> structure, \"str_temp\" should be scoped inside the loop, as there is\n> no need to make it available to the remainder of the function, I\n> think.  Also writing this way may make the intention more clear.\n>\n>         while (...) {\n>                 char *new_str = realloc(str, ...);\n>                 if (!new_str) {\n>                         free(str);\n>                         return;\n>                 }\n>                 memmove(... to shuffle ...);\n>\n> And after starting to write the above, I notice that the current\n> code around realloc may be completely bogus.  It goes like this:\n>\n>         while ((pos = strstr(str, \"...\"))) {\n>                 str = realloc(str, ...);\n>                 if (!str) { warn and bail; }\n>                 memmove(pos + 1, pos + 1, ...);\n>                 pos[1] = ' ';\n>         }\n>\n> If realloc() really allocated a new string, then pos that points\n> into the original str has no relation to the reallocated str, so\n> memmove() is not shuffling the string to make room for the SP in the\n> string that will be given to ReportEvent() at all, no?  This seems\n> to be a bug introduced by 2a6b149c (mingw: avoid using strbuf in\n> syslog, 2011-10-06).\n>\n> It makes me wonder if this codepath ever triggers in the first\n> place.\n\nIIRC, this hunk was really just written out of fear because IPv6\naddresses might contain %1, and I don't think it has ever been proven\nto be possible to trigger out in the wild. But of course, other\ncall-sites could do the same thing.\n"},{"id":"255283","messageId":"xmqqzj96mame.fsf@gitster.dls.corp.google.com","threadId":"38175","inReplyTo":"CABPQNSbCF7P23K+uj00PmjJkr=19t=W5wCN0-jweBJYEWgUWRw@mail.gmail.com","subject":"Re: [PATCH] win32: syslog: prevent potential realloc memory leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-26T04:25:29Z","receivedAt":"2015-01-26T04:25:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund <kusmabite@gmail.com> writes:\n\n> Sorry for very late reply. I had a bug in my mail rules that caused\n> this email to skip my inbox. That should be fixed now.\n>\n> On Mon, Dec 15, 2014 at 7:11 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> ...\n>> Regardless of that funny %1 business, I notice in\n>>\n>>     http://msdn.microsoft.com/en-us/library/windows/desktop/aa363679%28v=vs.85%29.aspx\n>>\n>> that each element of lpStrings array that is passed to ReportEvent()\n>> is limited to 32k or so.  Wouldn't it make it a lot simpler if we\n>> removed the dynamic allocation and use a fixed sized 32k buffer here\n>> (and truncate the result as necessary)?  That would make the \"leak\"\n>> disappear automatically.\n>\n> That's a very good point. Yes, I think that makes more sense.\n\nOK, so I'd expect a simpler non-reallocating code to materialize\nand will drop Arjun's patch for now.\n\nThanks.\n"}]}