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

[Erik Faye-Lund <kusmabite@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?
>

Something along these lines? (on top of the previous patch, uhm, with
some local modifications. Sorry, I'm not at home and do not have the
original version at hand. I'm sure you get the picture, though...)

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