Fixes: CFE-3982 do not kill unrelated process #4957

peckpeck · 2022-05-24T10:02:10Z

Pull request to fix https://tracker.mender.io/browse/CFE-3982

vpodzime

Also please read our contributing guidelines and try to keep the same coding style as the surrounding code.

vpodzime · 2022-05-24T12:45:28Z

libpromises/locks.c

@@ -903,15 +927,23 @@ CfLock AcquireLock(EvalContext *ctx, const char *operand, const char *host,
                    "Lock expired after %jd/%u minutes: %s",
                    (intmax_t) elapsedtime, expireafter, cflock);

-                if (KillLockHolder(cflock))
+                if (is_cfengine_process(cflock.pid))


I'd prefer a function that returns the path/name to/of the executable when given a PID and using that function here. A the function should go to the file where we do process handling and process table parsing. Also, it should use the process info parsing code from there instead of reading /proc directly because /proc is not a portable thing available everywhere.

I don't see any function for reading information for a process given its pid in ./libpromises/processes_select.c
If this is not what you call "the file where we do process handling" please point me to the correct one.

peckpeck · 2022-05-30T14:35:53Z

About the coding style, is there anything else than the function name that you want to point me to ?

olehermanse · 2022-05-31T12:42:29Z

About the coding style, is there anything else than the function name that you want to point me to ?

@peckpeck Some things I see at first glance:

Space after if, commas, and around operators (like >).
Opening curly brace on separate line ({).
Don't do if's without curly braces.
Avoid unnecessary typecasts.
Pointer asterisk to the right, next to the name (FILE *f).
Compare pointers to NULL explicitly (if (f != NULL)).

vpodzime · 2022-07-14T11:52:07Z

libpromises/locks.c

+    char* procfile = xcalloc(1024, sizeof(char));
+    char* cmd = xcalloc(1024, sizeof(char));
+    if (cmd && procfile){
+        sprintf(procfile, "/proc/%d/cmdline", pid);


This emits an error during compilation:

locks.c:565:9: error: 'sprintf' is deprecated [-Werror,-Wdeprecated-declarations] sprintf(procfile, "/proc/%d/cmdline", pid); ^ ./../libntech/libutils/deprecated.h:42:5: note: 'sprintf' has been explicitly marked deprecated here FUNC_DEPRECATED("Try snprintf() or xsnprintf() or xasprintf()"); ^ ./../libntech/libutils/compiler.h:44:49: note: expanded from macro 'FUNC_DEPRECATED' # define FUNC_DEPRECATED(msg) __attribute__((deprecated)) ^

vpodzime

A bit more work needed.

vpodzime · 2022-07-28T11:42:17Z

libpromises/locks.c

+    // There is no /proc on windows
+#ifndef _WIN32
+    char* procfile = xcalloc(1024, sizeof(char));
+    char* cmd = xcalloc(1024, sizeof(char));


Either free() these variables or, better, use arrays like char procfile[PATH_MAX] and similar.

vpodzime · 2022-07-28T11:42:47Z

libpromises/locks.c

+#ifndef _WIN32
+    char* procfile = xcalloc(1024, sizeof(char));
+    char* cmd = xcalloc(1024, sizeof(char));
+    if (cmd && procfile){


No need to check these, xcalloc() kills the process in case of ENOMEM.

vpodzime · 2022-07-28T11:44:11Z

libpromises/locks.c

@@ -587,6 +616,12 @@ static bool KillLockHolder(const char *lock)

    CloseLock(dbp);

+    if (!IsCfengineProcess(lock_data.pid)) {
+        Log(LOG_LEVEL_INFO,
+            "Lock holder disappeared");


This log message should be a bit more verbose and logged as VERBOSE.

…cess

peckpeck · 2022-09-15T12:47:33Z

sorry for the delay

peckpeck · 2022-12-14T08:19:56Z

@vpodzime any news on this ?

larsewi · 2022-12-14T15:39:11Z

Hi @peckpeck ! as far as i know, Windows is not the only platform without the /proc file. Please correct me if I'm wrong. In libpromises/process_select.h we have some functions for querying processes. Maybe you can use those? E.g.

ClearProcessTable(); /* Make sure old process table is not cached */

Item *procs = NULL;
if (LoadProcessTable())
{
    ProcessSelect ps = PROCESS_SELECT_INIT;
    ps.max_pid = ps.min_pid = lock_data.pid;
    procs = SelectProcesses("cf-.*", &ps, true);
}
if (procs == NULL)
{
    /* Process select returned nothing, it must be an unrelated process */
    return false;
}
DeleteItemList(procs);
return true;

Not sure if this would work, but feel free to give it a try. Otherwise you could maybe guard it with the #ifdef __linux__ macro.

…ted process

vpodzime · 2023-05-09T11:15:24Z

libpromises/locks.c

@@ -554,6 +554,33 @@ static void PromiseTypeString(char *dst, size_t dst_size, const Promise *pp)
    }
 }

+// returns true if pid points to a cfengine process
+static bool IsCfengineProcess(const int pid)


const has no meaning here, and the type should be pid_t not int.

vpodzime · 2023-05-09T11:18:20Z

libpromises/locks.c

+    FILE *f = fopen(procfile, "r");
+    if (f != NULL){
+        size_t size;
+        size = fread(cmd, sizeof(char), PATH_MAX, f);


Please check FullRead() in libntech/libutils

vpodzime · 2023-05-09T11:18:42Z

libpromises/locks.c

+        fclose(f);
+        // cmd contains the process command path
+        char *name = basename(cmd);
+        return (strncmp("cf-", name, 3) == 0);


Please check StringStartsWith() in libntech/libutils

vpodzime · 2023-05-09T11:20:30Z

libpromises/locks.c

+        }
+        fclose(f);
+        // cmd contains the process command path
+        char *name = basename(cmd);


If size is 0 then cmd is uninitialized here.

vpodzime · 2023-05-09T11:21:18Z

libpromises/locks.c

+    }
+#endif
+    // assume true if we don't know
+    return true;


Wouldn't it be safer to assume the opposite?

we don't know means use the old behaviour on other platforms

vpodzime · 2023-05-09T11:22:16Z

libpromises/locks.c

+    if (!IsCfengineProcess(lock_data.pid)) {
+        Log(LOG_LEVEL_VERBOSE,
+            "Lock holder with pid %d was replaced by a non cfengine process, do not try to kill it!, ", lock_data.pid);
+        return true;


So the function returns true here in this case, but it doesn't really kill the lock holder. That can be a source of issues.

This is still applicable.

… unrelated process

peckpeck · 2023-08-30T07:40:51Z

PR updated, sorry for the delay

vpodzime · 2023-10-18T06:26:08Z

libpromises/locks.c

+{
+    char procfile[PATH_MAX];
+    char cmd[PATH_MAX]; // we don't need the full ARG_MAX
+    snprintf(procfile, PATH_MAX, "/proc/%d/cmdline", pid);


pid_t doesn't have to be int and this can easily fail to compile in such a case because of the format (%d) and type (pid_t) mismatch. We usually use a trick with uintmax_t for things like this.

vpodzime · 2023-10-18T06:27:40Z

libpromises/locks.c

+    // this will just return null on platform where /proc doesn't exist and return true as before
+    if (f != NULL){
+        size_t size;
+        size = FullRead(f, cmd, PATH_MAX);


size_t size = FullRead(f, cmd, PATH_MAX);

vpodzime · 2023-10-18T06:29:45Z

libpromises/locks.c

+    if (!IsCfengineProcess(lock_data.pid)) {
+        Log(LOG_LEVEL_VERBOSE,
+            "Lock holder with pid %d was replaced by a non cfengine process, do not try to kill it!, ", lock_data.pid);
+        return true;


This is still applicable.

vpodzime · 2023-11-07T10:56:11Z

@peckpeck I took your code as a base for my work on the related ticket. Thank you for your contribution!

Fixes: CFE-3982 do not kill unrelated process

b322c29

olehermanse requested a review from vpodzime May 24, 2022 10:38

vpodzime reviewed May 24, 2022

View reviewed changes

fixup! Fixes: CFE-3982 do not kill unrelated process

396cf6a

peckpeck force-pushed the kill_cfengine_only branch from 7bb5648 to 396cf6a Compare May 30, 2022 14:52

fixup! fixup! Fixes: CFE-3982 do not kill unrelated process

41a4cce

vpodzime reviewed Jul 14, 2022

View reviewed changes

fixup! fixup! fixup! Fixes: CFE-3982 do not kill unrelated process

d64f3ea

vpodzime reviewed Jul 28, 2022

View reviewed changes

fixup! fixup! fixup! fixup! Fixes: CFE-3982 do not kill unrelated pro…

41634f4

…cess

fixup! fixup! fixup! fixup! fixup! Fixes: CFE-3982 do not kill unrela…

5cd78b6

…ted process

vpodzime reviewed May 9, 2023

View reviewed changes

fixup! fixup! fixup! fixup! fixup! fixup! Fixes: CFE-3982 do not kill…

c6b6d03

… unrelated process

vpodzime reviewed Oct 18, 2023

View reviewed changes

vpodzime closed this Nov 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes: CFE-3982 do not kill unrelated process #4957

Fixes: CFE-3982 do not kill unrelated process #4957

peckpeck commented May 24, 2022

vpodzime left a comment

vpodzime May 24, 2022

peckpeck May 30, 2022

peckpeck commented May 30, 2022

olehermanse commented May 31, 2022

vpodzime Jul 14, 2022

vpodzime left a comment

vpodzime Jul 28, 2022

vpodzime Jul 28, 2022

vpodzime Jul 28, 2022

peckpeck commented Sep 15, 2022

peckpeck commented Dec 14, 2022

larsewi commented Dec 14, 2022 •

edited

Loading

vpodzime May 9, 2023 •

edited

Loading

vpodzime May 9, 2023

vpodzime May 9, 2023

vpodzime May 9, 2023

vpodzime May 9, 2023

peckpeck Aug 30, 2023

vpodzime May 9, 2023

vpodzime Oct 18, 2023

peckpeck commented Aug 30, 2023

vpodzime Oct 18, 2023

vpodzime Oct 18, 2023

vpodzime Oct 18, 2023

vpodzime commented Nov 7, 2023

Fixes: CFE-3982 do not kill unrelated process #4957

Fixes: CFE-3982 do not kill unrelated process #4957

Conversation

peckpeck commented May 24, 2022

vpodzime left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peckpeck commented May 30, 2022

olehermanse commented May 31, 2022

Choose a reason for hiding this comment

vpodzime left a 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

peckpeck commented Sep 15, 2022

peckpeck commented Dec 14, 2022

larsewi commented Dec 14, 2022 • edited Loading

vpodzime May 9, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peckpeck commented Aug 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpodzime commented Nov 7, 2023

larsewi commented Dec 14, 2022 •

edited

Loading

vpodzime May 9, 2023 •

edited

Loading