[PATCH] Bunch of options for vfs_fileid

[Jeremy Allison]

On Sat, Jan 06, 2018 at 04:33:03PM +0100, Ralph Böhme via samba-technical wrote:
> Hi Christian,
> 
> On Sat, Jan 06, 2018 at 08:53:01AM +0100, Christian Ambach wrote:
> > > +	char hostname[HOST_NAME_MAX+1];
> > [...]
> > > +
> > > +	rc = gethostname(hostname, HOST_NAME_MAX+1);
> > > +	if (rc != 0) {
> > > +		DBG_ERR("gethostname failed\n");
> > > +		return UINT64_MAX;
> > > +	}
> > man gethostname on Linux notes a discrepancy between Linux and POSIX:
> > in POSIX gethostname is allowed to return up to 255 characters, while
> > HOST_NAME_MAX is just 64 characters on Linux. Not sure how this looks
> > like on other platforms. Maybe it makes more sense to use an array of
> > size 256 to be on the safe side?
> 
> Why? As long as the array size matches the size passed to gethostname()
> everything's fine.
> 
> > The error path that returns the same number for all devices might be
> > problematic too, as it will then generate the same fileid for files
> > with the same inode number on different filesystems.
> 
> Yeah, that's a bit problematic in theory. In practice, afaict gethostname() will
> only fail if the passed in buffer size is too small, but this can't happen when
> using HOST_NAME_MAX.
> 
> > > +	TALLOC_FREE(devname);
> > > +
> > > +	id = fileid_uint64_hash((uint8_t *)devname, devname_len);
> > 
> > Isn't this a use-after-free?
> 
> You bet it is. Damn! Thanks for catching this. Patch attached.

Ralph I'm sorry, I missed that ! I'll add this on top.

> -- 
> Ralph Boehme, Samba Team       https://samba.org/
> Samba Developer, SerNet GmbH   https://sernet.de/en/samba/

> From e05f315cb4fd93ec2924a0d528f90bc601445a8e Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Sat, 6 Jan 2018 16:13:52 +0100
> Subject: [PATCH] vfs_fileid: fix a use after free
> 
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
>  source3/modules/vfs_fileid.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/source3/modules/vfs_fileid.c b/source3/modules/vfs_fileid.c
> index 98cc32d62d5..c890876c998 100644
> --- a/source3/modules/vfs_fileid.c
> +++ b/source3/modules/vfs_fileid.c
> @@ -233,9 +233,11 @@ static uint64_t fileid_device_mapping_hostname(struct fileid_handle_data *data,
>  		return UINT64_MAX;
>  	}
>  	devname_len = talloc_array_length(devname) - 1;
> -	TALLOC_FREE(devname);
>  
>  	id = fileid_uint64_hash((uint8_t *)devname, devname_len);
> +
> +	TALLOC_FREE(devname);
> +
>  	return id;
>  }
>  
> -- 
> 2.13.6
>