Re: [PATCH] hfsplus: replace unbounded sprintf() in hfsplus_{lookup,link,unlink}

[Viacheslav Dubeyko <vdubeyko@redhat.com>]

On Fri, 2026-04-24 at 11:06 +0200, Thorsten Blum wrote:
> While the current code works correctly, replace unbounded sprintf()
> calls with the safer scnprintf() in hfsplus_lookup(), hfsplus_link(),
> and hfsplus_unlink() to follow secure coding best practices.

The patch makes sense. But I would like to request some refactoring work here.
:)

> 
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
>  fs/hfsplus/dir.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
> index 47194370c2c5..ff976995ef58 100644
> --- a/fs/hfsplus/dir.c
> +++ b/fs/hfsplus/dir.c
> @@ -98,7 +98,7 @@ static struct dentry *hfsplus_lookup(struct inode *dir, struct dentry *dentry,
>  				dentry->d_fsdata = (void *)(unsigned long)cnid;
>  				linkid =
>  					be32_to_cpu(entry.file.permissions.dev);
> -				str.len = sprintf(name, "iNode%d", linkid);
> +				str.len = scnprintf(name, sizeof(name), "iNode%d", linkid);
>  				str.name = name;
>  				err = hfsplus_cat_build_key(sb, fd.search_key,
>  					HFSPLUS_SB(sb)->hidden_dir->i_ino,

This piece of code looks like a static inline function. Could we rework this
piece of code?

char name[32];

This declaration looks not very well. I think we need to introduce some named
constant for hardcoded 32 value. Could you please rework it?

> @@ -322,7 +322,7 @@ static int hfsplus_link(struct dentry *src_dentry, struct inode *dst_dir,
>  			get_random_bytes(&id, sizeof(cnid));
>  			id &= 0x3fffffff;
>  			str.name = name;
> -			str.len = sprintf(name, "iNode%d", id);
> +			str.len = scnprintf(name, sizeof(name), "iNode%d", id);
>  			res = hfsplus_rename_cat(inode->i_ino,
>  						 src_dir, &src_dentry->d_name,
>  						 sbi->hidden_dir, &str);

The same issue with name declaration here:

char name[32];

Also, I think it makes sense to initialize the local variables:

char name[32] = {0};

Maybe it makes sense to introduce some small function that execute this:

str.len = sprintf(name, "iNode%d", linkid);
str.name = name;

Something like this:

str = hfsplus_qstr_init(<arguments>);

What do you think?

> @@ -393,7 +393,7 @@ static int hfsplus_unlink(struct inode *dir, struct dentry *dentry)
>  	if (inode->i_ino == cnid &&
>  	    atomic_read(&HFSPLUS_I(inode)->opencnt)) {
>  		str.name = name;
> -		str.len = sprintf(name, "temp%llu", inode->i_ino);
> +		str.len = scnprintf(name, sizeof(name), "temp%llu", inode->i_ino);
>  		res = hfsplus_rename_cat(inode->i_ino,
>  					 dir, &dentry->d_name,
>  					 sbi->hidden_dir, &str);

The same issues here too.

Thanks,
Slava.

[1] https://elixir.bootlin.com/linux/v7.0/source/fs/hfsplus/dir.c#L87