src/libcrun: fix handling of device paths with trailing slashes #1517

sohankunkerkar · 2024-08-12T16:49:53Z

This change handles device paths with trailing slashes correctly. It address the following issue when we use crun as the default container runtime:

Error: container create failed: mknod `/mnt/`: No such file or directory .

haircommander · 2024-08-12T16:52:34Z

FYI this came up because of kubernetes/kubernetes#126641

haircommander · 2024-08-12T16:56:19Z

I think SecureJoin is why this works for runc

src/libcrun/linux.c

packit-as-a-service · 2024-08-12T18:25:05Z

podman system tests failed. @containers/packit-build please check.

kolyshkin · 2024-08-12T21:05:35Z

src/libcrun/linux.c

+  if (normalized_path[path_len - 1] == '/')
+    {
+      normalized_path[path_len - 1] = '\0';
+    }


This won't work if there are two slashes at the end (which is otherwise OK).

Basically need to replace if with while.

Having said that, I'd rather fix it in e.g. cri-o. Asking to create a block device with a slash at the end is somewhat weird.

OTOH runc for some reason works.

I agree CRI-O is handing crun garbage. I am fine with fixing this, but the real source of the problem is a device with a trailing "/" is and invalid device.

OTOH runc for some reason works.

#1517 (comment)

@haircommander, should we try to handle this in CRI-O before it even gets to a runtime?

we could for sure, the code would be easier to write in go

I wouldn't mind handling this in CRI-O. The only caveat is that if some other CRI implementation hits the same issue, then they might question the crun code because this case is already handled in runc

I think that's a "cross the bridge when we get there" scenario. This was opened up because it's a bug in the test and it's arguable whether runc should even silently allow it (it's undefined in the runtime spec).

Yeah, I would prefer, personally, that the user receive feedback earlier that their path is not conformant (per what @rhatdan said above) rather than us correcting it behind the scenes, so to speak.

If not, then we need to clean the paths up in runc and crun, too. Perhaps even in CRI-O as the first line of defence against malformed values.

src/libcrun/linux.c

src/libcrun/utils.c

kwilczynski · 2024-08-13T12:52:25Z

src/libcrun/linux.c

-  const char *rel_dev = relative_path_under_dev (device->path);
+  // Normalize path by removing trailing slash if it exists.
+  cleanup_free char *normalized_path = xstrdup (fullname);
+  cleanup_free char *last_slash = trim_trailing_slashes (normalized_path);


consume_trailing_slashes(normalized_path);

See my other comment below.

kwilczynski · 2024-08-13T14:58:04Z

src/libcrun/linux.c

-          cleanup_free char *buffer = NULL;
+          char *dirname = normalized_path, *basename;
          cleanup_close int dirfd = -1;
-          char *basename, *tmp;
-
-          buffer = xstrdup (device->path);
-          dirname = buffer;
-
-          tmp = strrchr (buffer, '/');
-          *tmp = '\0';
-          basename = tmp + 1;
+          // Find the last slash in the normalized path after trimming
+          char *last_slash = strrchr (normalized_path, '/');
+          if (last_slash)
+            basename = last_slash + 1;
+          else
+            basename = normalized_path;


The previous code looks for the directory name and base directory. We can update it a bit, given the use of normalized_path, or keep it at large as-is per the current version.

cleanup_close int dirfd = -1; cleanup_free char dirname = NULL; char *basename, *found; dirname = strdup(normalized_path); found = strrchr(dirname, '/'); *found = '\0'; basename = found + 1;

Okay, I think the previous implementation will still work, but the dirname points to the same memory as normalized_path and any modifications to dirname would also affect normalized_path, which can lead to unintended result if not handled properly. I believe we need a slight modification the code you suggested. Thanks for the pointers.

cleanup_close int dirfd = -1; cleanup_free char *dirname = NULL; char *basename, *found; dirname = strdup (normalized_path); found = strrchr (dirname, '/'); if (found) *found = '\0'; basename = found + 1;

@sohankunkerkar, we might have to add a special case, if possible, where the path would be simply a single forward slash (so "/") or any value that is not a path, like "test" (not sure if ever possible to get such a value), then the code would misbehave.

Problematic values: "" (empty string; strlen() == 0), "/", "test", "test/"; at the moment.

kwilczynski · 2024-08-13T15:03:10Z

src/libcrun/utils.h

@@ -422,6 +422,8 @@ get_process_exit_status (int status)
 uid_t get_overflow_uid (void);
 gid_t get_overflow_gid (void);

+void consume_trailing_slashes (char *path);


We could move it right below the following:

https://github.com/containers/crun/pull/1517/files#diff-e84345e78d59b60cc2aadd7662467f8959e7baa95c1ec9b58ad04f7196ae2273R228-R234

src/libcrun/utils.c

src/libcrun/linux.c

packit-as-a-service · 2024-08-13T15:55:42Z

Ephemeral COPR build failed. @containers/packit-build please check.

kwilczynski · 2024-08-13T17:29:42Z

src/libcrun/linux.c

+          if (found)
+            *found = '\0';
+
+          basename = found + 1;


There is something interesting about this implementation, generally.

The added NULL check above is fine. However, if we ever get a NULL in found, then we would have a value of 1 in basename, and then the attempt to read from it will be either SIGSEGV, hopefully, or some undefined behaviour (worst case; should not happen on Linux, hopefully).

The old (original) code also has this problem.

kwilczynski · 2024-08-13T18:44:07Z

src/libcrun/utils.c

+  // Handle the case where the path is just a single slash.
+  if (strcmp (path, "/") == 0)
+    {
+      return;
+    }


This probably does not belong here, as it is a utility function.

Why? The handling of a single slash is a use case specific to the code that calls this function.

ok, I don't think we need to handle this condition separately. It will covered as part of the below snippet:

cleanup_free char *normalized_path = xstrdup (fullname); consume_trailing_slashes (normalized_path); if (normalized_path[0] == '\0') strcpy (normalized_path, "/");

kwilczynski · 2024-08-13T20:11:04Z

src/libcrun/linux.c

+          found = strrchr (dirname, '/');
+          if (found)
+            {
+              *found = '\0';
+              basename = found + 1;
+            }
+          else
+            {
+              basename = dirname;
+            }


Would this be the correct behaviour here?

This ensures that if there are no slashes in the path, basename is set to the entire path. This prevents any uninitialized variables and potential segmentation faults. Just an improvement over the original code. We need to clarify with @kolyshkin or @giuseppe to understand if there's any repercussion to it.

Example Scenarios:

Path with Slashes:
Input: "/path/to/file.txt"
Operation:
found = strrchr(dirname, '/') will point to the last slash ('/').
*found = '\0' will truncate dirname to "/path/to".
basename = found + 1 will set basename to "file.txt" (the part after the last slash).
Result:
dirname = "/path/to"
basename = "file.txt"

Path Without Slashes:
Input: "file.txt"
Operation:
found = strrchr(dirname, '/') will return NULL since there are no slashes in the path.
The line basename = found ? found + 1 : dirname; will evaluate to basename = dirname;, meaning basename will be set to "file.txt".
Result:
dirname = "file.txt"
basename = "file.txt"

Single Slash:
Input: "/"
Operation:
found = strrchr(dirname, '/') will point to the single slash.
*found = '\0' will truncate dirname to an empty string ("").
basename = found ? found + 1 : dirname; will evaluate to basename = ""; (because found + 1 points to the character after the null terminator).
Result:
dirname = ""
basename = ""

Empty String:
Input: ""
Operation:
found = strrchr(dirname, '/') will return NULL.
basename = found ? found + 1 : dirname; will evaluate to basename = ""; (because dirname is empty).
Result:
dirname = ""
basename = ""

kwilczynski · 2024-08-13T20:11:31Z

src/libcrun/utils.c

+  if (path[0] == '\0')
+    {
+      strcpy (path, "/");
+    }


Ditto. See my above comment.

src/libcrun/utils.c

giuseppe

just a minor comment, otherwise LGTM

src/libcrun/linux.c

kwilczynski · 2024-08-20T05:17:17Z

src/libcrun/linux.c

+  // Normalize the path by removing trailing slashes.
+  cleanup_free char *normalized_path = xstrdup (fullname);
+  consume_trailing_slashes (normalized_path);
+  if (normalized_path[0] == '\0')


Suggested change

if (normalized_path[0] == '\0')

if (is_empty_string(normalized_path))

This can also be used to check if the fullname variable is empty, per @giuseppe's request.

@sohankunkerkar, not using is_empty_string() here?

please use is_empty_string() here

I think is_empty_string would be redundant in this case as consume_trailing_slashes already handles the null and empty string cases, checking normalized_path[0] = '\0' immediately afterward is sufficient. Let me know if I'm missing out any test case.

The helper would be fine. I think we should use it. Also, it improves readability a bit (since the intent will be obvious given the function name).

A bit of a shame we didn't use the helper here. Consistency would be nice.

kwilczynski · 2024-08-20T05:18:23Z

src/libcrun/linux.c

-          char *dirname;
-          cleanup_free char *buffer = NULL;
          cleanup_close int dirfd = -1;
-          char *basename, *tmp;
+          cleanup_free char *dirname = NULL;
+          char *basename, *found;

-          buffer = xstrdup (device->path);
-          dirname = buffer;
+          dirname = strdup (normalized_path);

-          tmp = strrchr (buffer, '/');
-          *tmp = '\0';
-          basename = tmp + 1;
+          found = strrchr (dirname, '/');
+          if (found)
+            *found = '\0';
+
+          basename = found ? found + 1 : dirname;


@giuseppe, are you OK with this refactoring here? Especially how the basename will be set to dirname. I want to make sure we won't break anything.

that seems OK to me

packit-as-a-service · 2024-08-28T09:21:01Z

podman system tests failed. @containers/packit-build please check.

giuseppe · 2024-09-02T13:01:53Z

src/libcrun/linux.c


-          buffer = xstrdup (device->path);
-          dirname = buffer;
+          dirname = strdup (normalized_path);


strdup can return NULL. Let's use xstrdup

giuseppe · 2024-09-02T13:02:02Z

src/libcrun/linux.c

-          char *dirname;
-          cleanup_free char *buffer = NULL;
          cleanup_close int dirfd = -1;
-          char *basename, *tmp;
+          cleanup_free char *dirname = NULL;
+          char *basename, *found;

-          buffer = xstrdup (device->path);
-          dirname = buffer;
+          dirname = strdup (normalized_path);

-          tmp = strrchr (buffer, '/');
-          *tmp = '\0';
-          basename = tmp + 1;
+          found = strrchr (dirname, '/');
+          if (found)
+            *found = '\0';
+
+          basename = found ? found + 1 : dirname;


that seems OK to me

packit-as-a-service · 2024-09-03T12:04:09Z

Ephemeral COPR build failed. @containers/packit-build please check.

packit-as-a-service · 2024-09-03T12:12:05Z

podman system tests failed. @containers/packit-build please check.

giuseppe · 2024-09-03T14:40:26Z

src/libcrun/linux.c

@@ -1611,7 +1611,14 @@ libcrun_create_dev (libcrun_container_t *container, int devfd, int srcfd,
  int rootfsfd = get_private_data (container)->rootfsfd;
  const char *rootfs = get_private_data (container)->rootfs;
  size_t rootfs_len = get_private_data (container)->rootfs_len;
-  const char *rel_dev = relative_path_under_dev (device->path);
+  if (is_empty_string (fullname))
+    return crun_make_error (err, EINVAL, "fullname is empty");


users don't know what fullname is. Let's clarify the error message

This change handles device paths with trailing slashes correctly. Signed-off-by: Sohan Kunkerkar <sohank2602@gmail.com>

packit-as-a-service · 2024-09-03T15:01:43Z

Ephemeral COPR build failed. @containers/packit-build please check.

packit-as-a-service · 2024-09-03T15:12:33Z

podman system tests failed. @containers/packit-build please check.

giuseppe

LGTM

sohankunkerkar force-pushed the fix-trailing-slash branch from 75f8758 to ed67bf9 Compare August 12, 2024 16:54

flouthoc reviewed Aug 12, 2024

View reviewed changes

src/libcrun/linux.c Outdated Show resolved Hide resolved

sohankunkerkar force-pushed the fix-trailing-slash branch 2 times, most recently from ddc5c33 to ede6e99 Compare August 12, 2024 18:20

kolyshkin reviewed Aug 12, 2024

View reviewed changes

giuseppe reviewed Aug 12, 2024

View reviewed changes

src/libcrun/linux.c Outdated Show resolved Hide resolved

sohankunkerkar force-pushed the fix-trailing-slash branch from ede6e99 to b64fe9a Compare August 13, 2024 02:56