| Message ID | 20221208011122.2343363-4-jesse.brandeburg@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Michal Kubecek |
| Headers | show |
| Series | ethtool: clean up and fix | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Wed, Dec 07, 2022 at 05:11:12PM -0800, Jesse Brandeburg wrote: > After testing with this code in the debugger, it is technically possible > to pass a NULL argument to ethtool which then prods it to call strncmp > with a NULL value, which triggers this warning: > > Description: Null pointer passed to 1st parameter expecting 'nonnull' > File: /git/ethtool/ethtool.c > Line: 6129 > > Since segfaults are bad, let's just add a check for NULL when parsing > the initial arguments. The other cases of a NULL option are seemingly > handled. > > Fixes: 127f80691f96 ("Move argument parsing to sub-command functions") > Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com> Learning about (kernel) mainline commit dcd46d897adb ("exec: Force single empty string when argv is empty"), I'm not opposed to mistrusting kernel provided argc and argv[] but rather than hunting all places where we might possibly hit a null pointer in argv[] (mind that we may iterate over argv[] up to three times), I would simply check the whole array at the beginning of main() with something like k = 0; while (k <= argc && argv[k]) k++; if (k != argc) { fprintf(stderr, "ethtool: inconsistent command line (OS bug?)\n"); return 1; } and be done with it. Michal > --- > ethtool.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/ethtool.c b/ethtool.c > index 3207e49137c4..a72577b32601 100644 > --- a/ethtool.c > +++ b/ethtool.c > @@ -6389,7 +6389,7 @@ int main(int argc, char **argp) > * name to get settings for (which we don't expect to begin > * with '-'). > */ > - if (argc == 0) > + if (argc == 0 || *argp == NULL) > exit_bad_args(); > > k = find_option(*argp); > -- > 2.31.1 >
diff --git a/ethtool.c b/ethtool.c index 3207e49137c4..a72577b32601 100644 --- a/ethtool.c +++ b/ethtool.c @@ -6389,7 +6389,7 @@ int main(int argc, char **argp) * name to get settings for (which we don't expect to begin * with '-'). */ - if (argc == 0) + if (argc == 0 || *argp == NULL) exit_bad_args(); k = find_option(*argp);
After testing with this code in the debugger, it is technically possible to pass a NULL argument to ethtool which then prods it to call strncmp with a NULL value, which triggers this warning: Description: Null pointer passed to 1st parameter expecting 'nonnull' File: /git/ethtool/ethtool.c Line: 6129 Since segfaults are bad, let's just add a check for NULL when parsing the initial arguments. The other cases of a NULL option are seemingly handled. Fixes: 127f80691f96 ("Move argument parsing to sub-command functions") Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com> --- ethtool.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)