Merge pull request #18930 from anitazha/oomdfixleak

oomd: fix memory leak

docs/CODING_STYLE.md

@@ -587,6 +587,12 @@ layout: default
   time you need that please immediately undefine `basename()`, and add a
   comment about it, so that no code ever ends up using the POSIX version!
 
+- Never use FILENAME_MAX. Use PATH_MAX instead (for checking maximum size of
+  paths) and NAME_MAX (for checking maximum size of filenames). FILENAME_MAX is
+  not POSIX, and is a confusingly named alias for PATH_MAX on Linux. Note the
+  NAME_MAX does not include space for a trailing NUL, but PATH_MAX does. UNIX
+  FTW!
+
 ## Committing to git
 
 - Commit message subject lines should be prefixed with an appropriate component

docs/USER_NAMES.md

@@ -87,8 +87,8 @@ hyphen. A size limit is enforced: the minimum of `sysconf(_SC_LOGIN_NAME_MAX)`
 (typically 256 on Linux; rationale: this is how POSIX suggests to detect the
 limit), `UT_NAMESIZE-1` (typically 31 on Linux; rationale: names longer than
 this cannot correctly appear in `utmp`/`wtmp` and create ambiguity with login
-accounting) and `FILENAME_MAX` (4096 on Linux; rationale: user names typically
+accounting) and `NAME_MAX` (255 on Linux; rationale: user names typically
-appear in directory names, i.e. the home directory), thus MIN(256, 31, 4096) =
+appear in directory names, i.e. the home directory), thus MIN(256, 31, 255) =
 31.
 
 Note that these rules are both more strict and more relaxed than all of the

src/basic/cgroup-util.c

@@ -1567,7 +1567,7 @@ bool cg_controller_is_valid(const char *p) {
                 if (!strchr(CONTROLLER_VALID, *t))
                         return false;
 
-        if (t - p > FILENAME_MAX)
+        if (t - p > NAME_MAX)
                 return false;
 
         return true;

src/basic/fs-util.c

@@ -135,34 +135,34 @@ int rename_noreplace(int olddirfd, const char *oldpath, int newdirfd, const char
 }
 
 int readlinkat_malloc(int fd, const char *p, char **ret) {
-        size_t l = FILENAME_MAX+1;
+        size_t l = PATH_MAX;
-        int r;
 
         assert(p);
         assert(ret);
 
         for (;;) {
-                char *c;
+                _cleanup_free_ char *c = NULL;
                 ssize_t n;
 
-                c = new(char, l);
+                c = new(char, l+1);
                 if (!c)
                         return -ENOMEM;
 
-                n = readlinkat(fd, p, c, l-1);
+                n = readlinkat(fd, p, c, l);
-                if (n < 0) {
+                if (n < 0)
-                        r = -errno;
+                        return -errno;
-                        free(c);
-                        return r;
-                }
 
-                if ((size_t) n < l-1) {
+                if ((size_t) n < l) {
                         c[n] = 0;
-                        *ret = c;
+                        *ret = TAKE_PTR(c);
                         return 0;
                 }
 
-                free(c);
+                if (l > (SSIZE_MAX-1)/2) /* readlinkat() returns an ssize_t, and we want an extra byte for a
+                                          * trailing NUL, hence do an overflow check relative to SSIZE_MAX-1
+                                          * here */
+                        return -EFBIG;
+
                 l *= 2;
         }
 }

src/basic/mountpoint-util.c

@@ -148,7 +148,7 @@ static bool filename_possibly_with_slash_suffix(const char *s) {
         if (!slash)
                 return filename_is_valid(s);
 
-        if (slash - s > FILENAME_MAX) /* We want to allocate on the stack below, hence do a size check first */
+        if (slash - s > PATH_MAX) /* We want to allocate on the stack below, hence do a size check first */
                 return false;
 
         if (slash[strspn(slash, "/")] != 0) /* Check that the suffix consist only of one or more slashes */

src/basic/user-util.c

@@ -836,7 +836,7 @@ bool valid_user_group_name(const char *u, ValidUserFlags flags) {
 
                 if (l > (size_t) sz)
                         return false;
-                if (l > FILENAME_MAX)
+                if (l > NAME_MAX) /* must fit in a filename */
                         return false;
                 if (l > UT_NAMESIZE - 1)
                         return false;

src/oom/oomd-manager.c

@@ -112,7 +112,7 @@ static int process_managed_oom_reply(
                                 continue;
                 }
 
-                ret = oomd_insert_cgroup_context(NULL, monitor_hm, empty_to_root(reply.path));
+                ret = oomd_insert_cgroup_context(NULL, monitor_hm, reply.path);
                 if (ret == -ENOMEM) {
                         r = ret;
                         goto finish;

src/oom/oomd-util.c

@@ -384,16 +384,20 @@ int oomd_system_context_acquire(const char *proc_swaps_path, OomdSystemContext *
 
 int oomd_insert_cgroup_context(Hashmap *old_h, Hashmap *new_h, const char *path) {
         _cleanup_(oomd_cgroup_context_freep) OomdCGroupContext *curr_ctx = NULL;
-        OomdCGroupContext *old_ctx, *ctx;
+        OomdCGroupContext *old_ctx;
         int r;
 
         assert(new_h);
         assert(path);
 
+        path = empty_to_root(path);
+
         r = oomd_cgroup_context_acquire(path, &curr_ctx);
         if (r < 0)
                 return log_debug_errno(r, "Failed to get OomdCGroupContext for %s: %m", path);
 
+        assert_se(streq(path, curr_ctx->path));
+
         old_ctx = hashmap_get(old_h, path);
         if (old_ctx) {
                 curr_ctx->last_pgscan = old_ctx->pgscan;
@@ -401,11 +405,11 @@ int oomd_insert_cgroup_context(Hashmap *old_h, Hashmap *new_h, const char *path)
                 curr_ctx->last_hit_mem_pressure_limit = old_ctx->last_hit_mem_pressure_limit;
         }
 
-        ctx = TAKE_PTR(curr_ctx);
+        r = hashmap_put(new_h, curr_ctx->path, curr_ctx);
-        r = hashmap_put(new_h, ctx->path, ctx);
         if (r < 0)
                 return r;
 
+        TAKE_PTR(curr_ctx);
         return 0;
 }
 

src/oom/test-oomd-util.c

@@ -150,6 +150,7 @@ static void test_oomd_cgroup_context_acquire_and_insert(void) {
         assert_se(oomd_insert_cgroup_context(NULL, h1, cgroup) == 0);
         c1 = hashmap_get(h1, cgroup);
         assert_se(c1);
+        assert_se(oomd_insert_cgroup_context(NULL, h1, cgroup) == -EEXIST);
 
          /* make sure certain values from h1 get updated in h2 */
         c1->pgscan = 5555;

src/shared/efi-loader.c

@@ -809,10 +809,8 @@ bool efi_has_tpm2(void) {
 #endif
 
 bool efi_loader_entry_name_valid(const char *s) {
-        if (isempty(s))
-                return false;
 
-        if (strlen(s) > FILENAME_MAX) /* Make sure entry names fit in filenames */
+        if (!filename_is_valid(s)) /* Make sure entry names fit in filenames */
                 return false;
 
         return in_charset(s, ALPHANUMERICAL "+-_.");

src/test/test-path-util.c

@@ -838,6 +838,15 @@ static void test_path_startswith_strv(void) {
 int main(int argc, char **argv) {
         test_setup_logging(LOG_DEBUG);
 
+        log_info("PATH_MAX=%zu\n"
+                 "FILENAME_MAX=%zu\n"
+                 "NAME_MAX=%zu",
+                 (size_t) PATH_MAX,
+                 (size_t) FILENAME_MAX,
+                 (size_t) NAME_MAX);
+
+        assert_cc(FILENAME_MAX == PATH_MAX);
+
         test_print_paths();
         test_path();
         test_path_equal_root();

src/timedate/timedated.c

@@ -211,7 +211,7 @@ static int context_parse_ntp_services_from_disk(Context *c) {
                                 break;
 
                         word = strstrip(line);
-                        if (isempty(word) || startswith("#", word))
+                        if (isempty(word) || startswith(word, "#"))
                                 continue;
 
                         r = context_add_ntp_service(c, word, *f);