mailing list of musl libc
 help / color / Atom feed
* [musl] [PATCH] musl: lutimes: Add checks for input parameters
@ 2020-03-01  6:57 Liu Jie
  2020-03-01  7:17 ` Rich Felker
  2020-03-01  8:37 ` Markus Wichmann
  0 siblings, 2 replies; 6+ messages in thread
From: Liu Jie @ 2020-03-01  6:57 UTC (permalink / raw)
  To: musl; +Cc: yunlong.song, liujie1

For the input parameter struct timeval tv, need to
determine whether it is invalid inputs.

Signed-off-by: Liu Jie <liujie1@huawei.com>
---
 src/legacy/lutimes.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/src/legacy/lutimes.c b/src/legacy/lutimes.c
index 2e5502d1..6e7e1be3 100644
--- a/src/legacy/lutimes.c
+++ b/src/legacy/lutimes.c
@@ -2,13 +2,22 @@
 #include <sys/stat.h>
 #include <sys/time.h>
 #include <fcntl.h>
+#include <stdio.h>
+#include <errno.h>
 
 int lutimes(const char *filename, const struct timeval tv[2])
 {
 	struct timespec times[2];
-	times[0].tv_sec  = tv[0].tv_sec;
-	times[0].tv_nsec = tv[0].tv_usec * 1000;
-	times[1].tv_sec  = tv[1].tv_sec;
-	times[1].tv_nsec = tv[1].tv_usec * 1000;
-	return utimensat(AT_FDCWD, filename, times, AT_SYMLINK_NOFOLLOW);
+	if (tv != NULL) {
+		if (tv[0].tv_sec < 0 || tv[0].tv_usec < 0 ||
+		    tv[1].tv_sec < 0 || tv[1].tv_usec < 0) {
+			errno = EINVAL;
+			return -1;
+		}
+		times[0].tv_sec  = tv[0].tv_sec;
+		times[0].tv_nsec = tv[0].tv_usec * 1000;
+		times[1].tv_sec  = tv[1].tv_sec;
+		times[1].tv_nsec = tv[1].tv_usec * 1000;
+	}
+	return utimensat(AT_FDCWD, filename, tv ? times : NULL, AT_SYMLINK_NOFOLLOW);
 }
-- 
2.17.1


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

* Re: [musl] [PATCH] musl: lutimes: Add checks for input parameters
  2020-03-01  6:57 [musl] [PATCH] musl: lutimes: Add checks for input parameters Liu Jie
@ 2020-03-01  7:17 ` Rich Felker
  2020-03-01 20:37   ` Samuel Holland
  2020-03-01  8:37 ` Markus Wichmann
  1 sibling, 1 reply; 6+ messages in thread
From: Rich Felker @ 2020-03-01  7:17 UTC (permalink / raw)
  To: musl

On Sun, Mar 01, 2020 at 02:57:30PM +0800, Liu Jie wrote:
> For the input parameter struct timeval tv, need to
> determine whether it is invalid inputs.
> 
> Signed-off-by: Liu Jie <liujie1@huawei.com>
> ---
>  src/legacy/lutimes.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/src/legacy/lutimes.c b/src/legacy/lutimes.c
> index 2e5502d1..6e7e1be3 100644
> --- a/src/legacy/lutimes.c
> +++ b/src/legacy/lutimes.c
> @@ -2,13 +2,22 @@
>  #include <sys/stat.h>
>  #include <sys/time.h>
>  #include <fcntl.h>
> +#include <stdio.h>
> +#include <errno.h>
>  
>  int lutimes(const char *filename, const struct timeval tv[2])
>  {
>  	struct timespec times[2];
> -	times[0].tv_sec  = tv[0].tv_sec;
> -	times[0].tv_nsec = tv[0].tv_usec * 1000;
> -	times[1].tv_sec  = tv[1].tv_sec;
> -	times[1].tv_nsec = tv[1].tv_usec * 1000;
> -	return utimensat(AT_FDCWD, filename, times, AT_SYMLINK_NOFOLLOW);
> +	if (tv != NULL) {
> +		if (tv[0].tv_sec < 0 || tv[0].tv_usec < 0 ||
> +		    tv[1].tv_sec < 0 || tv[1].tv_usec < 0) {
> +			errno = EINVAL;
> +			return -1;
> +		}
> +		times[0].tv_sec  = tv[0].tv_sec;
> +		times[0].tv_nsec = tv[0].tv_usec * 1000;
> +		times[1].tv_sec  = tv[1].tv_sec;
> +		times[1].tv_nsec = tv[1].tv_usec * 1000;
> +	}
> +	return utimensat(AT_FDCWD, filename, tv ? times : NULL, AT_SYMLINK_NOFOLLOW);
>  }
> -- 
> 2.17.1

This patch causes uninitialized timespecs to be used if a null pointer
is passed, silently corrupting data. If there is any historical
documented precedent for this function accepting a null pointer and
doing something meaningful, then the patch needs to do whatever that
meaningful thing is rather than usign uninitialized data. If not, the
preferred behavior is the current behavior: to crash so that the usage
error is caught.

Rich

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

* Re: [musl] [PATCH] musl: lutimes: Add checks for input parameters
  2020-03-01  6:57 [musl] [PATCH] musl: lutimes: Add checks for input parameters Liu Jie
  2020-03-01  7:17 ` Rich Felker
@ 2020-03-01  8:37 ` Markus Wichmann
  1 sibling, 0 replies; 6+ messages in thread
From: Markus Wichmann @ 2020-03-01  8:37 UTC (permalink / raw)
  To: musl

On Sun, Mar 01, 2020 at 02:57:30PM +0800, Liu Jie wrote:
> For the input parameter struct timeval tv, need to
> determine whether it is invalid inputs.
>

Why? lutimes() is a Linux-specific function, so the manpage is as close
to a specification as you're ever going to get, and it does not specify
an EINVAL return.

Adding the NULL pointer check, though, is probably justified, given that
the manpage states that lutimes() acts "in the same way as utimes(2)"
(with an irrelevant exception afterwards), and utimes() allows for a
NULL tv input.

The kernel itself also checks the input values again. While I usually am
in favor of failing faster, in this case I fail to see the benefit.
Especially since you're not testing for the one case that could make the
kernel accept a timestamp that was invalid on input: An overflowing one.
But you don't test for the upper limit.

Oh, and the seconds are allowed to be negative. If someone wants to set
a timestamp from before 1970, the libc is the wrong place to stop them.
If such dates are invalid from your application's perspective, filter
that there.

Have a nice Sunday,
Markus

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

* Re: [musl] [PATCH] musl: lutimes: Add checks for input parameters
  2020-03-01  7:17 ` Rich Felker
@ 2020-03-01 20:37   ` Samuel Holland
  2020-03-01 20:39     ` Rich Felker
  0 siblings, 1 reply; 6+ messages in thread
From: Samuel Holland @ 2020-03-01 20:37 UTC (permalink / raw)
  To: musl, Rich Felker

On 3/1/20 1:17 AM, Rich Felker wrote:
> On Sun, Mar 01, 2020 at 02:57:30PM +0800, Liu Jie wrote:
>> For the input parameter struct timeval tv, need to
>> determine whether it is invalid inputs.
>>
>> Signed-off-by: Liu Jie <liujie1@huawei.com>
>> ---
>>  src/legacy/lutimes.c | 19 ++++++++++++++-----
>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/legacy/lutimes.c b/src/legacy/lutimes.c
>> index 2e5502d1..6e7e1be3 100644
>> --- a/src/legacy/lutimes.c
>> +++ b/src/legacy/lutimes.c
>> @@ -2,13 +2,22 @@
>>  #include <sys/stat.h>
>>  #include <sys/time.h>
>>  #include <fcntl.h>
>> +#include <stdio.h>
>> +#include <errno.h>
>>  
>>  int lutimes(const char *filename, const struct timeval tv[2])
>>  {
>>  	struct timespec times[2];
>> -	times[0].tv_sec  = tv[0].tv_sec;
>> -	times[0].tv_nsec = tv[0].tv_usec * 1000;
>> -	times[1].tv_sec  = tv[1].tv_sec;
>> -	times[1].tv_nsec = tv[1].tv_usec * 1000;
>> -	return utimensat(AT_FDCWD, filename, times, AT_SYMLINK_NOFOLLOW);
>> +	if (tv != NULL) {
>> +		if (tv[0].tv_sec < 0 || tv[0].tv_usec < 0 ||
>> +		    tv[1].tv_sec < 0 || tv[1].tv_usec < 0) {
>> +			errno = EINVAL;
>> +			return -1;
>> +		}
>> +		times[0].tv_sec  = tv[0].tv_sec;
>> +		times[0].tv_nsec = tv[0].tv_usec * 1000;
>> +		times[1].tv_sec  = tv[1].tv_sec;
>> +		times[1].tv_nsec = tv[1].tv_usec * 1000;
>> +	}
>> +	return utimensat(AT_FDCWD, filename, tv ? times : NULL, AT_SYMLINK_NOFOLLOW);
>>  }
>> -- 
>> 2.17.1
> 
> This patch causes uninitialized timespecs to be used if a null pointer
> is passed, silently corrupting data. If there is any historical
> documented precedent for this function accepting a null pointer and
> doing something meaningful, then the patch needs to do whatever that
> meaningful thing is rather than usign uninitialized data. If not, the
> preferred behavior is the current behavior: to crash so that the usage
> error is caught.

How do you see that uninitialized timespecs are used? times is only passed to
utimensat if tv is nonnull, and in that case times is initialized.

Regards,
Samuel



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

* Re: [musl] [PATCH] musl: lutimes: Add checks for input parameters
  2020-03-01 20:37   ` Samuel Holland
@ 2020-03-01 20:39     ` Rich Felker
  2020-03-20 19:54       ` Rich Felker
  0 siblings, 1 reply; 6+ messages in thread
From: Rich Felker @ 2020-03-01 20:39 UTC (permalink / raw)
  To: musl

On Sun, Mar 01, 2020 at 02:37:38PM -0600, Samuel Holland wrote:
> On 3/1/20 1:17 AM, Rich Felker wrote:
> > On Sun, Mar 01, 2020 at 02:57:30PM +0800, Liu Jie wrote:
> >> For the input parameter struct timeval tv, need to
> >> determine whether it is invalid inputs.
> >>
> >> Signed-off-by: Liu Jie <liujie1@huawei.com>
> >> ---
> >>  src/legacy/lutimes.c | 19 ++++++++++++++-----
> >>  1 file changed, 14 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/src/legacy/lutimes.c b/src/legacy/lutimes.c
> >> index 2e5502d1..6e7e1be3 100644
> >> --- a/src/legacy/lutimes.c
> >> +++ b/src/legacy/lutimes.c
> >> @@ -2,13 +2,22 @@
> >>  #include <sys/stat.h>
> >>  #include <sys/time.h>
> >>  #include <fcntl.h>
> >> +#include <stdio.h>
> >> +#include <errno.h>
> >>  
> >>  int lutimes(const char *filename, const struct timeval tv[2])
> >>  {
> >>  	struct timespec times[2];
> >> -	times[0].tv_sec  = tv[0].tv_sec;
> >> -	times[0].tv_nsec = tv[0].tv_usec * 1000;
> >> -	times[1].tv_sec  = tv[1].tv_sec;
> >> -	times[1].tv_nsec = tv[1].tv_usec * 1000;
> >> -	return utimensat(AT_FDCWD, filename, times, AT_SYMLINK_NOFOLLOW);
> >> +	if (tv != NULL) {
> >> +		if (tv[0].tv_sec < 0 || tv[0].tv_usec < 0 ||
> >> +		    tv[1].tv_sec < 0 || tv[1].tv_usec < 0) {
> >> +			errno = EINVAL;
> >> +			return -1;
> >> +		}
> >> +		times[0].tv_sec  = tv[0].tv_sec;
> >> +		times[0].tv_nsec = tv[0].tv_usec * 1000;
> >> +		times[1].tv_sec  = tv[1].tv_sec;
> >> +		times[1].tv_nsec = tv[1].tv_usec * 1000;
> >> +	}
> >> +	return utimensat(AT_FDCWD, filename, tv ? times : NULL, AT_SYMLINK_NOFOLLOW);
> >>  }
> >> -- 
> >> 2.17.1
> > 
> > This patch causes uninitialized timespecs to be used if a null pointer
> > is passed, silently corrupting data. If there is any historical
> > documented precedent for this function accepting a null pointer and
> > doing something meaningful, then the patch needs to do whatever that
> > meaningful thing is rather than usign uninitialized data. If not, the
> > preferred behavior is the current behavior: to crash so that the usage
> > error is caught.
> 
> How do you see that uninitialized timespecs are used? times is only passed to
> utimensat if tv is nonnull, and in that case times is initialized.

Oh, I misread it. In that case it seems like it might be correct
as-is.

Rich

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

* Re: [musl] [PATCH] musl: lutimes: Add checks for input parameters
  2020-03-01 20:39     ` Rich Felker
@ 2020-03-20 19:54       ` Rich Felker
  0 siblings, 0 replies; 6+ messages in thread
From: Rich Felker @ 2020-03-20 19:54 UTC (permalink / raw)
  To: musl

On Sun, Mar 01, 2020 at 03:39:21PM -0500, Rich Felker wrote:
> On Sun, Mar 01, 2020 at 02:37:38PM -0600, Samuel Holland wrote:
> > On 3/1/20 1:17 AM, Rich Felker wrote:
> > > On Sun, Mar 01, 2020 at 02:57:30PM +0800, Liu Jie wrote:
> > >> For the input parameter struct timeval tv, need to
> > >> determine whether it is invalid inputs.
> > >>
> > >> Signed-off-by: Liu Jie <liujie1@huawei.com>
> > >> ---
> > >>  src/legacy/lutimes.c | 19 ++++++++++++++-----
> > >>  1 file changed, 14 insertions(+), 5 deletions(-)
> > >>
> > >> diff --git a/src/legacy/lutimes.c b/src/legacy/lutimes.c
> > >> index 2e5502d1..6e7e1be3 100644
> > >> --- a/src/legacy/lutimes.c
> > >> +++ b/src/legacy/lutimes.c
> > >> @@ -2,13 +2,22 @@
> > >>  #include <sys/stat.h>
> > >>  #include <sys/time.h>
> > >>  #include <fcntl.h>
> > >> +#include <stdio.h>
> > >> +#include <errno.h>
> > >>  
> > >>  int lutimes(const char *filename, const struct timeval tv[2])
> > >>  {
> > >>  	struct timespec times[2];
> > >> -	times[0].tv_sec  = tv[0].tv_sec;
> > >> -	times[0].tv_nsec = tv[0].tv_usec * 1000;
> > >> -	times[1].tv_sec  = tv[1].tv_sec;
> > >> -	times[1].tv_nsec = tv[1].tv_usec * 1000;
> > >> -	return utimensat(AT_FDCWD, filename, times, AT_SYMLINK_NOFOLLOW);
> > >> +	if (tv != NULL) {
> > >> +		if (tv[0].tv_sec < 0 || tv[0].tv_usec < 0 ||
> > >> +		    tv[1].tv_sec < 0 || tv[1].tv_usec < 0) {
> > >> +			errno = EINVAL;
> > >> +			return -1;
> > >> +		}
> > >> +		times[0].tv_sec  = tv[0].tv_sec;
> > >> +		times[0].tv_nsec = tv[0].tv_usec * 1000;
> > >> +		times[1].tv_sec  = tv[1].tv_sec;
> > >> +		times[1].tv_nsec = tv[1].tv_usec * 1000;
> > >> +	}
> > >> +	return utimensat(AT_FDCWD, filename, tv ? times : NULL, AT_SYMLINK_NOFOLLOW);
> > >>  }
> > >> -- 
> > >> 2.17.1
> > > 
> > > This patch causes uninitialized timespecs to be used if a null pointer
> > > is passed, silently corrupting data. If there is any historical
> > > documented precedent for this function accepting a null pointer and
> > > doing something meaningful, then the patch needs to do whatever that
> > > meaningful thing is rather than usign uninitialized data. If not, the
> > > preferred behavior is the current behavior: to crash so that the usage
> > > error is caught.
> > 
> > How do you see that uninitialized timespecs are used? times is only passed to
> > utimensat if tv is nonnull, and in that case times is initialized.
> 
> Oh, I misread it. In that case it seems like it might be correct
> as-is.

I've looked back into this, and indeed I think the concept of the
patch makes sense. The equivalent logic for utimes() is buried in
__futimesat, but can't be reused directly because __futimesat doesn't
take a flags argument for AT_NOFOLLOW. There are some subtle
differences to how they handle error cases, so it'd be nice to figure
out exactly what the right behavior should be and get it right both
places.

Sorry for taking a while to get back to this.

Rich

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

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-01  6:57 [musl] [PATCH] musl: lutimes: Add checks for input parameters Liu Jie
2020-03-01  7:17 ` Rich Felker
2020-03-01 20:37   ` Samuel Holland
2020-03-01 20:39     ` Rich Felker
2020-03-20 19:54       ` Rich Felker
2020-03-01  8:37 ` Markus Wichmann

mailing list of musl libc

Archives are clonable: git clone --mirror http://inbox.vuxu.org/musl

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://inbox.vuxu.org/vuxu.archive.musl


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