| Summary: | shared: strv can't have more than 4294967295 elements | ||
|---|---|---|---|
| Product: | systemd | Reporter: | Hristo Venev <hristo> |
| Component: | general | Assignee: | systemd-bugs |
| Status: | RESOLVED FIXED | QA Contact: | systemd-bugs |
| Severity: | normal | ||
| Priority: | medium | ||
| Version: | unspecified | ||
| Hardware: | Other | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Bug Depends on: | |||
| Bug Blocks: | 76746 | ||
| Attachments: | Use size_t for strv size and index | ||
Please submit patches to the mailing list, not to Bugzilla. Please ignore the patch and fix the bug. strv is an array of strings, so if we got anywhere close to MAX_UNSIGNED elements we'd be in deep shit anyway. Various algorithms on strv are linear, so using those structures for more than a couple hundred elements would be painful anyway. If you care to provide a patch, then please do so (on the mailing list), but imo current implementation is sufficient. # busctl --host=... OK, we're listing all names. An eternity and a half later: Enter strv_push ...| // n = (1<<32) - 2 402| c = realloc(*l, sizeof(char*) * (n + 2)); ...| // *l is freed, malloc(0) (16 bytes allocated in glibc). ...| ... 406| c[n] = value; ...| Write into unallocated memory Therefore, either strv functions must fail properly if the strv is too large (1<<16 is reasonable) or they must work with sizes up to SIZE_MAX/sizeof(char*). OK, that's a real case. Can you provide an updated patch? Created attachment 107785 [details] [review] Use size_t for strv size and index This is not a complete patch. There probably are a lot more things to fix. I added two overflow checks now for strv_push() and strv_push_prepend() that should protect us if the arrays really grew that much in the first place. Any other place still missing? Thanks for the pointer! |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.
Even on computers with enough RAM. The following patch may not be complete but you should get the idea. --- a/src/shared/strv.h +++ b/src/shared/strv.h @@ -34,7 +34,7 @@ #define _cleanup_strv_free_ _cleanup_(strv_freep) char **strv_copy(char * const *l); -unsigned strv_length(char * const *l) _pure_; +size_t strv_length(char * const *l) _pure_; int strv_extend_strv(char ***a, char **b); int strv_extend_strv_concat(char ***a, char **b, const char *suffix); @@ -96,7 +96,7 @@ if (!first) \ _l = (char**) &first; \ else { \ - unsigned _n; \ + size_t _n; \ va_list _ap; \ \ _n = 1; \ --- a/src/shared/strv.c +++ b/src/shared/strv.c @@ -84,8 +84,8 @@ return r; } -unsigned strv_length(char * const *l) { - unsigned n = 0; +size_t strv_length(char * const *l) { + size_t n = 0; if (!l) return 0; @@ -99,7 +99,7 @@ char **strv_new_ap(const char *x, va_list ap) { const char *s; char **a; - unsigned n = 0, i = 0; + size_t n = 0, i = 0; va_list aq; /* As a special trick we ignore all listed strings that equal @@ -204,7 +204,7 @@ char *state; char *w; size_t l; - unsigned n, i; + size_t n, i; char **r; assert(s); @@ -236,7 +236,7 @@ char *state; char *w; size_t l; - unsigned n, i; + size_t n, i; char **r; assert(s); @@ -265,7 +265,7 @@ char **strv_split_newlines(const char *s) { char **l; - unsigned n; + size_t n; assert(s); @@ -361,7 +361,7 @@ int strv_push(char ***l, char *value) { char **c; - unsigned n; + size_t n; if (!value) return 0; @@ -436,7 +436,7 @@ char **strv_parse_nulstr(const char *s, size_t l) { const char *p; - unsigned c = 0, i = 0; + size_t c = 0, i = 0; char **v; assert(s || l <= 0);