From: Junio C Hamano <gitster@pobox.com>
To: git mailing list <git@vger.kernel.org>,
Erik Faye-Lund <kusmabite@googlemail.com>
Cc: Arjun Sreedharan <arjun024@gmail.com>, Mike Pape <dotzenlabs@gmail.com>
Subject: Re: [PATCH] win32: syslog: prevent potential realloc memory leak
Date: Mon, 15 Dec 2014 10:11:33 -0800 [thread overview]
Message-ID: <xmqqbnn4eqay.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1418463676-1753-1-git-send-email-arjun024@gmail.com> (Arjun Sreedharan's message of "Sat, 13 Dec 2014 15:11:16 +0530")
Arjun Sreedharan <arjun024@gmail.com> writes:
> use a temporary variable to free the memory in case
> realloc() fails.
>
> Signed-off-by: Arjun Sreedharan <arjun024@gmail.com>
> ---
> compat/win32/syslog.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
> index d015e43..3409e43 100644
> --- a/compat/win32/syslog.c
> +++ b/compat/win32/syslog.c
> @@ -16,7 +16,7 @@ void openlog(const char *ident, int logopt, int facility)
> void syslog(int priority, const char *fmt, ...)
> {
> WORD logtype;
> - char *str, *pos;
> + char *str, *str_temp, *pos;
> int str_len;
> va_list ap;
>
> @@ -43,9 +43,11 @@ void syslog(int priority, const char *fmt, ...)
> va_end(ap);
>
> while ((pos = strstr(str, "%1")) != NULL) {
> + str_temp = str;
> str = realloc(str, ++str_len + 1);
> if (!str) {
> warning("realloc failed: '%s'", strerror(errno));
> + free(str_temp);
> return;
> }
Hmm, the original, 088d8802 (mingw: implement syslog, 2010-11-04),
that introduced the special casing for %1, says:
Syslog does not usually exist on Windows, so implement our own
using Window's ReportEvent mechanism.
Strings containing "%1" gets expanded into them selves by
ReportEvent, resulting in an unreadable string. "%2" and above
is not a problem. Unfortunately, on Windows an IPv6 address can
contain "%1", so expand "%1" to "% 1" before reporting. "%%1" is
also a problem for ReportEvent, but that string cannot occur in
an IPv6 address.
It is unclear why it says '"%2" and above is not a problem' to me.
Is that because they expand to something not "an unreadable string",
or is that because in the original developer's testing only "%1" was
observed? It also says "%%1" is a problem, and it does not occur in
an IPv6 address, but that would suggest that every time a new caller
is added to syslog(), this imitation of syslog() can break, as there
is nothing that says the new caller must be reporting something
about an IP address. Perhaps this loop should cleanse what it
passes to ReportEvent() a bit more aggressively by expanding all "%"
to "%-sp" or something)?
Regardless of that funny %1 business, I notice in
http://msdn.microsoft.com/en-us/library/windows/desktop/aa363679%28v=vs.85%29.aspx
that each element of lpStrings array that is passed to ReportEvent()
is limited to 32k or so. Wouldn't it make it a lot simpler if we
removed the dynamic allocation and use a fixed sized 32k buffer here
(and truncate the result as necessary)? That would make the "leak"
disappear automatically.
Having said all that, if we were to still go with the current code
structure, "str_temp" should be scoped inside the loop, as there is
no need to make it available to the remainder of the function, I
think. Also writing this way may make the intention more clear.
while (...) {
char *new_str = realloc(str, ...);
if (!new_str) {
free(str);
return;
}
memmove(... to shuffle ...);
And after starting to write the above, I notice that the current
code around realloc may be completely bogus. It goes like this:
while ((pos = strstr(str, "..."))) {
str = realloc(str, ...);
if (!str) { warn and bail; }
memmove(pos + 1, pos + 1, ...);
pos[1] = ' ';
}
If realloc() really allocated a new string, then pos that points
into the original str has no relation to the reallocated str, so
memmove() is not shuffling the string to make room for the SP in the
string that will be given to ReportEvent() at all, no? This seems
to be a bug introduced by 2a6b149c (mingw: avoid using strbuf in
syslog, 2011-10-06).
It makes me wonder if this codepath ever triggers in the first
place.
next prev parent reply other threads:[~2014-12-15 18:11 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-12-13 9:41 [PATCH] win32: syslog: prevent potential realloc memory leak Arjun Sreedharan
2014-12-15 18:11 ` Junio C Hamano [this message]
2015-01-25 21:38 ` Erik Faye-Lund
2015-01-26 4:25 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqbnn4eqay.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox.com \
--cc=arjun024@gmail.com \
--cc=dotzenlabs@gmail.com \
--cc=git@vger.kernel.org \
--cc=kusmabite@googlemail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.