Merge pull request systemd#19243 from bluca/lgtm

keszybz · web-flow · commit 22344fcfb989 · 2021-04-09T13:12:41.000+02:00
Fix various issues reported by LGTM
diff --git a/src/basic/errno-util.h b/src/basic/errno-util.h
@@ -33,7 +33,7 @@ static inline int negative_errno(void) {
 
 static inline const char *strerror_safe(int error) {
         /* 'safe' here does NOT mean thread safety. */
-        return strerror(abs(error));
+        return strerror(abs(error)); /* lgtm [cpp/potentially-dangerous-function] */
 }
 
 static inline int errno_or_else(int fallback) {
diff --git a/src/basic/in-addr-util.c b/src/basic/in-addr-util.c
@@ -51,7 +51,7 @@ bool in4_addr_is_link_local(const struct in_addr *a) {
 bool in6_addr_is_link_local(const struct in6_addr *a) {
         assert(a);
 
-        return IN6_IS_ADDR_LINKLOCAL(a);
+        return IN6_IS_ADDR_LINKLOCAL(a); /* lgtm [cpp/potentially-dangerous-function] */
 }
 
 int in_addr_is_link_local(int family, const union in_addr_union *u) {
@@ -116,7 +116,7 @@ int in_addr_is_localhost(int family, const union in_addr_union *u) {
                 return in4_addr_is_localhost(&u->in);
 
         if (family == AF_INET6)
-                return IN6_IS_ADDR_LOOPBACK(&u->in6);
+                return IN6_IS_ADDR_LOOPBACK(&u->in6); /* lgtm [cpp/potentially-dangerous-function] */
 
         return -EAFNOSUPPORT;
 }
diff --git a/src/test/test-extract-word.c b/src/test/test-extract-word.c
@@ -428,6 +428,20 @@ static void test_extract_first_word(void) {
         assert_se(streq(t, "c"));
         free(t);
         assert_se(p == NULL);
+
+        p = original = "foobar=\"waldo\"maldo, baldo";
+        assert_se(extract_first_word(&p, &t, "=\", ", 0) > 0);
+        assert_se(streq(t, "foobar"));
+        free(t);
+        assert_se(extract_first_word(&p, &t, "=\", ", 0) > 0);
+        assert_se(streq(t, "waldo"));
+        free(t);
+        assert_se(extract_first_word(&p, &t, "=\", ", 0) > 0);
+        assert_se(streq(t, "maldo"));
+        free(t);
+        assert_se(extract_first_word(&p, &t, "=\", ", 0) > 0);
+        assert_se(streq(t, "baldo"));
+        free(t);
 }
 
 static void test_extract_first_word_and_warn(void) {
diff --git a/src/timedate/timedated.c b/src/timedate/timedated.c
@@ -801,6 +801,7 @@ static int method_set_local_rtc(sd_bus_message *m, void *userdata, sd_bus_error
 
 static int method_set_time(sd_bus_message *m, void *userdata, sd_bus_error *error) {
         sd_bus *bus = sd_bus_message_get_bus(m);
+        char buf[FORMAT_TIMESTAMP_MAX];
         int relative, interactive, r;
         Context *c = userdata;
         int64_t utc;
@@ -886,7 +887,7 @@ static int method_set_time(sd_bus_message *m, void *userdata, sd_bus_error *erro
         log_struct(LOG_INFO,
                    "MESSAGE_ID=" SD_MESSAGE_TIME_CHANGE_STR,
                    "REALTIME="USEC_FMT, timespec_load(&ts),
-                   LOG_MESSAGE("Changed local time to %s", ctime(&ts.tv_sec)));
+                   LOG_MESSAGE("Changed local time to %s", strnull(format_timestamp(buf, sizeof(buf), timespec_load(&ts)))));
 
         return sd_bus_reply_method_return(m, NULL);
 }
diff --git a/src/udev/scsi_id/scsi_id.c b/src/udev/scsi_id/scsi_id.c
@@ -19,9 +19,11 @@
 #include "alloc-util.h"
 #include "build.h"
 #include "device-nodes.h"
+#include "extract-word.h"
 #include "fd-util.h"
 #include "scsi_id.h"
 #include "string-util.h"
+#include "strv.h"
 #include "strxcpyx.h"
 #include "udev-util.h"
 
@@ -90,50 +92,6 @@ static void set_type(const char *from, char *to, size_t len) {
         strscpy(to, len, type);
 }
 
-/*
- * get_value:
- *
- * buf points to an '=' followed by a quoted string ("foo") or a string ending
- * with a space or ','.
- *
- * Return a pointer to the NUL terminated string, returns NULL if no
- * matches.
- */
-static char *get_value(char **buffer) {
-        static const char *quote_string = "\"\n";
-        static const char *comma_string = ",\n";
-        char *val;
-        const char *end;
-
-        if (**buffer == '"') {
-                /*
-                 * skip leading quote, terminate when quote seen
-                 */
-                (*buffer)++;
-                end = quote_string;
-        } else
-                end = comma_string;
-        val = strsep(buffer, end);
-        if (val && end == quote_string)
-                /*
-                 * skip trailing quote
-                 */
-                (*buffer)++;
-
-        while (isspace(**buffer))
-                (*buffer)++;
-
-        return val;
-}
-
-static int argc_count(char *opts) {
-        int i = 0;
-        while (*opts != '\0')
-                if (*opts++ == ' ')
-                        i++;
-        return i;
-}
-
 /*
  * get_file_options:
  *
@@ -145,14 +103,12 @@ static int argc_count(char *opts) {
  */
 static int get_file_options(const char *vendor, const char *model,
                             int *argc, char ***newargv) {
-        _cleanup_free_ char *buffer = NULL;
+        _cleanup_free_ char *buffer = NULL,
+                *vendor_in = NULL, *model_in = NULL, *options_in = NULL; /* read in from file */
+        _cleanup_strv_free_ char **options_argv = NULL;
         _cleanup_fclose_ FILE *f;
-        char *buf;
-        char *str1;
-        char *vendor_in, *model_in, *options_in; /* read in from file */
-        int lineno;
-        int c;
-        int retval = 0;
+        const char *buf;
+        int lineno, r;
 
         f = fopen(config_file, "re");
         if (!f) {
@@ -176,16 +132,16 @@ static int get_file_options(const char *vendor, const char *model,
         *newargv = NULL;
         lineno = 0;
         for (;;) {
+                _cleanup_free_ char *key = NULL, *value = NULL;
+
                 vendor_in = model_in = options_in = NULL;
 
                 buf = fgets(buffer, MAX_BUFFER_LEN, f);
                 if (!buf)
                         break;
                 lineno++;
-                if (buf[strlen(buffer) - 1] != '\n') {
-                        log_error("Config file line %d too long", lineno);
-                        break;
-                }
+                if (buf[strlen(buffer) - 1] != '\n')
+                        return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Config file line %d too long", lineno);
 
                 while (isspace(*buf))
                         buf++;
@@ -198,44 +154,36 @@ static int get_file_options(const char *vendor, const char *model,
                 if (*buf == '#')
                         continue;
 
-                str1 = strsep(&buf, "=");
-                if (str1 && strcaseeq(str1, "VENDOR")) {
-                        str1 = get_value(&buf);
-                        if (!str1) {
-                                retval = log_oom();
-                                break;
-                        }
-                        vendor_in = str1;
-
-                        str1 = strsep(&buf, "=");
-                        if (str1 && strcaseeq(str1, "MODEL")) {
-                                str1 = get_value(&buf);
-                                if (!str1) {
-                                        retval = log_oom();
-                                        break;
-                                }
-                                model_in = str1;
-                                str1 = strsep(&buf, "=");
-                        }
-                }
+                r = extract_many_words(&buf, "=\",\n", 0, &key, &value, NULL);
+                if (r < 2)
+                        return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Error parsing config file line %d '%s'", lineno, buffer);
 
-                if (str1 && strcaseeq(str1, "OPTIONS")) {
-                        str1 = get_value(&buf);
-                        if (!str1) {
-                                retval = log_oom();
-                                break;
+                if (strcaseeq(key, "VENDOR")) {
+                        vendor_in = TAKE_PTR(value);
+
+                        key = mfree(key);
+                        r = extract_many_words(&buf, "=\",\n", 0, &key, &value, NULL);
+                        if (r < 2)
+                                return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Error parsing config file line %d '%s'", lineno, buffer);
+
+                        if (strcaseeq(key, "MODEL")) {
+                                model_in = TAKE_PTR(value);
+
+                                key = mfree(key);
+                                r = extract_many_words(&buf, "=\",\n", 0, &key, &value, NULL);
+                                if (r < 2)
+                                        return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Error parsing config file line %d '%s'", lineno, buffer);
                         }
-                        options_in = str1;
                 }
 
+                if (strcaseeq(key, "OPTIONS"))
+                        options_in = TAKE_PTR(value);
+
                 /*
                  * Only allow: [vendor=foo[,model=bar]]options=stuff
                  */
-                if (!options_in || (!vendor_in && model_in)) {
-                        log_error("Error parsing config file line %d '%s'", lineno, buffer);
-                        retval = -1;
-                        break;
-                }
+                if (!options_in || (!vendor_in && model_in))
+                        return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Error parsing config file line %d '%s'", lineno, buffer);
                 if (!vendor) {
                         if (!vendor_in)
                                 break;
@@ -251,39 +199,30 @@ static int get_file_options(const char *vendor, const char *model,
                                  */
                                 break;
                 }
-        }
 
-        if (retval == 0) {
-                if (vendor_in != NULL || model_in != NULL ||
-                    options_in != NULL) {
-                        /*
-                         * Something matched. Allocate newargv, and store
-                         * values found in options_in.
-                         */
-                        strcpy(buffer, options_in);
-                        c = argc_count(buffer) + 2;
-                        *newargv = calloc(c, sizeof(**newargv));
-                        if (!*newargv)
-                                retval = log_oom();
-                        else {
-                                *argc = c;
-                                c = 0;
-                                /*
-                                 * argv[0] at 0 is skipped by getopt, but
-                                 * store the buffer address there for
-                                 * later freeing
-                                 */
-                                (*newargv)[c] = buffer;
-                                for (c = 1; c < *argc; c++)
-                                        (*newargv)[c] = strsep(&buffer, " \t");
-                                buffer = NULL;
-                        }
-                } else {
-                        /* No matches  */
-                        retval = 1;
-                }
+                vendor_in = mfree(vendor_in);
+                model_in = mfree(model_in);
+                options_in = mfree(options_in);
+
         }
-        return retval;
+
+        if (vendor_in == NULL && model_in == NULL && options_in == NULL)
+                return 1; /* No matches  */
+
+        /*
+        * Something matched. Allocate newargv, and store
+        * values found in options_in.
+        */
+        options_argv = strv_split(options_in, " \t");
+        if (!options_argv)
+                return log_oom();
+        r = strv_prepend(&options_argv, ""); /* getopt skips over argv[0] */
+        if (r < 0)
+                return r;
+        *newargv = TAKE_PTR(options_argv);
+        *argc = strv_length(*newargv);
+
+        return 0;
 }
 
 static void help(void) {
@@ -391,9 +330,9 @@ static int set_options(int argc, char **argv,
 }
 
 static int per_dev_options(struct scsi_id_device *dev_scsi, int *good_bad, int *page_code) {
+        _cleanup_strv_free_ char **newargv = NULL;
         int retval;
         int newargc;
-        char **newargv = NULL;
         int option;
 
         *good_bad = all_good;
@@ -436,10 +375,6 @@ static int per_dev_options(struct scsi_id_device *dev_scsi, int *good_bad, int *
                 }
         }
 
-        if (newargv) {
-                free(newargv[0]);
-                free(newargv);
-        }
         return retval;
 }
 
@@ -543,10 +478,10 @@ static int scsi_id(char *maj_min_dev) {
 }
 
 int main(int argc, char **argv) {
+        _cleanup_strv_free_ char **newargv = NULL;
         int retval = 0;
         char maj_min_dev[MAX_PATH_LEN];
         int newargc;
-        char **newargv = NULL;
 
         log_set_target(LOG_TARGET_AUTO);
         udev_parse_config();
@@ -585,10 +520,6 @@ int main(int argc, char **argv) {
         retval = scsi_id(maj_min_dev);
 
 exit:
-        if (newargv) {
-                free(newargv[0]);
-                free(newargv);
-        }
         log_close();
         return retval;
 }

Original file line number	Diff line number	Diff line change
`@@ -33,7 +33,7 @@ static inline int negative_errno(void) {`
`33`	`33`
`34`	`34`	`static inline const char *strerror_safe(int error) {`
`35`	`35`	`/* 'safe' here does NOT mean thread safety. */`
`36`		`- return strerror(abs(error));`
	`36`	`+ return strerror(abs(error)); /* lgtm [cpp/potentially-dangerous-function] */`
`37`	`37`	`}`
`38`	`38`
`39`	`39`	`static inline int errno_or_else(int fallback) {`
Original file line number	Diff line number	Diff line change
`@@ -51,7 +51,7 @@ bool in4_addr_is_link_local(const struct in_addr *a) {`
`51`	`51`	`bool in6_addr_is_link_local(const struct in6_addr *a) {`
`52`	`52`	`assert(a);`
`53`	`53`
`54`		`- return IN6_IS_ADDR_LINKLOCAL(a);`
	`54`	`+ return IN6_IS_ADDR_LINKLOCAL(a); /* lgtm [cpp/potentially-dangerous-function] */`
`55`	`55`	`}`
`56`	`56`
`57`	`57`	`int in_addr_is_link_local(int family, const union in_addr_union *u) {`
`@@ -116,7 +116,7 @@ int in_addr_is_localhost(int family, const union in_addr_union *u) {`
`116`	`116`	`return in4_addr_is_localhost(&u->in);`
`117`	`117`
`118`	`118`	`if (family == AF_INET6)`
`119`		`- return IN6_IS_ADDR_LOOPBACK(&u->in6);`
	`119`	`+ return IN6_IS_ADDR_LOOPBACK(&u->in6); /* lgtm [cpp/potentially-dangerous-function] */`
`120`	`120`
`121`	`121`	`return -EAFNOSUPPORT;`
`122`	`122`	`}`