[PATCH] libhandle: fix potential unterminated string problem

[PATCH] libhandle: fix potential unterminated string problem

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

gcc 10.2 complains about the strncpy call here, since it's possible that
the source string is so long that the fspath inside the fdhash structure
will end up without a null terminator.  Work around strncpy braindamage
yet again by forcing the string to be terminated properly.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
Unless this is supposed to be a memcpy?  But it doesn't look like it.
---
 libhandle/handle.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libhandle/handle.c b/libhandle/handle.c
index eb099f43791e..5c1686b3968d 100644
--- a/libhandle/handle.c
+++ b/libhandle/handle.c
@@ -107,7 +107,8 @@ path_to_fshandle(
 		}
 
 		fdhp->fsfd = fd;
-		strncpy(fdhp->fspath, fspath, sizeof(fdhp->fspath));
+		strncpy(fdhp->fspath, fspath, sizeof(fdhp->fspath) - 1);
+		fdhp->fspath[sizeof(fdhp->fspath) - 1] = 0;
 		memcpy(fdhp->fsh, *fshanp, FSIDSIZE);
 
 		fdhp->fnxt = fdhash_head;

Re: [PATCH] libhandle: fix potential unterminated string problem

[Eric Sandeen]

On 10/7/20 10:58 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> gcc 10.2 complains about the strncpy call here, since it's possible that
> the source string is so long that the fspath inside the fdhash structure
> will end up without a null terminator.  Work around strncpy braindamage
> yet again by forcing the string to be terminated properly.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> Unless this is supposed to be a memcpy?  But it doesn't look like it.

so FWIW, coverity has complained about this one forever, CID 1297520

I think the thing that kept me from addressing it before was uncertainty
around lopping one byte off of PATH_MAX / MAXPATHLEN, essentially.

In reality, I think (?) PATH_MAX is a bit of a fiction anyway, and
there's probably nothing special about that one byte, and I think (?)
this is always a path to a mountpoint (?) which is unlikely to be 4096
or 4095 chars long anyway (?) so ... um ... sure.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  libhandle/handle.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libhandle/handle.c b/libhandle/handle.c
> index eb099f43791e..5c1686b3968d 100644
> --- a/libhandle/handle.c
> +++ b/libhandle/handle.c
> @@ -107,7 +107,8 @@ path_to_fshandle(
>  		}
>  
>  		fdhp->fsfd = fd;
> -		strncpy(fdhp->fspath, fspath, sizeof(fdhp->fspath));
> +		strncpy(fdhp->fspath, fspath, sizeof(fdhp->fspath) - 1);
> +		fdhp->fspath[sizeof(fdhp->fspath) - 1] = 0;
>  		memcpy(fdhp->fsh, *fshanp, FSIDSIZE);
>  
>  		fdhp->fnxt = fdhash_head;
> 

Re: [PATCH] libhandle: fix potential unterminated string problem

[Christoph Hellwig]

On Wed, Oct 07, 2020 at 08:58:34PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> gcc 10.2 complains about the strncpy call here, since it's possible that
> the source string is so long that the fspath inside the fdhash structure
> will end up without a null terminator.  Work around strncpy braindamage
> yet again by forcing the string to be terminated properly.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> Unless this is supposed to be a memcpy?  But it doesn't look like it.

No, it should not be memcy, and the change looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

That being said path_to_fspath where fspath originates is horrible
and not mutlithreading safe due to the static filename buffer.