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

On Mon, Oct 11, 2010 at 5:28 PM, Erik Faye-Lund [off-list ref] wrote:

On Mon, Oct 11, 2010 at 1:20 AM, Eric Sunshine [off-list ref] wrote:

quoted

On 10/10/2010 6:16 PM, Erik Faye-Lund wrote:

quoted

On Sun, Oct 10, 2010 at 11:28 PM, Eric Sunshine[off-list ref]
 wrote:

quoted

(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