Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

zfs send does not handle invalid input gracefully #9001

Merged
merged 1 commit into from
Jul 8, 2019
Merged

zfs send does not handle invalid input gracefully #9001

merged 1 commit into from
Jul 8, 2019

Conversation

loli10K
Copy link
Contributor

@loli10K loli10K commented Jul 7, 2019

Motivation and Context

On master zfs send can crash when provided with invalid inputs:

root@linux:/usr/src/zfs# zfs send -R $POOLNAME/fs3 > /dev/null
Segmentation fault (core dumped)
root@linux:/usr/src/zfs# gdb -q zfs /var/crash/zfs.30818
Reading symbols from zfs...done.
[New LWP 30818]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Core was generated by `zfs send -R testpool/fs3'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x00005557b743c545 in zfs_do_send (argc=1, argv=0x5557b8083f08) at zfs_main.c:4340
4340		*cp = '\0';
(gdb) bt
#0  0x00005557b743c545 in zfs_do_send (argc=1, argv=0x5557b8083f08) at zfs_main.c:4340
#1  0x00005557b74347a8 in main (argc=4, argv=<optimized out>) at zfs_main.c:8260
(gdb) list 
4335			    "cannot be sent redacted.\n"));
4336			return (1);
4337		}
4338	
4339		cp = strchr(argv[0], '@');
4340		*cp = '\0';
4341		toname = cp + 1;
4342		zhp = zfs_open(g_zfs, argv[0], ZFS_TYPE_FILESYSTEM | ZFS_TYPE_VOLUME);
4343		if (zhp == NULL)
4344			return (1);
(gdb) p cp
$1 = 0x0
(gdb) p argv[0]
$2 = 0x5557b8083c60 "testpool/fs3"
(gdb)

Description

This change attempts to add more checks to the affected code paths.

How Has This Been Tested?

Run "zfs_send" test group on a local builder.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

@loli10K
zfs send does not handle invalid input gracefully
7feb7ac 
Due to some changes introduced in 30af21b 'zfs send' can crash when
provided with invalid inputs: this change attempts to add more checks
to the affected code paths.

Signed-off-by: loli10K <[email protected]>
AttilaFueloep
AttilaFueloep reviewed Jul 7, 2019
View reviewed changes
Copy link
Contributor

@AttilaFueloep AttilaFueloep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By coincidence I came up with nearly the same patch yesterday with just two differences

AttilaFueloep
AttilaFueloep reviewed Jul 7, 2019
View reviewed changes
lib/libzfs/libzfs_sendrecv.c
@@ -2422,6 +2422,10 @@ zfs_send(zfs_handle_t *zhp, const char *fromsnap, const char *tosnap,
}
zfs_handle_t *tosnap = zfs_open(zhp->zfs_hdl,
full_tosnap_name, ZFS_TYPE_SNAPSHOT);
if (tosnap == NULL) {
err = -1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

err = EZFS_BADPATH;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

err = EZFS_BADPATH;

This is not necessarily true, zfs_open() can fail for many different reasons, not just invalid input.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats right. IRC this was how the old code handled it. Would have to look again to be sure though. Nonetheless I'm fine either way.

AttilaFueloep
AttilaFueloep reviewed Jul 7, 2019
View reviewed changes
lib/libzfs/libzfs_sendrecv.c
@@ -2707,6 +2711,8 @@ zfs_send_one(zfs_handle_t *zhp, const char *from, int fd, sendflags_t *flags,
if (from != NULL && strchr(from, '@')) {
zfs_handle_t *from_zhp = zfs_open(hdl, from,
ZFS_TYPE_DATASET);
if (from_zhp == NULL)
return (-1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return (EZFS_NOENT);

@codecov
Copy link

codecov bot commented Jul 8, 2019 •
edited
Loading

Codecov Report

Merging #9001 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9001      +/-   ##
==========================================
- Coverage   78.62%   78.59%   -0.04%     
==========================================
  Files         388      388              
  Lines      119992   119997       +5     
==========================================
- Hits        94349    94307      -42     
- Misses      25643    25690      +47
Flag Coverage Δ
#kernel 79.44% <ø> (ø) ⬆️
#user 66.29% <100%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1086f54...7feb7ac. Read the comment docs.

@AttilaFueloep
Copy link
Contributor

As a side note, maybe we should add a regression test like so AttilaFueloep@d400ce1 ?

@AttilaFueloep AttilaFueloep mentioned this pull request Jul 8, 2019
Merged
12 tasks
@loli10K loli10K marked this pull request as ready for review July 8, 2019 18:41
@loli10K loli10K added the Status: Code Review Needed Ready for review and testing label Jul 8, 2019
behlendorf
behlendorf approved these changes Jul 8, 2019
View reviewed changes
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I'd be OK with extending the test cases, but I don't think it's strictly necessarily since if #9003 has been applied the CI would have caught this prior to merging the feature.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jul 8, 2019
@behlendorf behlendorf merged commit 1d20b76 into openzfs:master Jul 8, 2019
TulsiJain pushed a commit to TulsiJain/zfs that referenced this pull request Jul 20, 2019
@loli10K
zfs send does not handle invalid input gracefully
2647d32 
Due to some changes introduced in 30af21b 'zfs send' can crash when
provided with invalid inputs: this change attempts to add more checks
to the affected code paths.

Reviewed-by: Attila Fülöp <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Tony Hutter <[email protected]>
Signed-off-by: loli10K <[email protected]>
Closes openzfs#9001
TulsiJain pushed a commit to TulsiJain/zfs that referenced this pull request Jul 20, 2019
@loli10K
zfs send does not handle invalid input gracefully
6a8a06c 
Due to some changes introduced in 30af21b 'zfs send' can crash when
provided with invalid inputs: this change attempts to add more checks
to the affected code paths.

Reviewed-by: Attila Fülöp <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Tony Hutter <[email protected]>
Signed-off-by: loli10K <[email protected]>
Closes openzfs#9001
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AttilaFueloep AttilaFueloep AttilaFueloep left review comments

@tonyhutter tonyhutter tonyhutter approved these changes

@behlendorf behlendorf behlendorf approved these changes

@pcd1193182 pcd1193182 Awaiting requested review from pcd1193182

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@loli10K @AttilaFueloep @behlendorf @tonyhutter