Fixed some issues found by static analyzers #2634

lnykryn · 2024-03-06T11:21:39Z

No description provided.

Error: RESOURCE_LEAK (CWE-772): src/util/util.c:188: alloc_fn: Storage is returned from allocation function "strdup". src/util/util.c:188: var_assign: Assigning: "cmdline" = storage returned from "strdup(p)". src/util/util.c:209: identity_transfer: Passing "cmdline" as argument 1 to function "next_arg", which returns an offset off that argument. src/util/util.c:209: identity_transfer: Passing "cmdline" as argument 1 to function "next_arg", which sets "key" to an offset off that argument. src/util/util.c:209: identity_transfer: Passing "cmdline" as argument 1 to function "next_arg", which sets "value" to an offset off that argument. src/util/util.c:209: var_assign: Assigning: "cmdline" = storage returned from "next_arg(cmdline, &key, &value)". src/util/util.c:210: noescape: Resource "key" is not freed or pointed-to in "strcmp". src/util/util.c:219: leaked_storage: Variable "value" going out of scope leaks the storage it points to. src/util/util.c:219: leaked_storage: Variable "key" going out of scope leaks the storage it points to. src/util/util.c:209: identity_transfer: Passing "cmdline" as argument 1 to function "next_arg", which returns an offset off that argument. src/util/util.c:209: identity_transfer: Passing "cmdline" as argument 1 to function "next_arg", which sets "key" to an offset off that argument. src/util/util.c:209: identity_transfer: Passing "cmdline" as argument 1 to function "next_arg", which sets "value" to an offset off that argument. src/util/util.c:209: var_assign: Assigning: "cmdline" = storage returned from "next_arg(cmdline, &key, &value)". src/util/util.c:210: noescape: Resource "key" is not freed or pointed-to in "strcmp". src/util/util.c:219: leaked_storage: Variable "value" going out of scope leaks the storage it points to. src/util/util.c:219: leaked_storage: Variable "key" going out of scope leaks the storage it points to. src/util/util.c:238: leaked_storage: Variable "cmdline" going out of scope leaks the storage it points to.

Error: OVERRUN (CWE-119): src/install/dracut-install.c:464: identity_transfer: Passing "4097UL" as argument 3 to function "readlink", which returns that argument. [Note: The source code implementation of the function has been overridden by a builtin model.] src/install/dracut-install.c:464: assignment: Assigning: "linksz" = "readlink(fullsrcpath, linktarget, 4097UL)". The value of "linksz" is now 4097. src/install/dracut-install.c:467: overrun-local: Overrunning array "linktarget" of 4097 bytes at byte offset 4097 using index "linksz" (which evaluates to 4097). 465| if (linksz < 0) 466| return NULL; 467|-> linktarget[linksz] = '\0'; 468| 469| log_debug("get_real_file: readlink('%s') returns '%s'", fullsrcpath, linktarget);

"Error: RESOURCE_LEAK (CWE-772): src/util/util.c:252: alloc_fn: Storage is returned from allocation function ""strdup"". src/util/util.c:252: var_assign: Assigning: ""cmdline"" = storage returned from ""strdup(p)"". src/util/util.c:273: identity_transfer: Passing ""cmdline"" as argument 1 to function ""next_arg"", which returns an offset off that argument. src/util/util.c:273: identity_transfer: Passing ""cmdline"" as argument 1 to function ""next_arg"", which sets ""key"" to an offset off that argument. src/util/util.c:273: identity_transfer: Passing ""cmdline"" as argument 1 to function ""next_arg"", which sets ""value"" to an offset off that argument. src/util/util.c:273: var_assign: Assigning: ""cmdline"" = storage returned from ""next_arg(cmdline, &key, &value)"". src/util/util.c:274: noescape: Resource ""key"" is not freed or pointed-to in ""strcmp"". src/util/util.c:289: leaked_storage: Variable ""value"" going out of scope leaks the storage it points to. src/util/util.c:289: leaked_storage: Variable ""key"" going out of scope leaks the storage it points to. src/util/util.c:273: identity_transfer: Passing ""cmdline"" as argument 1 to function ""next_arg"", which returns an offset off that argument. src/util/util.c:273: identity_transfer: Passing ""cmdline"" as argument 1 to function ""next_arg"", which sets ""key"" to an offset off that argument. src/util/util.c:273: identity_transfer: Passing ""cmdline"" as argument 1 to function ""next_arg"", which sets ""value"" to an offset off that argument. src/util/util.c:273: var_assign: Assigning: ""cmdline"" = storage returned from ""next_arg(cmdline, &key, &value)"". src/util/util.c:274: noescape: Resource ""key"" is not freed or pointed-to in ""strcmp"". src/util/util.c:289: leaked_storage: Variable ""value"" going out of scope leaks the storage it points to. src/util/util.c:289: leaked_storage: Variable ""key"" going out of scope leaks the storage it points to. src/util/util.c:290: leaked_storage: Variable ""cmdline"" going out of scope leaks the storage it points to. 288| } 289| } while (cmdline[0]); 290|-> return found_value ? EXIT_SUCCESS : EXIT_FAILURE; 291| } 292| "

"Error: RESOURCE_LEAK (CWE-772): src/install/dracut-install.c:1135: alloc_fn: Storage is returned from allocation function ""strv_split"". src/install/dracut-install.c:1135: var_assign: Assigning: ""firmwaredirs"" = storage returned from ""strv_split(optarg, "":"")"". src/install/dracut-install.c:1135: overwrite_var: Overwriting ""firmwaredirs"" in ""firmwaredirs = strv_split(optarg, "":"")"" leaks the storage that ""firmwaredirs"" points to. 1133| break; 1134| case ARG_FIRMWAREDIRS: 1135|-> firmwaredirs = strv_split(optarg, "":""); 1136| break; 1137| case 'f':"

dtardon · 2024-03-07T07:40:06Z

src/util/util.c

@@ -253,16 +253,15 @@ static int getargs(int argc, char **argv)
        char *search_value;
        bool found_value = false;
        char *cmdline = NULL;
+        char *cmdline_iter = NULL;


Merge this into the preceding line?

dtardon · 2024-03-07T07:47:54Z

src/util/util.c

@@ -204,9 +204,16 @@ static int getarg(int argc, char **argv)
        if (strlen(search_key) == 0)
                usage(GETARG, EXIT_FAILURE, "search key undefined");

+        cmdline = strdup(p);


Hmm, I see no reason to copy p in the first place... The function could just use it directly, i.e., initialize cmdline (the iterator) simply as cmdline = p. Or have I overlooked something?

I have... next_arg() modifies the string, so it must be copied. So scratch this.

dtardon · 2024-03-07T07:52:39Z

src/util/util.c

@@ -180,12 +180,12 @@ static int getarg(int argc, char **argv)
        char *end_value = NULL;
        bool bool_value = false;
        char *cmdline = NULL;
+        char *cmdline_iter = NULL;


Merge this into the preceding line?

Also, the var doesn't have to be initialized.

LaszloGombos · 2024-03-07T14:58:33Z

CI tests seems to be failing with

free(): invalid pointer

I have not had a chance to investigate yet.

dtardon · 2024-03-12T07:09:40Z

src/util/util.c

        do {
                char *key = NULL, *value = NULL;
-                cmdline = next_arg(cmdline, &key, &value);
+                cmdline = next_arg(cmdline_iter, &key, &value);


cmdline_iter = ...

dtardon · 2024-03-12T07:10:12Z

src/util/util.c

@@ -180,12 +180,12 @@ static int getarg(int argc, char **argv)
        char *end_value = NULL;
        bool bool_value = false;
        char *cmdline = NULL;
+        char *cmdline_iter = NULL;


Also, the var doesn't have to be initialized.

dtardon · 2024-03-12T07:10:50Z

src/util/util.c

        do {
                char *key = NULL, *value = NULL;
-                cmdline = next_arg(cmdline, &key, &value);
+                cmdline = next_arg(cmdline_iter, &key, &value);


cmdline_iter = ...

dtardon · 2024-03-12T07:12:38Z

src/util/util.c

-        } while (cmdline[0]);
+        } while (cmdline_iter[0]);
+
+        free(cmdline_iter);


free(cmdline)

dtardon · 2024-03-12T07:15:59Z

src/install/dracut-install.c

@@ -1140,6 +1140,7 @@ static int parse_argv(int argc, char *argv[])
                        kerneldir = optarg;
                        break;
                case ARG_FIRMWAREDIRS:
+                        strv_free(firmwaredirs);
                        firmwaredirs = strv_split(optarg, ":");


Not related to the fix, but maybe add a check that strv_split() succeeded?

stale · 2024-04-22T06:46:12Z

This issue is being marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. If this is still an issue in the latest release of Dracut and you would like to keep it open please comment on this issue within the next 7 days. Thank you for your contributions.

lnykryn requested review from haraldh and johannbg as code owners March 6, 2024 11:21

github-actions bot added dracut-install Issues related to dracut install dracut-util labels Mar 6, 2024

lnykryn added 4 commits March 6, 2024 12:26

lnykryn force-pushed the sast branch from 403c2a8 to 9d1742a Compare March 6, 2024 11:27

dtardon reviewed Mar 7, 2024

View reviewed changes

dtardon suggested changes Mar 12, 2024

View reviewed changes

stale bot added the stale communication is stuck label Apr 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed some issues found by static analyzers #2634

Fixed some issues found by static analyzers #2634

lnykryn commented Mar 6, 2024

dtardon Mar 7, 2024 •

edited

Loading

dtardon Mar 7, 2024

dtardon Mar 7, 2024

dtardon Mar 7, 2024

dtardon Mar 12, 2024

LaszloGombos commented Mar 7, 2024

dtardon Mar 12, 2024

dtardon Mar 12, 2024

dtardon Mar 12, 2024

dtardon Mar 12, 2024

dtardon Mar 12, 2024

stale bot commented Apr 22, 2024

Fixed some issues found by static analyzers #2634

Are you sure you want to change the base?

Fixed some issues found by static analyzers #2634

Conversation

lnykryn commented Mar 6, 2024

dtardon Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LaszloGombos commented Mar 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Apr 22, 2024

dtardon Mar 7, 2024 •

edited

Loading