Linux-XFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] libhandle: fix potential unterminated string problem
@ 2020-10-08  3:58 Darrick J. Wong
  2020-10-08 21:53 ` Eric Sandeen
  2020-10-15  8:29 ` Christoph Hellwig
  0 siblings, 2 replies; 3+ messages in thread
From: Darrick J. Wong @ 2020-10-08  3:58 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: xfs

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;

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

* Re: [PATCH] libhandle: fix potential unterminated string problem
  2020-10-08  3:58 [PATCH] libhandle: fix potential unterminated string problem Darrick J. Wong
@ 2020-10-08 21:53 ` Eric Sandeen
  2020-10-15  8:29 ` Christoph Hellwig
  1 sibling, 0 replies; 3+ messages in thread
From: Eric Sandeen @ 2020-10-08 21:53 UTC (permalink / raw)
  To: Darrick J. Wong, Eric Sandeen; +Cc: xfs

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;
> 

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

* Re: [PATCH] libhandle: fix potential unterminated string problem
  2020-10-08  3:58 [PATCH] libhandle: fix potential unterminated string problem Darrick J. Wong
  2020-10-08 21:53 ` Eric Sandeen
@ 2020-10-15  8:29 ` Christoph Hellwig
  1 sibling, 0 replies; 3+ messages in thread
From: Christoph Hellwig @ 2020-10-15  8:29 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: Eric Sandeen, xfs

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.

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

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-08  3:58 [PATCH] libhandle: fix potential unterminated string problem Darrick J. Wong
2020-10-08 21:53 ` Eric Sandeen
2020-10-15  8:29 ` Christoph Hellwig

Linux-XFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-xfs/0 linux-xfs/git/0.git

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

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-xfs


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