threads / patch / 28606

patchmingw: avoid using strbuf in syslog

Subject: [PATCH] mingw: avoid using strbuf in syslog

## tl;dr

3 messages between Oct 6, 2011 and Oct 6, 2011. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Erik Faye-Lund· Oct 6, 2011, 16:57 UTC · lore
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...
 compat/win32/syslog.c |   26 ++++++++++++++------------
 1 files changed, 14 insertions(+), 12 deletions(-)
Show changes to compat/win32/syslog.c +14 −12
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 */
+
 	va_start(ap, fmt);
 	vsnprintf(str, str_len + 1, fmt, ap);
 	va_end(ap);
-	strbuf_expand(&sb, str, strbuf_expand_dict_cb, &dict);
-	free(str);
+
+	while ((pos = strstr(str, "%1")) != NULL) {
+		str = realloc(str, ++str_len + 1);
+		if (!str)
+			return;
+		memmove(pos + 2, pos + 1, strlen(pos));
+		pos[1] = ' ';
+	}
 
 	switch (priority) {
 	case LOG_EMERG:
@@ -66,7 +69,6 @@ void syslog(int priority, const char *fmt, ...)
 	}
 
 	ReportEventA(ms_eventlog, logtype, 0, 0, NULL, 1, 0,
-	    (const char **)&sb.buf, NULL);
-
-	strbuf_release(&sb);
+	    (const char **)&str, NULL);
+	free(str);
 }

-- 
1.7.6.msysgit.0.579.ga3d6f
Brandon Casey· Oct 6, 2011, 17:27 UTC · re: Erik Faye-Lund · lore

Re: [PATCH] mingw: avoid using strbuf in syslog

On Thu, Oct 6, 2011 at 11:57 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
Show 7 quoted lines
> 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.

Show 35 quoted lines
>  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.

Show 16 quoted lines
>        va_start(ap, fmt);
>        vsnprintf(str, str_len + 1, fmt, ap);
>        va_end(ap);
> -       strbuf_expand(&sb, str, strbuf_expand_dict_cb, &dict);
> -       free(str);
> +
> +       while ((pos = strstr(str, "%1")) != NULL) {
> +               str = realloc(str, ++str_len + 1);
> +               if (!str)
> +                       return;
> +               memmove(pos + 2, pos + 1, strlen(pos));
> +               pos[1] = ' ';
> +       }
>
>        switch (priority) {
>        case LOG_EMERG:
-Brandon
Erik Faye-Lund· Oct 6, 2011, 17:32 UTC · re: Brandon Casey · lore

Re: [PATCH] mingw: avoid using strbuf in syslog

On Thu, Oct 6, 2011 at 7:27 PM, Brandon Casey <drafnel@gmail.com> wrote:
Show 12 quoted lines
> 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?

Show 40 quoted lines
>>  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: ---

Show changes to compat/win32/syslog.c +7 −3
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] = ' ';
 	}

← back to recent threads