btrfs-progs: Don't crash when processing a clone request during received

Commit Message

Nikolay Borisov Jan. 10, 2022, 11:12 a.m. UTC

If subvol_uuid_search can't find the clone root then 'si' would either
be NULL or contain an errno. The behavior of this function was
changed as a result of commit
ac5954ee36a5 ("btrfs-progs: merge subvol_uuid_search helpers"). Before
this commit subvol_uuid_search() was a wrapper around subvol_uuid_search2
and it guaranteed to either return well-fromed 'si' or NULL. This was
sufficient for the check after the out label in process_clone.

Properly handle this new semantic by changing the simple null check to
IS_ERR_OR_NULL which covers all possible return value of
subvol_uuid_search.

Signed-off-by: Nikolay Borisov <[e-mail]>
Reported-by: Chris Muprhy <[e-mail]>
Link: https://lore.kernel.org/linux-btrfs/[e-mail]/
---
 cmds/receive.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--
2.17.1

Comments

Nikolay Borisov Jan. 10, 2022, 11:13 a.m. UTC | #1

On 10.01.22 г. 13:12, Nikolay Borisov wrote:
> If subvol_uuid_search can't find the clone root then 'si' would either
> be NULL or contain an errno. The behavior of this function was
> changed as a result of commit
> ac5954ee36a5 ("btrfs-progs: merge subvol_uuid_search helpers"). Before
> this commit subvol_uuid_search() was a wrapper around subvol_uuid_search2
> and it guaranteed to either return well-fromed 'si' or NULL. This was
> sufficient for the check after the out label in process_clone.
> 
> Properly handle this new semantic by changing the simple null check to
> IS_ERR_OR_NULL which covers all possible return value of
> subvol_uuid_search.
> 
> Signed-off-by: Nikolay Borisov <[e-mail]>
> Reported-by: Chris Muprhy <[e-mail]>
> Link: https://lore.kernel.org/linux-btrfs/[e-mail]/


Qu pointed me to
https://patchwork.kernel.org/project/linux-btrfs/patch/[e-mail]/


which is essentially the same patch. So you can drop this.

Qu Wenruo Jan. 10, 2022, 11:30 a.m. UTC | #2

On 2022/1/10 19:12, Nikolay Borisov wrote:
> If subvol_uuid_search can't find the clone root then 'si' would either
> be NULL or contain an errno. The behavior of this function was
> changed as a result of commit
> ac5954ee36a5 ("btrfs-progs: merge subvol_uuid_search helpers"). Before
> this commit subvol_uuid_search() was a wrapper around subvol_uuid_search2
> and it guaranteed to either return well-fromed 'si' or NULL. This was
> sufficient for the check after the out label in process_clone.
>
> Properly handle this new semantic by changing the simple null check to
> IS_ERR_OR_NULL which covers all possible return value of
> subvol_uuid_search.
>
> Signed-off-by: Nikolay Borisov <[e-mail]>
> Reported-by: Chris Muprhy <[e-mail]>
> Link: https://lore.kernel.org/linux-btrfs/[e-mail]/

Reviewed-by: Qu Wenruo <[e-mail]>

Just curious that, can we refactor the helper to remove the NULL pointer
return case?

Thanks,
Qu
> ---
>   cmds/receive.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/cmds/receive.c b/cmds/receive.c
> index b4099bc482b2..16b9b4a853be 100644
> --- a/cmds/receive.c
> +++ b/cmds/receive.c
> @@ -794,7 +794,7 @@ static int process_clone(const char *path, u64 offset, u64 len,
>   	}
>
>   out:
> -	if (si) {
> +	if (!IS_ERR_OR_NULL(si)) {
>   		free(si->path);
>   		free(si);
>   	}
> --
> 2.17.1
>

Nikolay Borisov Jan. 10, 2022, 11:36 a.m. UTC | #3

On 10.01.22 г. 13:30, Qu Wenruo wrote:
> 
> Just curious that, can we refactor the helper to remove the NULL pointer
> return case?


Yes, the change is simple enough:

diff --git a/common/send-utils.c b/common/send-utils.c
index 4ed448b1f306..5002cfbbd0e8 100644
--- a/common/send-utils.c
+++ b/common/send-utils.c
@@ -308,10 +308,8 @@ struct subvol_info *subvol_uuid_search(int mnt_fd,
                goto out;

        info = calloc(1, sizeof(*info));
-       if (!info) {
-               ret = -ENOMEM;
-               goto out;
-       }
+       if (!info)
+               return ERR_PTR(-ENOMEM);
        info->root_id = root_id;
        memcpy(info->uuid, root_item.uuid, BTRFS_UUID_SIZE);
        memcpy(info->received_uuid, root_item.received_uuid,
BTRFS_UUID_SIZE);



However this only results in callers doing IS_ERR() rather than
IS_ERR_OR_NULL imo not much of a win.

David Sterba Jan. 11, 2022, 4:37 p.m. UTC | #4

On Mon, Jan 10, 2022 at 01:13:46PM +0200, Nikolay Borisov wrote:
> 
> 
> On 10.01.22 г. 13:12, Nikolay Borisov wrote:
> > If subvol_uuid_search can't find the clone root then 'si' would either
> > be NULL or contain an errno. The behavior of this function was
> > changed as a result of commit
> > ac5954ee36a5 ("btrfs-progs: merge subvol_uuid_search helpers"). Before
> > this commit subvol_uuid_search() was a wrapper around subvol_uuid_search2
> > and it guaranteed to either return well-fromed 'si' or NULL. This was
> > sufficient for the check after the out label in process_clone.
> > 
> > Properly handle this new semantic by changing the simple null check to
> > IS_ERR_OR_NULL which covers all possible return value of
> > subvol_uuid_search.
> > 
> > Signed-off-by: Nikolay Borisov <[e-mail]>
> > Reported-by: Chris Muprhy <[e-mail]>
> > Link: https://lore.kernel.org/linux-btrfs/[e-mail]/
> 
> 
> Qu pointed me to
> https://patchwork.kernel.org/project/linux-btrfs/patch/[e-mail]/
> 
> which is essentially the same patch. So you can drop this.

As suggested I've updated the changelog with yours as it's more
descriptive, thanks.

Patch

diff --git a/cmds/receive.c b/cmds/receive.c
index b4099bc482b2..16b9b4a853be 100644
--- a/cmds/receive.c
+++ b/cmds/receive.c
@@ -794,7 +794,7 @@  static int process_clone(const char *path, u64 offset, u64 len,
 	}

 out:
-	if (si) {
+	if (!IS_ERR_OR_NULL(si)) {
 		free(si->path);
 		free(si);
 	}