[musl] [PATCH] compat: time32: Fix potential NULL dereference in

[musl] [PATCH] compat: time32: Fix potential NULL dereference in

[Anton Moryakov]

Static analyzer reported:
Pointer '0', which is passed as 2nd parameter in call to function '__lutimes_time64' at 
lutimes_time32.c:9, where it is dereferenced at lutimes.c:9, may have a NULL value.

Corrections explained:
The function __lutimes_time32 could pass a NULL pointer to lutimes
when times32 was NULL, leading to potential dereference of NULL
inside lutimes. This has been fixed by explicitly handling the
case where times32 is NULL and passing NULL directly to lutimes.

Additionally, the conversion from struct timeval32 to struct timeval
is now done in a separate block when times32 is not NULL, ensuring
safe access to the times32 array.

Triggers found by static analyzer Svace.

Signed-off-by: Anton Moryakov <ant.v.moryakov@gmail.com>

---
 compat/time32/lutimes_time32.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/compat/time32/lutimes_time32.c b/compat/time32/lutimes_time32.c
index 7f75cd4a..48f839f6 100644
--- a/compat/time32/lutimes_time32.c
+++ b/compat/time32/lutimes_time32.c
@@ -6,7 +6,13 @@
 
 int __lutimes_time32(const char *path, const struct timeval32 times32[2])
 {
-	return lutimes(path, !times32 ? 0 : ((struct timeval[2]){
-		{.tv_sec = times32[0].tv_sec,.tv_usec = times32[0].tv_usec},
-		{.tv_sec = times32[1].tv_sec,.tv_usec = times32[1].tv_usec}}));
-}
+    if (!times32) {
+        return lutimes(path, NULL);
+    } else {
+        struct timeval times[2] = {
+            {.tv_sec = times32[0].tv_sec, .tv_usec = times32[0].tv_usec},
+            {.tv_sec = times32[1].tv_sec, .tv_usec = times32[1].tv_usec}
+        };
+        return lutimes(path, times);
+    }
+}
\ No newline at end of file
-- 
2.30.2

RE: [musl] [PATCH] compat: time32: Fix potential NULL dereference in

[Mike Granby]

Doesn't this still pass a NULL when times32 is zero?

In fact, isn't it just exactly equivalent?

In fact, isn't a bad a idea just to blindly post the output of static analysis tools?

-----Original Message-----
From: Anton Moryakov <ant.v.moryakov@gmail.com> 
Sent: Sunday, February 16, 2025 12:56 PM
To: musl@lists.openwall.com
Cc: Anton Moryakov <ant.v.moryakov@gmail.com>
Subject: [musl] [PATCH] compat: time32: Fix potential NULL dereference in

Static analyzer reported:
Pointer '0', which is passed as 2nd parameter in call to function '__lutimes_time64' at lutimes_time32.c:9, where it is dereferenced at lutimes.c:9, may have a NULL value.

Corrections explained:
The function __lutimes_time32 could pass a NULL pointer to lutimes when times32 was NULL, leading to potential dereference of NULL inside lutimes. This has been fixed by explicitly handling the case where times32 is NULL and passing NULL directly to lutimes.

Additionally, the conversion from struct timeval32 to struct timeval is now done in a separate block when times32 is not NULL, ensuring safe access to the times32 array.

Triggers found by static analyzer Svace.

Signed-off-by: Anton Moryakov <ant.v.moryakov@gmail.com>

---
 compat/time32/lutimes_time32.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/compat/time32/lutimes_time32.c b/compat/time32/lutimes_time32.c index 7f75cd4a..48f839f6 100644
--- a/compat/time32/lutimes_time32.c
+++ b/compat/time32/lutimes_time32.c
@@ -6,7 +6,13 @@
 
 int __lutimes_time32(const char *path, const struct timeval32 times32[2])  {
-	return lutimes(path, !times32 ? 0 : ((struct timeval[2]){
-		{.tv_sec = times32[0].tv_sec,.tv_usec = times32[0].tv_usec},
-		{.tv_sec = times32[1].tv_sec,.tv_usec = times32[1].tv_usec}}));
-}
+    if (!times32) {
+        return lutimes(path, NULL);
+    } else {
+        struct timeval times[2] = {
+            {.tv_sec = times32[0].tv_sec, .tv_usec = times32[0].tv_usec},
+            {.tv_sec = times32[1].tv_sec, .tv_usec = times32[1].tv_usec}
+        };
+        return lutimes(path, times);
+    }
+}
\ No newline at end of file
--
2.30.2

Re: [musl] [PATCH] compat: time32: Fix potential NULL dereference in

[Rich Felker]

On Sun, Feb 16, 2025 at 06:00:07PM +0000, Mike Granby wrote:
> Doesn't this still pass a NULL when times32 is zero?
> 
> In fact, isn't it just exactly equivalent?

Yes, the dereference is already conditional on times32 being non-null.

> In fact, isn't a bad a idea just to blindly post the output of static analysis tools?

Especially if the static analysis tool does not understand that the
untaken branch of the tertiary operator is not evaluated...


> -----Original Message-----
> From: Anton Moryakov <ant.v.moryakov@gmail.com> 
> Sent: Sunday, February 16, 2025 12:56 PM
> To: musl@lists.openwall.com
> Cc: Anton Moryakov <ant.v.moryakov@gmail.com>
> Subject: [musl] [PATCH] compat: time32: Fix potential NULL dereference in
> 
> Static analyzer reported:
> Pointer '0', which is passed as 2nd parameter in call to function '__lutimes_time64' at lutimes_time32.c:9, where it is dereferenced at lutimes.c:9, may have a NULL value.
> 
> Corrections explained:
> The function __lutimes_time32 could pass a NULL pointer to lutimes when times32 was NULL, leading to potential dereference of NULL inside lutimes. This has been fixed by explicitly handling the case where times32 is NULL and passing NULL directly to lutimes.
> 
> Additionally, the conversion from struct timeval32 to struct timeval is now done in a separate block when times32 is not NULL, ensuring safe access to the times32 array.
> 
> Triggers found by static analyzer Svace.
> 
> Signed-off-by: Anton Moryakov <ant.v.moryakov@gmail.com>
> 
> ---
>  compat/time32/lutimes_time32.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/compat/time32/lutimes_time32.c b/compat/time32/lutimes_time32.c index 7f75cd4a..48f839f6 100644
> --- a/compat/time32/lutimes_time32.c
> +++ b/compat/time32/lutimes_time32.c
> @@ -6,7 +6,13 @@
>  
>  int __lutimes_time32(const char *path, const struct timeval32 times32[2])  {
> -	return lutimes(path, !times32 ? 0 : ((struct timeval[2]){
> -		{.tv_sec = times32[0].tv_sec,.tv_usec = times32[0].tv_usec},
> -		{.tv_sec = times32[1].tv_sec,.tv_usec = times32[1].tv_usec}}));
> -}
> +    if (!times32) {
> +        return lutimes(path, NULL);
> +    } else {
> +        struct timeval times[2] = {
> +            {.tv_sec = times32[0].tv_sec, .tv_usec = times32[0].tv_usec},
> +            {.tv_sec = times32[1].tv_sec, .tv_usec = times32[1].tv_usec}
> +        };
> +        return lutimes(path, times);
> +    }
> +}
> \ No newline at end of file
> --
> 2.30.2

Re: [musl] [PATCH] compat: time32: Fix potential NULL dereference in

[Anton Moryakov]

[-- Attachment #1: Type: text/plain, Size: 2887 bytes --]

thanks for the feedback. I checked again, yes, the static analyzer was wrong

вс, 16 февр. 2025 г. в 21:04, Rich Felker <dalias@libc.org>:

> On Sun, Feb 16, 2025 at 06:00:07PM +0000, Mike Granby wrote:
> > Doesn't this still pass a NULL when times32 is zero?
> >
> > In fact, isn't it just exactly equivalent?
>
> Yes, the dereference is already conditional on times32 being non-null.
>
> > In fact, isn't a bad a idea just to blindly post the output of static
> analysis tools?
>
> Especially if the static analysis tool does not understand that the
> untaken branch of the tertiary operator is not evaluated...
>
>
> > -----Original Message-----
> > From: Anton Moryakov <ant.v.moryakov@gmail.com>
> > Sent: Sunday, February 16, 2025 12:56 PM
> > To: musl@lists.openwall.com
> > Cc: Anton Moryakov <ant.v.moryakov@gmail.com>
> > Subject: [musl] [PATCH] compat: time32: Fix potential NULL dereference in
> >
> > Static analyzer reported:
> > Pointer '0', which is passed as 2nd parameter in call to function
> '__lutimes_time64' at lutimes_time32.c:9, where it is dereferenced at
> lutimes.c:9, may have a NULL value.
> >
> > Corrections explained:
> > The function __lutimes_time32 could pass a NULL pointer to lutimes when
> times32 was NULL, leading to potential dereference of NULL inside lutimes.
> This has been fixed by explicitly handling the case where times32 is NULL
> and passing NULL directly to lutimes.
> >
> > Additionally, the conversion from struct timeval32 to struct timeval is
> now done in a separate block when times32 is not NULL, ensuring safe access
> to the times32 array.
> >
> > Triggers found by static analyzer Svace.
> >
> > Signed-off-by: Anton Moryakov <ant.v.moryakov@gmail.com>
> >
> > ---
> >  compat/time32/lutimes_time32.c | 14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/compat/time32/lutimes_time32.c
> b/compat/time32/lutimes_time32.c index 7f75cd4a..48f839f6 100644
> > --- a/compat/time32/lutimes_time32.c
> > +++ b/compat/time32/lutimes_time32.c
> > @@ -6,7 +6,13 @@
> >
> >  int __lutimes_time32(const char *path, const struct timeval32
> times32[2])  {
> > -     return lutimes(path, !times32 ? 0 : ((struct timeval[2]){
> > -             {.tv_sec = times32[0].tv_sec,.tv_usec =
> times32[0].tv_usec},
> > -             {.tv_sec = times32[1].tv_sec,.tv_usec =
> times32[1].tv_usec}}));
> > -}
> > +    if (!times32) {
> > +        return lutimes(path, NULL);
> > +    } else {
> > +        struct timeval times[2] = {
> > +            {.tv_sec = times32[0].tv_sec, .tv_usec =
> times32[0].tv_usec},
> > +            {.tv_sec = times32[1].tv_sec, .tv_usec = times32[1].tv_usec}
> > +        };
> > +        return lutimes(path, times);
> > +    }
> > +}
> > \ No newline at end of file
> > --
> > 2.30.2
>

[-- Attachment #2: Type: text/html, Size: 3771 bytes --]