Eric Biggers <ebiggers@xxxxxxxxxx> writes: > On Wed, Aug 02, 2023 at 10:49:31AM +0100, Luís Henriques wrote: >> If casefolding the filename fails, we'll be leaking fscrypt_buf name. > > fscrypt_buf => fscrypt_name > >> diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c >> index e20ac0654b3f..3c05c7f3415b 100644 >> --- a/fs/ext4/crypto.c >> +++ b/fs/ext4/crypto.c >> @@ -33,6 +33,8 @@ int ext4_fname_setup_filename(struct inode *dir, const struct qstr *iname, >> struct fscrypt_name name; >> int err; >> >> err = fscrypt_setup_filename(dir, iname, lookup, &name); >> if (err) >> return err; >> >> ext4_fname_from_fscrypt_name(fname, &name); >> >> #if IS_ENABLED(CONFIG_UNICODE) >> err = ext4_fname_setup_ci_filename(dir, iname, fname); >> + if (err) >> + fscrypt_free_filename(&name); >> #endif >> return err; >> } >> @@ -51,6 +53,8 @@ int ext4_fname_prepare_lookup(struct inode *dir, struct dentry *dentry, >> struct fscrypt_name name; >> int err; >> >> err = fscrypt_prepare_lookup(dir, dentry, &name); >> if (err) >> return err; >> >> ext4_fname_from_fscrypt_name(fname, &name); >> >> #if IS_ENABLED(CONFIG_UNICODE) >> err = ext4_fname_setup_ci_filename(dir, &dentry->d_name, fname); >> + if (err) >> + fscrypt_free_filename(&name); >> #endif >> return err; >> } > > This works, but it's a bit weird that the freeing happens on the original struct > fscrypt_name after it has already been "moved" to the struct ext4_filename by > ext4_fname_from_fscrypt_name(). That leaves a dangling pointer in the struct > ext4_filename. Maybe you should call ext4_fname_free_filename() instead, even > though it would do some unnecessary work? That makes sense, specially because fname is a parameter and it's probably a good idea to clean-up everything before returning an error. Thanks. Cheers, -- Luís
