Don't modify argv[] in user tools #7760

DeHackEd · 2018-08-01T21:38:55Z

argv[] gets modified during string parsing for input arguments. This
is reflected in the live process listing. Don't do that.

Signed-off-by: DHE git@dehacked.net

Motivation and Context

ps output of zfs send currently never includes any @ characters while working because the in-memory commandline has been edited. Other commands have similar missing characters.

Description

alloca() + strcpy() the strings, edit those instead.

How Has This Been Tested?

Various manual runs with valgrind --leak-check=full

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:

My code follows the ZFS on Linux code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
All new and existing tests passed.
All commit messages are properly formatted and contain Signed-off-by.
Change has been approved by a ZFS on Linux member.

behlendorf · 2018-08-15T19:19:30Z

cmd/zfs/zfs_main.c

 	char *fromname = NULL;
 	char *toname = NULL;
+	char *dataset = NULL;


In order to simply the error handling I think it would be reasonable to put the dataset and fromname on the stack. Since this code always runs in user space we don't have to worry about the kernel's tiny stacks. You can could do something like.

char fromdup[ZFS_MAX_DATASET_NAME_LEN]; ... (void) strlcpy(fromdup, optarg, sizeof (fromdup)); fromname = &fromdup;

What do you think about using alloca(strlen(fromname)) instead for a dynamic allocation still on the stack?

Edit: on second thought, never mind. Your method is used in other places in this function. Best to be consistent.

behlendorf · 2018-08-15T20:03:24Z

cmd/zfs/zfs_main.c

@@ -4145,16 +4149,17 @@ zfs_do_send(int argc, char **argv)
 			fromname = frombuf;
 		}
 		err = zfs_send_one(zhp, fromname, STDOUT_FILENO, flags);
+		free(fromname);


This could free frombuf which is on the stack. This might be what caused the ZTS failure.

DeHackEd · 2018-08-16T01:07:04Z

I ended up using something of a mixture of alloca() and fixed stack arrays in an attempt to minimize the number of changes.

Also replaced various instances of argv[0] with dataset for consistency and rebased to master.

loli10K · 2018-08-16T05:00:20Z

Motivation and Context
ps output of zfs send currently never includes any @ characters while working because the in-memory commandline has been edited.

This same issue seems to be affecting other zfs sub-commands (e.g. snapshot) and other characters as well (e.g. create, set are missing = in the commandline)

codecov · 2018-08-16T06:16:13Z

Codecov Report

Merging #7760 into master will decrease coverage by 0.16%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #7760      +/-   ##
==========================================
- Coverage   78.32%   78.15%   -0.17%     
==========================================
  Files         373      373              
  Lines      112791   112805      +14     
==========================================
- Hits        88341    88168     -173     
- Misses      24450    24637     +187

Flag	Coverage Δ
#kernel	`78.45% <ø> (-0.09%)`	⬇️
#user	`66.92% <100%> (-0.61%)`	⬇️

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 802715b...4bdc1ec. Read the comment docs.

DeHackEd · 2018-08-16T10:36:01Z

Interesting. I didn't check that because zfs send is the only command I've noticed since it runs for extended periods.

Maybe main() should just give functions string duplicates of the original argv... :/

behlendorf · 2018-08-16T18:15:36Z

Maybe main() should just give functions string duplicates of the original argv...

That's not a bad idea. I suspect there's more existing code which makes this assumption and it'd be nice to handle this all in one place. We'd want to update main() in both zfs and zpool.

DeHackEd · 2018-08-17T01:43:51Z

Updated to just duplicate argv[] entirely on the stack and pass the duplicate to all functions instead.

This changes the intent of the commit as a whole while still meeting the same goals.

DeHackEd · 2018-08-17T09:43:52Z

Last round of buildbot failures seem related to a github outage last night. Pushing a noop rebase to make them try again.

sdimitro · 2018-08-17T21:57:52Z

cmd/zfs/zfs_main.c

+	 */
+	newargv = alloca((argc + 1) * sizeof (newargv[0]));
+	for (i = 0; i < argc; i++) {
+		newargv[i] = alloca(strlen(argv[i] + 1));


Hi there! Thanks for working on this change.

Maybe we could change this:

for (i = 0; i < argc; i++) { newargv[i] = alloca(strlen(argv[i] + 1)); strcpy(newargv[i], argv[i]); }

to this:

for (i = 0; i < argc; i++) { newargv[i] = strdupa(argv[i]); }

(in both cases where it is used)

To be honest though, I'm not sure how to feel about alloca() being
used as it is both platform and compiler dependent. Do we actually
get any wins from using it?

Stack allocations are extremely fast don't have to be free()'d at the end of a program since they are effectively freed on return from main(). The alternative with regular strdup() is to call free() afterwards just for the sake of cleanliness. Not that big a deal but we have an alternative that, at least in the context of Linux, works equally as well.

Though platform and compiler dependent, it is well supported on gcc and the Linux man page implies it will be just fine on BSD as well. strdupa is noted as strictly a gcc thing whereas it seems alloca has a somewhat wider range of support. Heck I see an equivalent on MSDN. If alloca is not practical I'd go back to normal strdup/free.

I have to admit I'm a bit skittish about using alloca() too. I agree that it should be portable, and it shouldn't cause any issues. But I'd prefer to be conservative and avoid using it unless there's a meaningful win to be had. Let's follow the wise advice in the man page.

The alloca() function is machine- and compiler-dependent. For certain
applications, its use can improve efficiency compared to the use of
malloc(3) plus free(3). In certain cases, it can also simplify memory
deallocation in applications that use longjmp(3) or siglongjmp(3).
Otherwise, its use is discouraged.

Plus this would be the only use of alloca(3) in the entire ZFS code base. Why add another dependency unless we have too.

argv[] gets modified during string parsing for input arguments. This is reflected in the live process listing. Don't do that. Signed-off-by: DHE <git@dehacked.net>

DeHackEd · 2018-08-18T00:30:04Z

strdup() it is then.

argv[] gets modified during string parsing for input arguments. This is reflected in the live process listing. Don't do that. Reviewed-by: Serapheim Dimitropoulos <serapheim@delphix.com> Reviewed-by: loli10K <ezomori.nozomu@gmail.com> Reviewed-by: Giuseppe Di Natale <guss80@gmail.com> Reviewed-by: George Melikov <mail@gmelikov.ru> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: DHE <git@dehacked.net> Closes openzfs#7760

ahrens requested a review from pcd1193182 August 2, 2018 18:16

behlendorf self-requested a review August 13, 2018 15:25

behlendorf requested changes Aug 15, 2018

View reviewed changes

DeHackEd force-pushed the zfs_send_edits_argv branch 2 times, most recently from 2a87f6a to 0322a85 Compare August 16, 2018 01:06

DeHackEd closed this Aug 17, 2018

DeHackEd force-pushed the zfs_send_edits_argv branch from 0322a85 to 4338c5c Compare August 17, 2018 01:35

DeHackEd reopened this Aug 17, 2018

DeHackEd force-pushed the zfs_send_edits_argv branch from 921f4da to 211f842 Compare August 17, 2018 09:43

DeHackEd changed the title ~~zfs send: don't modify argv[]~~ Don't modify argv[] in user tools Aug 17, 2018

sdimitro reviewed Aug 17, 2018

View reviewed changes

Don't modify argv[] in user tools

4bdc1ec

argv[] gets modified during string parsing for input arguments. This is reflected in the live process listing. Don't do that. Signed-off-by: DHE <git@dehacked.net>

DeHackEd force-pushed the zfs_send_edits_argv branch from 211f842 to 4bdc1ec Compare August 17, 2018 23:01

behlendorf approved these changes Aug 18, 2018

View reviewed changes

behlendorf requested a review from loli10K August 18, 2018 23:51

sdimitro approved these changes Aug 19, 2018

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) Reviewed labels Aug 19, 2018

loli10K approved these changes Aug 19, 2018

View reviewed changes

gmelikov approved these changes Aug 19, 2018

View reviewed changes

dinatale2 approved these changes Aug 19, 2018

View reviewed changes

behlendorf merged commit edc05fd into openzfs:master Aug 20, 2018

tonyhutter added this to To do in 0.7.10 Aug 27, 2018

tonyhutter moved this from To do to In progress in 0.7.10 Aug 30, 2018

tonyhutter moved this from In progress to Done in 0.7.10 Aug 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't modify argv[] in user tools #7760

Don't modify argv[] in user tools #7760

DeHackEd commented Aug 1, 2018 •

edited

behlendorf Aug 15, 2018

DeHackEd Aug 15, 2018 •

edited

behlendorf Aug 15, 2018

DeHackEd commented Aug 16, 2018

loli10K commented Aug 16, 2018

codecov bot commented Aug 16, 2018 •

edited

DeHackEd commented Aug 16, 2018

behlendorf commented Aug 16, 2018

DeHackEd commented Aug 17, 2018

DeHackEd commented Aug 17, 2018

sdimitro Aug 17, 2018

DeHackEd Aug 17, 2018

behlendorf Aug 17, 2018

DeHackEd commented Aug 18, 2018

Don't modify argv[] in user tools #7760

Don't modify argv[] in user tools #7760

Conversation

DeHackEd commented Aug 1, 2018 • edited

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

behlendorf Aug 15, 2018

Choose a reason for hiding this comment

DeHackEd Aug 15, 2018 • edited

Choose a reason for hiding this comment

behlendorf Aug 15, 2018

Choose a reason for hiding this comment

DeHackEd commented Aug 16, 2018

loli10K commented Aug 16, 2018

codecov bot commented Aug 16, 2018 • edited

Codecov Report

DeHackEd commented Aug 16, 2018

behlendorf commented Aug 16, 2018

DeHackEd commented Aug 17, 2018

DeHackEd commented Aug 17, 2018

sdimitro Aug 17, 2018

Choose a reason for hiding this comment

DeHackEd Aug 17, 2018

Choose a reason for hiding this comment

behlendorf Aug 17, 2018

Choose a reason for hiding this comment

DeHackEd commented Aug 18, 2018

DeHackEd commented Aug 1, 2018 •

edited

DeHackEd Aug 15, 2018 •

edited

codecov bot commented Aug 16, 2018 •

edited