From 530727aed8bad2657c2b606bc45f98d7bc0fe2ad Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 18 Feb 2019 09:21:47 +0900 Subject: [PATCH 1/6] udev-rules: use GREEDY_REALLOC() macro where it applicable This also changes types of several variables e.g. token_max to size_t. --- src/udev/udev-rules.c | 89 +++++++++++++------------------------------ 1 file changed, 26 insertions(+), 63 deletions(-) diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index 8603c75cb6..e4fff94081 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -63,19 +63,19 @@ struct UdevRules { /* every key in the rules file becomes a token */ struct token *tokens; - unsigned token_cur; - unsigned token_max; + size_t token_cur; + size_t token_max; /* all key strings are copied and de-duplicated in a single continuous string buffer */ struct strbuf *strbuf; /* during rule parsing, uid/gid lookup results are cached */ struct uid_gid *uids; - unsigned uids_cur; - unsigned uids_max; + size_t uids_cur; + size_t uids_max; struct uid_gid *gids; - unsigned gids_cur; - unsigned gids_max; + size_t gids_cur; + size_t gids_max; }; static char *rules_str(UdevRules *rules, unsigned off) { @@ -219,7 +219,7 @@ struct rule_tmp { UdevRules *rules; struct token rule; struct token token[MAX_TK]; - unsigned token_cur; + size_t token_cur; }; #if ENABLE_DEBUG_UDEV @@ -430,9 +430,9 @@ static void dump_token(UdevRules *rules, struct token *token) { } static void dump_rules(UdevRules *rules) { - unsigned i; + size_t i; - log_debug("Dumping %u (%zu bytes) tokens, %zu (%zu bytes) strings", + log_debug("Dumping %zu (%zu bytes) tokens, %zu (%zu bytes) strings", rules->token_cur, rules->token_cur * sizeof(struct token), rules->strbuf->nodes_count, @@ -447,21 +447,9 @@ static void dump_rules(UdevRules *rules) {} static int add_token(UdevRules *rules, struct token *token) { /* grow buffer if needed */ - if (rules->token_cur+1 >= rules->token_max) { - struct token *tokens; - unsigned add; + if (!GREEDY_REALLOC(rules->tokens, rules->token_max, rules->token_cur + 1)) + return -ENOMEM; - /* double the buffer size */ - add = rules->token_max; - if (add < 8) - add = 8; - - tokens = reallocarray(rules->tokens, rules->token_max + add, sizeof(struct token)); - if (!tokens) - return -ENOMEM; - rules->tokens = tokens; - rules->token_max += add; - } memcpy(&rules->tokens[rules->token_cur], token, sizeof(struct token)); rules->token_cur++; return 0; @@ -475,9 +463,9 @@ static void log_unknown_owner(sd_device *dev, int error, const char *entity, con } static uid_t add_uid(UdevRules *rules, const char *owner) { - unsigned i; uid_t uid = 0; unsigned off; + size_t i; int r; /* lookup, if we know it already */ @@ -491,21 +479,9 @@ static uid_t add_uid(UdevRules *rules, const char *owner) { log_unknown_owner(NULL, r, "user", owner); /* grow buffer if needed */ - if (rules->uids_cur+1 >= rules->uids_max) { - struct uid_gid *uids; - unsigned add; + if (!GREEDY_REALLOC(rules->uids, rules->uids_max, rules->uids_cur + 1)) + return -ENOMEM; - /* double the buffer size */ - add = rules->uids_max; - if (add < 1) - add = 8; - - uids = reallocarray(rules->uids, rules->uids_max + add, sizeof(struct uid_gid)); - if (!uids) - return uid; - rules->uids = uids; - rules->uids_max += add; - } rules->uids[rules->uids_cur].uid = uid; off = rules_add_string(rules, owner); if (off <= 0) @@ -516,9 +492,9 @@ static uid_t add_uid(UdevRules *rules, const char *owner) { } static gid_t add_gid(UdevRules *rules, const char *group) { - unsigned i; gid_t gid = 0; unsigned off; + size_t i; int r; /* lookup, if we know it already */ @@ -532,21 +508,9 @@ static gid_t add_gid(UdevRules *rules, const char *group) { log_unknown_owner(NULL, r, "group", group); /* grow buffer if needed */ - if (rules->gids_cur+1 >= rules->gids_max) { - struct uid_gid *gids; - unsigned add; + if (!GREEDY_REALLOC(rules->gids, rules->gids_max, rules->gids_cur + 1)) + return -ENOMEM; - /* double the buffer size */ - add = rules->gids_max; - if (add < 1) - add = 8; - - gids = reallocarray(rules->gids, rules->gids_max + add, sizeof(struct uid_gid)); - if (!gids) - return gid; - rules->gids = gids; - rules->gids_max += add; - } rules->gids[rules->gids_cur].gid = gid; off = rules_add_string(rules, group); if (off <= 0) @@ -973,15 +937,15 @@ static int rule_add_key(struct rule_tmp *rule_tmp, enum token_type type, } static int sort_token(UdevRules *rules, struct rule_tmp *rule_tmp) { - unsigned i; - unsigned start = 0; - unsigned end = rule_tmp->token_cur; + size_t i; + size_t start = 0; + size_t end = rule_tmp->token_cur; int r; for (i = 0; i < rule_tmp->token_cur; i++) { enum token_type next_val = TK_UNSET; - unsigned next_idx = 0; - unsigned j; + size_t next_idx = 0; + size_t j; /* find smallest value */ for (j = start; j < end; j++) { @@ -1013,7 +977,7 @@ static int sort_token(UdevRules *rules, struct rule_tmp *rule_tmp) { #define LOG_RULE_WARNING(fmt, ...) LOG_RULE_FULL(LOG_WARNING, fmt, ##__VA_ARGS__) #define LOG_RULE_DEBUG(fmt, ...) LOG_RULE_FULL(LOG_DEBUG, fmt, ##__VA_ARGS__) #define LOG_AND_RETURN(fmt, ...) { LOG_RULE_ERROR(fmt, __VA_ARGS__); return; } -#define LOG_AND_RETURN_ADD_KEY LOG_AND_RETURN("Temporary rule array too small, aborting event processing with %u items", rule_tmp.token_cur); +#define LOG_AND_RETURN_ADD_KEY LOG_AND_RETURN("Temporary rule array too small, aborting event processing with %zu items", rule_tmp.token_cur); static void add_rule(UdevRules *rules, char *line, const char *filename, unsigned filename_off, unsigned lineno) { @@ -1500,11 +1464,10 @@ static void add_rule(UdevRules *rules, char *line, static int parse_file(UdevRules *rules, const char *filename) { _cleanup_fclose_ FILE *f = NULL; - unsigned first_token; + size_t first_token, i; unsigned filename_off; char line[UTIL_LINE_SIZE]; int line_nr = 0; - unsigned i; f = fopen(filename, "re"); if (!f) { @@ -1562,7 +1525,7 @@ static int parse_file(UdevRules *rules, const char *filename) { for (i = first_token+1; i < rules->token_cur; i++) { if (rules->tokens[i].type == TK_A_GOTO) { char *label = rules_str(rules, rules->tokens[i].key.value_off); - unsigned j; + size_t j; for (j = i+1; j < rules->token_cur; j++) { if (rules->tokens[j].type != TK_RULE) @@ -1625,7 +1588,7 @@ int udev_rules_new(UdevRules **ret_rules, ResolveNameTiming resolve_name_timing) struct token end_token = { .type = TK_END }; add_token(rules, &end_token); - log_debug("Rules contain %zu bytes tokens (%u * %zu bytes), %zu bytes strings", + log_debug("Rules contain %zu bytes tokens (%zu * %zu bytes), %zu bytes strings", rules->token_max * sizeof(struct token), rules->token_max, sizeof(struct token), rules->strbuf->len); /* cleanup temporary strbuf data */ From 759fb3a904277d24d75e2c0541b0e988db660ac8 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 18 Feb 2019 09:24:03 +0900 Subject: [PATCH 2/6] udev-rules: use size_t for array index --- src/udev/udev-rules.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index e4fff94081..c63af7cb4b 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -676,7 +676,7 @@ static void attr_subst_subdir(char *attr, size_t len) { static int get_key(char **line, char **key, enum operation_type *op, char **value) { char *linepos; char *temp; - unsigned i, j; + size_t i, j; linepos = *line; if (!linepos || linepos[0] == '\0') From f10aa08e3e48de7dcb71be348f021c6b1385304f Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 18 Feb 2019 10:37:49 +0900 Subject: [PATCH 3/6] udev-rules: use read_line() and drop fgets() --- src/udev/udev-rules.c | 65 +++++++++++++++++++++++++++++-------------- 1 file changed, 44 insertions(+), 21 deletions(-) diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index c63af7cb4b..b741d053f7 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -1463,11 +1463,12 @@ static void add_rule(UdevRules *rules, char *line, } static int parse_file(UdevRules *rules, const char *filename) { + _cleanup_free_ char *continuation = NULL; _cleanup_fclose_ FILE *f = NULL; + bool ignore_line = false; size_t first_token, i; unsigned filename_off; - char line[UTIL_LINE_SIZE]; - int line_nr = 0; + int line_nr = 0, r; f = fopen(filename, "re"); if (!f) { @@ -1486,39 +1487,61 @@ static int parse_file(UdevRules *rules, const char *filename) { first_token = rules->token_cur; filename_off = rules_add_string(rules, filename); - while (fgets(line, sizeof(line), f)) { - char *key; + for (;;) { + _cleanup_free_ char *buf = NULL; size_t len; + char *line; + + r = read_line(f, UTIL_LINE_SIZE, &buf); + if (r < 0) + return r; + if (r == 0) + break; - /* skip whitespace */ line_nr++; - key = line; - while (isspace(key[0])) - key++; + line = buf + strspn(buf, WHITESPACE); - /* comment */ - if (key[0] == '#') + if (line[0] == '#') continue; len = strlen(line); if (len < 3) continue; - /* continue reading if backslash+newline is found */ - while (line[len-2] == '\\') { - if (!fgets(&line[len-2], (sizeof(line)-len)+2, f)) - break; - if (strlen(&line[len-2]) < 2) - break; - line_nr++; - len = strlen(line); + if (continuation && !ignore_line) { + if (strlen(continuation) + len >= UTIL_LINE_SIZE) + ignore_line = true; + + if (!strextend(&continuation, line, NULL)) + return log_oom(); + + if (!ignore_line) { + line = continuation; + len = strlen(line); + } } - if (len+1 >= sizeof(line)) { - log_error("Line too long '%s':%u, ignored", filename, line_nr); + if (line[len - 1] == '\\') { + if (ignore_line) + continue; + + line[len - 1] = '\0'; + if (!continuation) { + continuation = strdup(line); + if (!continuation) + return log_oom(); + } + continue; } - add_rule(rules, key, filename, filename_off, line_nr); + + if (ignore_line) + log_error("Line too long '%s':%u, ignored", filename, line_nr); + else + add_rule(rules, line, filename, filename_off, line_nr); + + continuation = mfree(continuation); + ignore_line = false; } /* link GOTOs to LABEL rules in this file to be able to fast-forward */ From 1e797cf596df50a6bdd8cbf8e9b2467a3a934171 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 18 Feb 2019 10:38:29 +0900 Subject: [PATCH 4/6] test-udev: add a testcase of too long line --- test/udev-test.pl | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/test/udev-test.pl b/test/udev-test.pl index 957cda541c..3a50694fa9 100755 --- a/test/udev-test.pl +++ b/test/udev-test.pl @@ -39,6 +39,11 @@ for (my $i = 1; $i <= 10000; ++$i) { $rules_10k_tags .= 'KERNEL=="sda", TAG+="test' . $i . "\"\n"; } +my $rules_10k_tags_continuation = ""; +for (my $i = 1; $i <= 10000; ++$i) { + $rules_10k_tags_continuation .= 'KERNEL=="sda", TAG+="test' . $i . "\"\\\n"; +} + my @tests = ( { desc => "no rules", @@ -1444,6 +1449,16 @@ EOF exp_name => "found", rules => $rules_10k_tags . < "don't crash with lots of tags with continuation", + devpath => "/devices/pci0000:00/0000:00:1f.2/host0/target0:0:0/0:0:0:0/block/sda", + exp_name => "found", + not_exp_name => "bad" , + rules => $rules_10k_tags_continuation . < Date: Mon, 18 Feb 2019 10:41:48 +0900 Subject: [PATCH 5/6] udev-rules: use new() macro instead of malloc_multiply() --- src/udev/udev-rules.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index b741d053f7..4d755ae9bc 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -1584,7 +1584,7 @@ int udev_rules_new(UdevRules **ret_rules, ResolveNameTiming resolve_name_timing) }; /* init token array and string buffer */ - rules->tokens = malloc_multiply(PREALLOC_TOKEN, sizeof(struct token)); + rules->tokens = new(struct token, PREALLOC_TOKEN); if (!rules->tokens) return -ENOMEM; rules->token_max = PREALLOC_TOKEN; From 72ca8f71c1640c8ab413bd46121396134420d506 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 18 Feb 2019 12:18:56 +0900 Subject: [PATCH 6/6] udev-rules: use parse_uid() or parse_gid() --- src/udev/udev-rules.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index 4d755ae9bc..ce787786a6 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -1334,13 +1334,11 @@ static void add_rule(UdevRules *rules, char *line, } else if (streq(key, "OWNER")) { uid_t uid; - char *endptr; if (op == OP_REMOVE) LOG_AND_RETURN("Invalid %s operation", key); - uid = strtoul(value, &endptr, 10); - if (endptr[0] == '\0') + if (parse_uid(value, &uid) >= 0) r = rule_add_key(&rule_tmp, TK_A_OWNER_ID, op, NULL, &uid); else if (rules->resolve_name_timing == RESOLVE_NAME_EARLY && !strchr("$%", value[0])) { uid = add_uid(rules, value); @@ -1358,13 +1356,11 @@ static void add_rule(UdevRules *rules, char *line, } else if (streq(key, "GROUP")) { gid_t gid; - char *endptr; if (op == OP_REMOVE) LOG_AND_RETURN("Invalid %s operation", key); - gid = strtoul(value, &endptr, 10); - if (endptr[0] == '\0') + if (parse_gid(value, &gid) >= 0) r = rule_add_key(&rule_tmp, TK_A_GROUP_ID, op, NULL, &gid); else if ((rules->resolve_name_timing == RESOLVE_NAME_EARLY) && !strchr("$%", value[0])) { gid = add_gid(rules, value);