From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Thu, 06 Oct 2011 17:32:09 GMT
Subject: Re: [PATCH] mingw: avoid using strbuf in syslog
Message-ID: <CABPQNSYUzmHgFO9VzHRX-or8b5h+HEXapBoiQDWRA_4KWi9nAQ@mail.gmail.com>
In-Reply-To: <CA+sFfMediaCnzFH6uKfVc_Bb+W+AA1nO+OdvB8vWjjrsD1kh9w@mail.gmail.com>

On Thu, Oct 6, 2011 at 7:27 PM, Brandon Casey <drafnel@gmail.com> wrote:
> On Thu, Oct 6, 2011 at 11:57 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
>> strbuf can call die, which again can call syslog from git-daemon.
>>
>> Endless recursion is no fun; fix it by hand-rolling the logic.
>>
>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
>> ---
>> Fixes the problem Brandon pointed out...
>
> Actually, Johannes should get that credit.  He brought up the whole
> recursion issue.
>

OK, I didn't spot that one. But it's not in the commit message,
though. Perhaps it should be?

>>  compat/win32/syslog.c |   26 ++++++++++++++------------
>>  1 files changed, 14 insertions(+), 12 deletions(-)
>>
>> diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
>> index 42b95a9..3b8e2b7 100644
>> --- a/compat/win32/syslog.c
>> +++ b/compat/win32/syslog.c
>> @@ -1,5 +1,4 @@
>>  #include "../../git-compat-util.h"
>> -#include "../../strbuf.h"
>>
>>  static HANDLE ms_eventlog;
>>
>> @@ -16,13 +15,8 @@ void openlog(const char *ident, int logopt, int facility)
>>
>>  void syslog(int priority, const char *fmt, ...)
>>  {
>> -       struct strbuf sb = STRBUF_INIT;
>> -       struct strbuf_expand_dict_entry dict[] = {
>> -               {"1", "% 1"},
>> -               {NULL, NULL}
>> -       };
>>        WORD logtype;
>> -       char *str;
>> +       char *str, *pos;
>>        int str_len;
>>        va_list ap;
>>
>> @@ -39,11 +33,20 @@ void syslog(int priority, const char *fmt, ...)
>>        }
>>
>>        str = malloc(str_len + 1);
>> +       if (!str)
>> +               return; /* no chance to report error */
>> +
>
> Just above this, warning() is used to at least print a message to
> stderr if vsnprintf() fails.  It could be used here, and below when
> realloc fails.
>

Good point, so this should be squashed in:
---

diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
index 3b8e2b7..d015e43 100644
--- a/compat/win32/syslog.c
+++ b/compat/win32/syslog.c
@@ -33,8 +33,10 @@ void syslog(int priority, const char *fmt, ...)
 	}

 	str = malloc(str_len + 1);
-	if (!str)
-		return; /* no chance to report error */
+	if (!str) {
+		warning("malloc failed: '%s'", strerror(errno));
+		return;
+	}

 	va_start(ap, fmt);
 	vsnprintf(str, str_len + 1, fmt, ap);
@@ -42,8 +44,10 @@ void syslog(int priority, const char *fmt, ...)

 	while ((pos = strstr(str, "%1")) != NULL) {
 		str = realloc(str, ++str_len + 1);
-		if (!str)
+		if (!str) {
+			warning("realloc failed: '%s'", strerror(errno));
 			return;
+		}
 		memmove(pos + 2, pos + 1, strlen(pos));
 		pos[1] = ' ';
 	}