git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] mingw: abort on invalid strftime formats
@ 2018-03-19 16:49 Johannes Schindelin
  2018-03-19 17:52 ` Junio C Hamano
  0 siblings, 1 reply; 3+ messages in thread
From: Johannes Schindelin @ 2018-03-19 16:49 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano

On Windows, strftime() does not silently ignore invalid formats, but
warns about them and then returns 0 and sets errno to EINVAL.

Unfortunately, Git does not expect such a behavior, as it disagrees
with strftime()'s semantics on Linux. As a consequence, Git
misinterprets the return value 0 as "I need more space" and grows the
buffer. As the larger buffer does not fix the format, the buffer grows
and grows and grows until we are out of memory and abort.

Ideally, we would switch off the parameter validation just for
strftime(), but we cannot even override the invalid parameter handler
via _set_thread_local_invalid_parameter_handler() using MINGW because
that function is not declared. Even _set_invalid_parameter_handler(),
which *is* declared, does not help, as it simply does... nothing.

So let's just bite the bullet and override strftime() for MINGW and
abort on an invalid format string. While this does not provide the
best user experience, it is the best we can do.

See https://msdn.microsoft.com/en-us/library/fe06s4ak.aspx for more
details.

This fixes https://github.com/git-for-windows/git/issues/863

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	This is a really old patch (from 2016) that I had not managed to
	contribute to git.git yet...

 compat/mingw.c | 11 +++++++++++
 compat/mingw.h |  3 +++
 2 files changed, 14 insertions(+)

diff --git a/compat/mingw.c b/compat/mingw.c
index 2d44d21aca8..a67872babf3 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -761,6 +761,17 @@ int mingw_utime (const char *file_name, const struct utimbuf *times)
 	return rc;
 }
 
+#undef strftime
+size_t mingw_strftime(char *s, size_t max,
+		      const char *format, const struct tm *tm)
+{
+	size_t ret = strftime(s, max, format, tm);
+
+	if (!ret && errno == EINVAL)
+		die("invalid strftime format: '%s'", format);
+	return ret;
+}
+
 unsigned int sleep (unsigned int seconds)
 {
 	Sleep(seconds*1000);
diff --git a/compat/mingw.h b/compat/mingw.h
index e03aecfe2e6..571019d0bdd 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -361,6 +361,9 @@ int mingw_fstat(int fd, struct stat *buf);
 
 int mingw_utime(const char *file_name, const struct utimbuf *times);
 #define utime mingw_utime
+size_t mingw_strftime(char *s, size_t max,
+		   const char *format, const struct tm *tm);
+#define strftime mingw_strftime
 
 pid_t mingw_spawnvpe(const char *cmd, const char **argv, char **env,
 		     const char *dir,

base-commit: 0afbf6caa5b16dcfa3074982e5b48e27d452dbbb
-- 
2.16.1.windows.4

Published-As: https://github.com/dscho/git/releases/tag/mingw-strftime-v1
Fetch-It-Via: git fetch https://github.com/dscho/git mingw-strftime-v1

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] mingw: abort on invalid strftime formats
  2018-03-19 16:49 [PATCH] mingw: abort on invalid strftime formats Johannes Schindelin
@ 2018-03-19 17:52 ` Junio C Hamano
  2018-03-20 13:57   ` Johannes Schindelin
  0 siblings, 1 reply; 3+ messages in thread
From: Junio C Hamano @ 2018-03-19 17:52 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> On Windows, strftime() does not silently ignore invalid formats, but
> warns about them and then returns 0 and sets errno to EINVAL.
>
> Unfortunately, Git does not expect such a behavior, as it disagrees
> with strftime()'s semantics on Linux. As a consequence, Git
> misinterprets the return value 0 as "I need more space" and grows the
> buffer. As the larger buffer does not fix the format, the buffer grows
> and grows and grows until we are out of memory and abort.

Yuck, it is bad that the callers assume 0 is always "need more
space", as "If a conversion specification does not correspond to any
of the above, the behavior is undefined." is what we get from POSIX.

> So let's just bite the bullet and override strftime() for MINGW and
> abort on an invalid format string. While this does not provide the
> best user experience, it is the best we can do.

As long as we allow arbitrary end-user input passed to strftime(),
this is probably a step in the right direction.  

As to the future direction, I however wonder if we can return an
error from here and make the caller cooperate a bit more.  Of
course, implementation of strftime() that silently ignore invalid
formats would not be able to return correct errors like an updated
mingw_strftime() that does not die but communicates error to its
caller would, though.

My "git grep" is hitting only one caller of strftime(), which is
strbuf_addftime(), which already does some magic to the format
string, so such a future enhancement may not be _so_ bad.

Will apply, thanks.  I do not think there is no reason to cook this
in 'next', and would assume this can instead go directly to 'master'
to be part of v2.17-rc1 and onward, right?

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] mingw: abort on invalid strftime formats
  2018-03-19 17:52 ` Junio C Hamano
@ 2018-03-20 13:57   ` Johannes Schindelin
  0 siblings, 0 replies; 3+ messages in thread
From: Johannes Schindelin @ 2018-03-20 13:57 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

Hi Junio,

On Mon, 19 Mar 2018, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > On Windows, strftime() does not silently ignore invalid formats, but
> > warns about them and then returns 0 and sets errno to EINVAL.
> >
> > Unfortunately, Git does not expect such a behavior, as it disagrees
> > with strftime()'s semantics on Linux. As a consequence, Git
> > misinterprets the return value 0 as "I need more space" and grows the
> > buffer. As the larger buffer does not fix the format, the buffer grows
> > and grows and grows until we are out of memory and abort.
> 
> Yuck, it is bad that the callers assume 0 is always "need more space",
> as "If a conversion specification does not correspond to any of the
> above, the behavior is undefined." is what we get from POSIX.

Yeah, well, our own rules state that we are sometimes stricter than POSIX
when it is pragmatic. This is one of those cases, methinks.

> > So let's just bite the bullet and override strftime() for MINGW and
> > abort on an invalid format string. While this does not provide the
> > best user experience, it is the best we can do.
> 
> As long as we allow arbitrary end-user input passed to strftime(), this
> is probably a step in the right direction.  
> 
> As to the future direction, I however wonder if we can return an error
> from here and make the caller cooperate a bit more.  Of course,
> implementation of strftime() that silently ignore invalid formats would
> not be able to return correct errors like an updated mingw_strftime()
> that does not die but communicates error to its caller would, though.

And I would not know what the caller should do in that case, quite
honestly... Would strftime() somehow tell us *which* placeholder it took
offense with? And would we "quote" that?

> My "git grep" is hitting only one caller of strftime(), which is
> strbuf_addftime(), which already does some magic to the format
> string, so such a future enhancement may not be _so_ bad.

Right, except that I do not think I could get the exact error condition
necessary to know *how* to munge the format further, not unless I invest a
serious amount of work in it ;-)

> Will apply, thanks.  I do not think there is no reason to cook this
> in 'next', and would assume this can instead go directly to 'master'
> to be part of v2.17-rc1 and onward, right?

Thanks. Given that this patch has been in Git for Windows for quite a
while without problems, I think it is safe to get it directly into
`master`. FWIW this late in the -rc phase, I would be very reluctant to
send anything I deem unworthy of `master` anyway.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2018-03-20 13:58 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-19 16:49 [PATCH] mingw: abort on invalid strftime formats Johannes Schindelin
2018-03-19 17:52 ` Junio C Hamano
2018-03-20 13:57   ` Johannes Schindelin

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git