Re: [msysGit] [PATCH v3 02/14] mingw: implement syslog

From

Erik Faye-Lund <kusmabite@gmail.com>

Date

Oct 11, 2010, 15:59 UTC

Message-ID

<AANLkTikXMCgC0P-=255SRvqfDSGua7Gcb-jmqS8paUNj@mail.gmail.com>

In-Reply-To

<AANLkTi==sd=jm9LZ6p6NLVBMc4wnU0PrYvJY6ezGWiWt@mail.gmail.com>

On Mon, Oct 11, 2010 at 5:28 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:

> On Mon, Oct 11, 2010 at 1:20 AM, Eric Sunshine <ericsunshine@gmail.com> wrote:
>> On 10/10/2010 6:16 PM, Erik Faye-Lund wrote:
>>>
>>> On Sun, Oct 10, 2010 at 11:28 PM, Eric Sunshine<ericsunshine@gmail.com>
>>>  wrote:
>>>> (On the other hand, for the '%s' check above, the code does report a
>>>> warning
>>>> and then exits, so it is not inconceivable that a '%n' could also emit a
>>>> warning.)
>>>
>>> I guess I could add something like this:
>>>
>>> if (strstr(arg, "%1"))
>>>        warning("arg contains %1, message might be corrupted");
>>>
>>> I don't want to return in that case, because I think some output is
>>> better than no output, and it seems to work on Vista.
>>
>> Rather than emitting a warning, it might be reasonable to perform a simple
>> transformation on the string if it contains a %1 (or %n generally) in order
>> to avoid ReportEvent()'s shortcoming. Even something as simple as inserting
>> a space between '%' and '1' might be sufficiently defensive.
>>
>
> Yes, but I'm tempted to defer fixing this until we see that it's a
> problem in reality. The logic to somehow escape such sequences looks a
> bit nasty in my head. But perhaps strbuf_expand() is the right hammer
> for this use...
>
> Then the logical next question becomes what we should expand it to.
> Does "%1" -> "% 1" make sense for IPv6 addresses?
>

I also added a +1 that was missing and caused the string to be capped.

diff --git a/compat/mingw.c b/compat/mingw.c
index ae6b448..d1444d2 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1438,6 +1438,11 @@ 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;
 	int str_len;
@@ -1457,8 +1462,9 @@ void syslog(int priority, const char *fmt, ...)

 	str = malloc(str_len + 1);
 	va_start(ap, fmt);
-	vsnprintf(str, str_len, fmt, ap);
+	vsnprintf(str, str_len + 1, fmt, ap);
 	va_end(ap);
+	strbuf_expand(&sb, str, strbuf_expand_dict_cb, &dict);

 	switch (priority) {
 	case LOG_EMERG:
@@ -1480,10 +1486,6 @@ void syslog(int priority, const char *fmt, ...)
 		break;
 	}

-	/*
-	 * FIXME: ReportEvent() doesn't handle strings containing "%1".
-	 * Such events must currently be reformatted by the caller.
-	 */
 	ReportEventA(ms_eventlog,
 	    logtype,
 	    0,
@@ -1491,9 +1493,10 @@ void syslog(int priority, const char *fmt, ...)
 	    NULL,
 	    1,
 	    0,
-	    (const char **)&str,
+	    (const char **)&sb.buf,
 	    NULL);
 	free(str);
+	sb_release(&sb);
 }

 #undef signal
diff --git a/daemon.c b/daemon.c