[PATCH] Handle localtime errors in ctime

[PATCH] Handle localtime errors in ctime

[Omer Anson]

ctime passes the result from localtime directly to asctime. But in case
of error, localtime returns 0. This causes an error (NULL pointer
dereference) in asctime.

According to the man pages [1], ctime should also return 0 upon error.
Therefore, if localtime fails (return 0), ctime also returns 0.

[1] https://linux.die.net/man/3/ctime
---
 src/time/ctime.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/time/ctime.c b/src/time/ctime.c
index 185ec55..6955f2d 100644
--- a/src/time/ctime.c
+++ b/src/time/ctime.c
@@ -2,5 +2,9 @@
 
 char *ctime(const time_t *t)
 {
-	return asctime(localtime(t));
+	struct tm * tm = localtime(t);
+	if (!tm) {
+		return 0;
+	}
+	return asctime(tm);
 }
-- 
2.4.11

Re: [PATCH] Handle localtime errors in ctime

[Rich Felker]

On Thu, Jun 15, 2017 at 07:43:36PM +0300, Omer Anson wrote:
> ctime passes the result from localtime directly to asctime. But in case
> of error, localtime returns 0. This causes an error (NULL pointer
> dereference) in asctime.
> 
> According to the man pages [1], ctime should also return 0 upon error.
> Therefore, if localtime fails (return 0), ctime also returns 0.
> 
> [1] https://linux.die.net/man/3/ctime
> 
> ---
>  src/time/ctime.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/time/ctime.c b/src/time/ctime.c
> index 185ec55..6955f2d 100644
> --- a/src/time/ctime.c
> +++ b/src/time/ctime.c
> @@ -2,5 +2,9 @@
>  
>  char *ctime(const time_t *t)
>  {
> -	return asctime(localtime(t));
> +	struct tm * tm = localtime(t);
> +	if (!tm) {
> +		return 0;
> +	}
> +	return asctime(tm);
>  }
> -- 
> 2.4.11

Content looks good. Making minor edits for style and I'll commit.

Also please don't link to linux.die.net; the standard (C or POSIX),
not the man page, is authoritative for things like this, and
linux.die.net is pretty much just SEO spam (using outdated versions of
scraped content, i.e. man pages, to boost pagerank).

Rich

Re: [PATCH] Handle localtime errors in ctime

[Alexander Monakov]

On Thu, 15 Jun 2017, Rich Felker wrote:
> On Thu, Jun 15, 2017 at 07:43:36PM +0300, Omer Anson wrote:
> > ctime passes the result from localtime directly to asctime. But in case
> > of error, localtime returns 0. This causes an error (NULL pointer
> > dereference) in asctime.
> > 
> > According to the man pages [1], ctime should also return 0 upon error.
> > Therefore, if localtime fails (return 0), ctime also returns 0.
> > 
> > [1] https://linux.die.net/man/3/ctime

> Content looks good. Making minor edits for style and I'll commit.

Um, the previous time an opposite direction was taken:
http://www.openwall.com/lists/musl/2014/09/05/17

Alexander

Re: [PATCH] Handle localtime errors in ctime

[Rich Felker]

On Thu, Jun 15, 2017 at 08:03:30PM +0300, Alexander Monakov wrote:
> On Thu, 15 Jun 2017, Rich Felker wrote:
> > On Thu, Jun 15, 2017 at 07:43:36PM +0300, Omer Anson wrote:
> > > ctime passes the result from localtime directly to asctime. But in case
> > > of error, localtime returns 0. This causes an error (NULL pointer
> > > dereference) in asctime.
> > > 
> > > According to the man pages [1], ctime should also return 0 upon error.
> > > Therefore, if localtime fails (return 0), ctime also returns 0.
> > > 
> > > [1] https://linux.die.net/man/3/ctime
> 
> > Content looks good. Making minor edits for style and I'll commit.
> 
> Um, the previous time an opposite direction was taken:
> http://www.openwall.com/lists/musl/2014/09/05/17

I found this in POSIX while reviewing the new patch:

[CX] [Option Start] Upon successful completion, ctime_r() shall return
a pointer to the string pointed to by buf. When an error is
encountered, a null pointer shall be returned. [Option End]

So while ISO C may not have anything to say about it (i.e. it's UB in
plain C), POSIX does seem to require handling the error. I forgot we'd
looked at this before but it seems we missed what POSIX had to say.

Rich

Re: [PATCH] Handle localtime errors in ctime

[Alexander Monakov]

On Thu, 15 Jun 2017, Rich Felker wrote:
> > Um, the previous time an opposite direction was taken:
> > http://www.openwall.com/lists/musl/2014/09/05/17
> 
> I found this in POSIX while reviewing the new patch:
> 
> [CX] [Option Start] Upon successful completion, ctime_r() shall return
> a pointer to the string pointed to by buf. When an error is
> encountered, a null pointer shall be returned. [Option End]
> 
> So while ISO C may not have anything to say about it (i.e. it's UB in
> plain C), POSIX does seem to require handling the error. I forgot we'd
> looked at this before but it seems we missed what POSIX had to say.

... even though the quoted POSIX spec specifically mentions ctime_r, not ctime?
(musl implementation of ctime_r does appear to miss that, though)

Alexander

Re: [PATCH] Handle localtime errors in ctime

[Rich Felker]

On Thu, Jun 15, 2017 at 08:19:29PM +0300, Alexander Monakov wrote:
> On Thu, 15 Jun 2017, Rich Felker wrote:
> > > Um, the previous time an opposite direction was taken:
> > > http://www.openwall.com/lists/musl/2014/09/05/17
> > 
> > I found this in POSIX while reviewing the new patch:
> > 
> > [CX] [Option Start] Upon successful completion, ctime_r() shall return
> > a pointer to the string pointed to by buf. When an error is
> > encountered, a null pointer shall be returned. [Option End]
> > 
> > So while ISO C may not have anything to say about it (i.e. it's UB in
> > plain C), POSIX does seem to require handling the error. I forgot we'd
> > looked at this before but it seems we missed what POSIX had to say.
> 
> .... even though the quoted POSIX spec specifically mentions ctime_r, not ctime?
> (musl implementation of ctime_r does appear to miss that, though)

Doh, I misread. Sorry. I'll go fix that now...

Rich

Re: [PATCH] Handle localtime errors in ctime

[Rich Felker]

On Thu, Jun 15, 2017 at 01:26:18PM -0400, Rich Felker wrote:
> On Thu, Jun 15, 2017 at 08:19:29PM +0300, Alexander Monakov wrote:
> > On Thu, 15 Jun 2017, Rich Felker wrote:
> > > > Um, the previous time an opposite direction was taken:
> > > > http://www.openwall.com/lists/musl/2014/09/05/17
> > > 
> > > I found this in POSIX while reviewing the new patch:
> > > 
> > > [CX] [Option Start] Upon successful completion, ctime_r() shall return
> > > a pointer to the string pointed to by buf. When an error is
> > > encountered, a null pointer shall be returned. [Option End]
> > > 
> > > So while ISO C may not have anything to say about it (i.e. it's UB in
> > > plain C), POSIX does seem to require handling the error. I forgot we'd
> > > looked at this before but it seems we missed what POSIX had to say.
> > 
> > .... even though the quoted POSIX spec specifically mentions ctime_r, not ctime?
> > (musl implementation of ctime_r does appear to miss that, though)
> 
> Doh, I misread. Sorry. I'll go fix that now...

Look ok?

diff --git a/src/time/ctime_r.c b/src/time/ctime_r.c
index d2260a1..05699ca 100644
--- a/src/time/ctime_r.c
+++ b/src/time/ctime_r.c
@@ -3,6 +3,5 @@
 char *ctime_r(const time_t *t, char *buf)
 {
 	struct tm tm;
-	localtime_r(t, &tm);
-	return asctime_r(&tm, buf);
+	return localtime_r(t, &tm) ? asctime_r(&tm, buf) : 0;
 }

Rich

Re: [PATCH] Handle localtime errors in ctime

[Alexander Monakov]

On Thu, 15 Jun 2017, Rich Felker wrote:
> Look ok?
> 
> diff --git a/src/time/ctime_r.c b/src/time/ctime_r.c
> index d2260a1..05699ca 100644
> --- a/src/time/ctime_r.c
> +++ b/src/time/ctime_r.c
> @@ -3,6 +3,5 @@
>  char *ctime_r(const time_t *t, char *buf)
>  {
>  	struct tm tm;
> -	localtime_r(t, &tm);
> -	return asctime_r(&tm, buf);
> +	return localtime_r(t, &tm) ? asctime_r(&tm, buf) : 0;
>  }

Sure, although personally I'm highly tempted to pick up the size optimization
from reusing return value of localtime_r:

Alexander

diff --git a/src/time/ctime_r.c b/src/time/ctime_r.c
index d2260a16..9047b38f 100644
--- a/src/time/ctime_r.c
+++ b/src/time/ctime_r.c
@@ -2,7 +2,6 @@
 
 char *ctime_r(const time_t *t, char *buf)
 {
-	struct tm tm;
-	localtime_r(t, &tm);
-	return asctime_r(&tm, buf);
+	struct tm tm, *tm_p;
+	return (tm_p = localtime_r(t, &tm)) ? asctime_r(tm_p, buf) : 0;
 }

Re: [PATCH] Handle localtime errors in ctime

[Rich Felker]

On Thu, Jun 15, 2017 at 08:46:18PM +0300, Alexander Monakov wrote:
> On Thu, 15 Jun 2017, Rich Felker wrote:
> > Look ok?
> > 
> > diff --git a/src/time/ctime_r.c b/src/time/ctime_r.c
> > index d2260a1..05699ca 100644
> > --- a/src/time/ctime_r.c
> > +++ b/src/time/ctime_r.c
> > @@ -3,6 +3,5 @@
> >  char *ctime_r(const time_t *t, char *buf)
> >  {
> >  	struct tm tm;
> > -	localtime_r(t, &tm);
> > -	return asctime_r(&tm, buf);
> > +	return localtime_r(t, &tm) ? asctime_r(&tm, buf) : 0;
> >  }
> 
> Sure, although personally I'm highly tempted to pick up the size optimization
> from reusing return value of localtime_r:
> 
> Alexander
> 
> diff --git a/src/time/ctime_r.c b/src/time/ctime_r.c
> index d2260a16..9047b38f 100644
> --- a/src/time/ctime_r.c
> +++ b/src/time/ctime_r.c
> @@ -2,7 +2,6 @@
>  
>  char *ctime_r(const time_t *t, char *buf)
>  {
> -	struct tm tm;
> -	localtime_r(t, &tm);
> -	return asctime_r(&tm, buf);
> +	struct tm tm, *tm_p;
> +	return (tm_p = localtime_r(t, &tm)) ? asctime_r(tm_p, buf) : 0;
>  }

OK, using this one with a minor change to make it more readable (at
least to me).

Rich

Re: [PATCH] Handle localtime errors in ctime

[Alexander Monakov]

On Tue, 20 Jun 2017, Rich Felker wrote:
> OK, using this one with a minor change to make it more readable (at
> least to me).

Thanks. Shouldn't the ctime change be reverted?

Alexander

Re: [PATCH] Handle localtime errors in ctime

[Rich Felker]

On Wed, Jun 21, 2017 at 10:18:40AM +0300, Alexander Monakov wrote:
> On Tue, 20 Jun 2017, Rich Felker wrote:
> > OK, using this one with a minor change to make it more readable (at
> > least to me).
> 
> Thanks. Shouldn't the ctime change be reverted?

That's a tricky question. I probably wouldn't have accepted it if I
hadn't misread the spec, but it's also not wrong (it only affects a
case with undefined behavior), and it's moderately unfriendly to
contributors to accept then revert a change that's not wrong but just
not needed. The change could mildly reduce the ability to catch/trap
code with UB, but normally the caller will crash anyway from
dereferencing the null pointer returned. So unless others have strong
feelings on this (or the patch contributor wants it reverted) my
leaning is to just leave it.

Rich