[PATCH] libsemanage: Fallback to semanage_copy_dir when rename() failed

[PATCH] libsemanage: Fallback to semanage_copy_dir when rename() failed

[Petr Lautrbach]

In some circumstances, like semanage-store being on overlayfs, rename()
could fail with EXDEV - Invalid cross-device link. This is due to fact
that overlays doesn't support rename() is source and target are not on
the same layer, e.g. in rpm-ostree based containers. Even though it's
not atomic operation, it's better to try to to copy files from src to
dst on our own in this case. Next rebuild will probably not fail as the
new directories will be on the same layer.

Fixes: https://github.com/SELinuxProject/selinux/issues/343
Before:
    # semodule -B
    libsemanage.semanage_commit_sandbox: Error while renaming /etc/selinux/targeted/active to /etc/selinux/targeted/previous. (Invalid cross-device link).
semodule:  Failed!

After:
    # semodule -B
    Warning: rename(/etc/selinux/targeted/active, /etc/selinux/targeted/previous) failed: Invalid cross-device link, fallback to non-atomic semanage_copy_dir_flags()

Reported-by: Joseph Marrero Corchado <jmarrero@redhat.com>
Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
---
 libsemanage/src/semanage_store.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index 767f05cb2853..aa44ebb7e481 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -697,6 +697,10 @@ int semanage_store_access_check(void)
 
 /********************* other I/O functions *********************/
 
+static int semanage_rename(const char *tmp, const char *dst);
+int semanage_remove_directory(const char *path);
+static int semanage_copy_dir_flags(const char *src, const char *dst, int flag);
+
 /* Callback used by scandir() to select files. */
 static int semanage_filename_select(const struct dirent *d)
 {
@@ -760,7 +764,7 @@ int semanage_copy_file(const char *src, const char *dst, mode_t mode,
 		retval = -1;
 	}
 
-	if (!retval && rename(tmp, dst) == -1)
+	if (!retval && semanage_rename(tmp, dst) == -1)
 		return -1;
 
 out:
@@ -768,7 +772,20 @@ out:
 	return retval;
 }
 
-static int semanage_copy_dir_flags(const char *src, const char *dst, int flag);
+static int semanage_rename(const char *src, const char *dst) {
+	int retval = -1;
+
+	retval = rename(src, dst);
+	if (retval == 0 || errno != EXDEV)
+		return retval;
+
+	/* we can't use rename() due to filesystem limitation, lets try to copy files manually */
+	fprintf(stderr, "Warning: rename(%s, %s) failed: %s, fallback to non-atomic semanage_copy_dir_flags()\n", src, dst, strerror(errno));
+	if (semanage_copy_dir_flags(src, dst, 1) == -1) {
+		return -1;
+	}
+	return semanage_remove_directory(src);
+}
 
 /* Copies all of the files from src to dst, recursing into
  * subdirectories.  Returns 0 on success, -1 on error. */
@@ -1770,7 +1787,7 @@ static int semanage_commit_sandbox(semanage_handle_t * sh)
 		goto cleanup;
 	}
 
-	if (rename(active, backup) == -1) {
+	if (semanage_rename(active, backup) == -1) {
 		ERR(sh, "Error while renaming %s to %s.", active, backup);
 		retval = -1;
 		goto cleanup;
@@ -1779,12 +1796,12 @@ static int semanage_commit_sandbox(semanage_handle_t * sh)
 	/* clean up some files from the sandbox before install */
 	/* remove homedir_template from sandbox */
 
-	if (rename(sandbox, active) == -1) {
+	if (semanage_rename(sandbox, active) == -1) {
 		ERR(sh, "Error while renaming %s to %s.", sandbox, active);
 		/* note that if an error occurs during the next
 		 * function then the store will be left in an
 		 * inconsistent state */
-		if (rename(backup, active) < 0)
+		if (semanage_rename(backup, active) < 0)
 			ERR(sh, "Error while renaming %s back to %s.", backup,
 			    active);
 		retval = -1;
@@ -1795,10 +1812,10 @@ static int semanage_commit_sandbox(semanage_handle_t * sh)
 		 * function then the store will be left in an
 		 * inconsistent state */
 		int errsv = errno;
-		if (rename(active, sandbox) < 0)
+		if (semanage_rename(active, sandbox) < 0)
 			ERR(sh, "Error while renaming %s back to %s.", active,
 			    sandbox);
-		else if (rename(backup, active) < 0)
+		else if (semanage_rename(backup, active) < 0)
 			ERR(sh, "Error while renaming %s back to %s.", backup,
 			    active);
 		else
-- 
2.35.1

Re: [PATCH] libsemanage: Fallback to semanage_copy_dir when rename() failed

[Ondrej Mosnacek]

On Wed, Mar 23, 2022 at 6:34 PM Petr Lautrbach <plautrba@redhat.com> wrote:

In subject: s/Fallback/Fall back/ and s/failed/fails/

Other than a couple nits above and below the patch looks good to me.

I think we could make the fallback rename atomic by creating a file
before the rename, removing it afterwards, and checking if it exists
at the beginning of each transaction (in case it does, we would retry
the rename from scratch before proceeding). But maybe it's not worth
the hassle for such a corner case...

> In some circumstances, like semanage-store being on overlayfs, rename()
> could fail with EXDEV - Invalid cross-device link. This is due to fact

due to the

> that overlays doesn't support rename() is source and target are not on

s/is/if/

> the same layer, e.g. in rpm-ostree based containers. Even though it's
> not atomic operation, it's better to try to to copy files from src to

s/to to/to/

> dst on our own in this case. Next rebuild will probably not fail as the
> new directories will be on the same layer.
>
> Fixes: https://github.com/SELinuxProject/selinux/issues/343
> Before:
>     # semodule -B
>     libsemanage.semanage_commit_sandbox: Error while renaming /etc/selinux/targeted/active to /etc/selinux/targeted/previous. (Invalid cross-device link).
> semodule:  Failed!
>
> After:
>     # semodule -B
>     Warning: rename(/etc/selinux/targeted/active, /etc/selinux/targeted/previous) failed: Invalid cross-device link, fallback to non-atomic semanage_copy_dir_flags()
>
> Reported-by: Joseph Marrero Corchado <jmarrero@redhat.com>
> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
> ---
>  libsemanage/src/semanage_store.c | 31 ++++++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
> index 767f05cb2853..aa44ebb7e481 100644
> --- a/libsemanage/src/semanage_store.c
> +++ b/libsemanage/src/semanage_store.c
> @@ -697,6 +697,10 @@ int semanage_store_access_check(void)
>
>  /********************* other I/O functions *********************/
>
> +static int semanage_rename(const char *tmp, const char *dst);
> +int semanage_remove_directory(const char *path);rename
> +static int semanage_copy_dir_flags(const char *src, const char *dst, int flag);
> +
>  /* Callback used by scandir() to select files. */
>  static int semanage_filename_select(const struct dirent *d)
>  {
> @@ -760,7 +764,7 @@ int semanage_copy_file(const char *src, const char *dst, mode_t mode,
>                 retval = -1;
>         }
>
> -       if (!retval && rename(tmp, dst) == -1)
> +       if (!retval && semanage_rename(tmp, dst) == -1)
>                 return -1;
>
>  out:
> @@ -768,7 +772,20 @@ out:
>         return retval;
>  }
>
> -static int semanage_copy_dir_flags(const char *src, const char *dst, int flag);
> +static int semanage_rename(const char *src, const char *dst) {
> +       int retval = -1;

The value is immediately overwritten, so there is no need to
initialize retval to -1 here.

> +
> +       retval = rename(src, dst);
> +       if (retval == 0 || errno != EXDEV)
> +               return retval;
> +
> +       /* we can't use rename() due to filesystem limitation, lets try to copy files manually */
> +       fprintf(stderr, "Warning: rename(%s, %s) failed: %s, fallback to non-atomic semanage_copy_dir_flags()\n", src, dst, strerror(errno));

I believe this should use the WARN() macro from debug.h instead. Also
s/fallback/fall back/.

> +       if (semanage_copy_dir_flags(src, dst, 1) == -1) {
> +               return -1;
> +       }
> +       return semanage_remove_directory(src);
> +}
>
>  /* Copies all of the files from src to dst, recursing into
>   * subdirectories.  Returns 0 on success, -1 on error. */
> @@ -1770,7 +1787,7 @@ static int semanage_commit_sandbox(semanage_handle_t * sh)
>                 goto cleanup;
>         }
>
> -       if (rename(active, backup) == -1) {
> +       if (semanage_rename(active, backup) == -1) {
>                 ERR(sh, "Error while renaming %s to %s.", active, backup);
>                 retval = -1;
>                 goto cleanup;
> @@ -1779,12 +1796,12 @@ static int semanage_commit_sandbox(semanage_handle_t * sh)
>         /* clean up some files from the sandbox before install */
>         /* remove homedir_template from sandbox */
>
> -       if (rename(sandbox, active) == -1) {
> +       if (semanage_rename(sandbox, active) == -1) {
>                 ERR(sh, "Error while renaming %s to %s.", sandbox, active);
>                 /* note that if an error occurs during the next
>                  * function then the store will be left in an
>                  * inconsistent state */
> -               if (rename(backup, active) < 0)
> +               if (semanage_rename(backup, active) < 0)
>                         ERR(sh, "Error while renaming %s back to %s.", backup,
>                             active);
>                 retval = -1;
> @@ -1795,10 +1812,10 @@ static int semanage_commit_sandbox(semanage_handle_t * sh)
>                  * function then the store will be left in an
>                  * inconsistent state */
>                 int errsv = errno;
> -               if (rename(active, sandbox) < 0)
> +               if (semanage_rename(active, sandbox) < 0)
>                         ERR(sh, "Error while renaming %s back to %s.", active,
>                             sandbox);
> -               else if (rename(backup, active) < 0)
> +               else if (semanage_rename(backup, active) < 0)
>                         ERR(sh, "Error while renaming %s back to %s.", backup,
>                             active);
>                 else
> --
> 2.35.1
>

-- 
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

[PATCH v2] libsemanage: Fall back to semanage_copy_dir when rename() fails

[Petr Lautrbach]

In some circumstances, like semanage-store being on overlayfs, rename()
could fail with EXDEV - Invalid cross-device link. This is due to the
fact that overlays doesn't support rename() if source and target are not
on the same layer, e.g. in containers built from several layers. Even
though it's not atomic operation, it's better to try to copy files from
src to dst on our own in this case. Next rebuild will probably not fail
as the new directories will be on the same layer.

Fixes: https://github.com/SELinuxProject/selinux/issues/343

Reproducer:

    $ cd selinux1

    $ cat Dockerfile
    FROM fedora:35
    RUN dnf install -y selinux-policy selinux-policy-targeted

    $ podman build -t localhost/selinux . --no-cache

    $ cd ../selinux2

    $ cat Dockerfile
    FROM localhost/selinux
    RUN semodule -B

    $ podman build -t localhost/selinux2 . --no-cache
    STEP 2/2: RUN semodule -B
    libsemanage.semanage_commit_sandbox: Error while renaming /var/lib/selinux/targeted/active to /var/lib/selinux/targeted/previous. (Invalid cross-device link).
    semodule:  Failed!
    Error: error building at STEP "RUN semodule -B": error while running runtime: exit status 1

With the fix:

    $ podman build -t localhost/selinux2 . --no-cache
    STEP 2/2: RUN semodule -B
    libsemanage.semanage_rename: Warning: rename(/var/lib/selinux/targeted/active, /var/lib/selinux/targeted/previous) failed: Invalid cross-device link, fall back to non-atomic semanage_copy_dir_flags()

    COMMIT localhost/selinux2
    --> d2cfcebc1a1
    Successfully tagged localhost/selinux2:latest
    d2cfcebc1a1b34f1c2cd661ac18292b0612c3e5fa71d6fa1441be244da91b1af

Reported-by: Joseph Marrero Corchado <jmarrero@redhat.com>
Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
---
changes to v1 base on Ondrej's feedback:

- improve the commit message
- use WARN() instead of fprintf(stderr, 

 libsemanage/src/semanage_store.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index 767f05cb2853..3ae60493c8e5 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -697,6 +697,10 @@ int semanage_store_access_check(void)
 
 /********************* other I/O functions *********************/
 
+static int semanage_rename(semanage_handle_t * sh, const char *tmp, const char *dst);
+int semanage_remove_directory(const char *path);
+static int semanage_copy_dir_flags(const char *src, const char *dst, int flag);
+
 /* Callback used by scandir() to select files. */
 static int semanage_filename_select(const struct dirent *d)
 {
@@ -768,7 +772,20 @@ out:
 	return retval;
 }
 
-static int semanage_copy_dir_flags(const char *src, const char *dst, int flag);
+static int semanage_rename(semanage_handle_t * sh, const char *src, const char *dst) {
+	int retval;
+
+	retval = rename(src, dst);
+	if (retval == 0 || errno != EXDEV)
+		return retval;
+
+	/* we can't use rename() due to filesystem limitation, lets try to copy files manually */
+	WARN(sh, "Warning: rename(%s, %s) failed: %s, fall back to non-atomic semanage_copy_dir_flags()\n", src, dst, strerror(errno));
+	if (semanage_copy_dir_flags(src, dst, 1) == -1) {
+		return -1;
+	}
+	return semanage_remove_directory(src);
+}
 
 /* Copies all of the files from src to dst, recursing into
  * subdirectories.  Returns 0 on success, -1 on error. */
@@ -1770,7 +1787,7 @@ static int semanage_commit_sandbox(semanage_handle_t * sh)
 		goto cleanup;
 	}
 
-	if (rename(active, backup) == -1) {
+	if (semanage_rename(sh, active, backup) == -1) {
 		ERR(sh, "Error while renaming %s to %s.", active, backup);
 		retval = -1;
 		goto cleanup;
@@ -1779,12 +1796,12 @@ static int semanage_commit_sandbox(semanage_handle_t * sh)
 	/* clean up some files from the sandbox before install */
 	/* remove homedir_template from sandbox */
 
-	if (rename(sandbox, active) == -1) {
+	if (semanage_rename(sh, sandbox, active) == -1) {
 		ERR(sh, "Error while renaming %s to %s.", sandbox, active);
 		/* note that if an error occurs during the next
 		 * function then the store will be left in an
 		 * inconsistent state */
-		if (rename(backup, active) < 0)
+		if (semanage_rename(sh, backup, active) < 0)
 			ERR(sh, "Error while renaming %s back to %s.", backup,
 			    active);
 		retval = -1;
@@ -1795,10 +1812,10 @@ static int semanage_commit_sandbox(semanage_handle_t * sh)
 		 * function then the store will be left in an
 		 * inconsistent state */
 		int errsv = errno;
-		if (rename(active, sandbox) < 0)
+		if (semanage_rename(sh, active, sandbox) < 0)
 			ERR(sh, "Error while renaming %s back to %s.", active,
 			    sandbox);
-		else if (rename(backup, active) < 0)
+		else if (semanage_rename(sh, backup, active) < 0)
 			ERR(sh, "Error while renaming %s back to %s.", backup,
 			    active);
 		else
-- 
2.35.1

[PATCH v3] libsemanage: Fall back to semanage_copy_dir when rename() fails

[Petr Lautrbach]

In some circumstances, like semanage-store being on overlayfs, rename()
could fail with EXDEV - Invalid cross-device link. This is due to the
fact that overlays doesn't support rename() if source and target are not
on the same layer, e.g. in containers built from several layers. Even
though it's not atomic operation, it's better to try to copy files from
src to dst on our own in this case. Next rebuild will probably not fail
as the new directories will be on the same layer.

Fixes: https://github.com/SELinuxProject/selinux/issues/343

Reproducer:

    $ cd selinux1

    $ cat Dockerfile
    FROM fedora:35
    RUN dnf install -y selinux-policy selinux-policy-targeted

    $ podman build -t localhost/selinux . --no-cache

    $ cd ../selinux2

    $ cat Dockerfile
    FROM localhost/selinux
    RUN semodule -B

    $ podman build -t localhost/selinux2 . --no-cache
    STEP 2/2: RUN semodule -B
    libsemanage.semanage_commit_sandbox: Error while renaming /var/lib/selinux/targeted/active to /var/lib/selinux/targeted/previous. (Invalid cross-device link).
    semodule:  Failed!
    Error: error building at STEP "RUN semodule -B": error while running runtime: exit status 1

With the fix:

    $ podman build -t localhost/selinux2 . --no-cache
    STEP 2/2: RUN semodule -B
    libsemanage.semanage_rename: Warning: rename(/var/lib/selinux/targeted/active, /var/lib/selinux/targeted/previous) failed: Invalid cross-device link, fall back to non-atomic semanage_copy_dir_flags()

    COMMIT localhost/selinux2
    --> d2cfcebc1a1
    Successfully tagged localhost/selinux2:latest
    d2cfcebc1a1b34f1c2cd661ac18292b0612c3e5fa71d6fa1441be244da91b1af

Reported-by: Joseph Marrero Corchado <jmarrero@redhat.com>
Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
---

v2
- improve the commit message
- use WARN() instead of fprintf(stderr, 

v3 
- WARN without \n at the end
- split long line

libsemanage/src/semanage_store.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/libsemanage/src/semanage_store.c b/libsemanage/src/semanage_store.c
index 767f05cb2853..c6d2c5e72709 100644
--- a/libsemanage/src/semanage_store.c
+++ b/libsemanage/src/semanage_store.c
@@ -697,6 +697,10 @@ int semanage_store_access_check(void)
 
 /********************* other I/O functions *********************/
 
+static int semanage_rename(semanage_handle_t * sh, const char *tmp, const char *dst);
+int semanage_remove_directory(const char *path);
+static int semanage_copy_dir_flags(const char *src, const char *dst, int flag);
+
 /* Callback used by scandir() to select files. */
 static int semanage_filename_select(const struct dirent *d)
 {
@@ -768,7 +772,21 @@ out:
 	return retval;
 }
 
-static int semanage_copy_dir_flags(const char *src, const char *dst, int flag);
+static int semanage_rename(semanage_handle_t * sh, const char *src, const char *dst) {
+	int retval;
+
+	retval = rename(src, dst);
+	if (retval == 0 || errno != EXDEV)
+		return retval;
+
+	/* we can't use rename() due to filesystem limitation, lets try to copy files manually */
+	WARN(sh, "WARNING: rename(%s, %s) failed: %s, fall back to non-atomic semanage_copy_dir_flags()",
+		 src, dst, strerror(errno));
+	if (semanage_copy_dir_flags(src, dst, 1) == -1) {
+		return -1;
+	}
+	return semanage_remove_directory(src);
+}
 
 /* Copies all of the files from src to dst, recursing into
  * subdirectories.  Returns 0 on success, -1 on error. */
@@ -1770,7 +1788,7 @@ static int semanage_commit_sandbox(semanage_handle_t * sh)
 		goto cleanup;
 	}
 
-	if (rename(active, backup) == -1) {
+	if (semanage_rename(sh, active, backup) == -1) {
 		ERR(sh, "Error while renaming %s to %s.", active, backup);
 		retval = -1;
 		goto cleanup;
@@ -1779,12 +1797,12 @@ static int semanage_commit_sandbox(semanage_handle_t * sh)
 	/* clean up some files from the sandbox before install */
 	/* remove homedir_template from sandbox */
 
-	if (rename(sandbox, active) == -1) {
+	if (semanage_rename(sh, sandbox, active) == -1) {
 		ERR(sh, "Error while renaming %s to %s.", sandbox, active);
 		/* note that if an error occurs during the next
 		 * function then the store will be left in an
 		 * inconsistent state */
-		if (rename(backup, active) < 0)
+		if (semanage_rename(sh, backup, active) < 0)
 			ERR(sh, "Error while renaming %s back to %s.", backup,
 			    active);
 		retval = -1;
@@ -1795,10 +1813,10 @@ static int semanage_commit_sandbox(semanage_handle_t * sh)
 		 * function then the store will be left in an
 		 * inconsistent state */
 		int errsv = errno;
-		if (rename(active, sandbox) < 0)
+		if (semanage_rename(sh, active, sandbox) < 0)
 			ERR(sh, "Error while renaming %s back to %s.", active,
 			    sandbox);
-		else if (rename(backup, active) < 0)
+		else if (semanage_rename(sh, backup, active) < 0)
 			ERR(sh, "Error while renaming %s back to %s.", backup,
 			    active);
 		else
-- 
2.35.1

Re: [PATCH v3] libsemanage: Fall back to semanage_copy_dir when rename() fails

[Ondrej Mosnacek]

On Thu, Mar 24, 2022 at 1:01 PM Petr Lautrbach <plautrba@redhat.com> wrote:
> In some circumstances, like semanage-store being on overlayfs, rename()
> could fail with EXDEV - Invalid cross-device link. This is due to the
> fact that overlays doesn't support rename() if source and target are not
> on the same layer, e.g. in containers built from several layers. Even
> though it's not atomic operation, it's better to try to copy files from
> src to dst on our own in this case. Next rebuild will probably not fail
> as the new directories will be on the same layer.
>
> Fixes: https://github.com/SELinuxProject/selinux/issues/343
>
> Reproducer:
>
>     $ cd selinux1
>
>     $ cat Dockerfile
>     FROM fedora:35
>     RUN dnf install -y selinux-policy selinux-policy-targeted
>
>     $ podman build -t localhost/selinux . --no-cache
>
>     $ cd ../selinux2
>
>     $ cat Dockerfile
>     FROM localhost/selinux
>     RUN semodule -B
>
>     $ podman build -t localhost/selinux2 . --no-cache
>     STEP 2/2: RUN semodule -B
>     libsemanage.semanage_commit_sandbox: Error while renaming /var/lib/selinux/targeted/active to /var/lib/selinux/targeted/previous. (Invalid cross-device link).
>     semodule:  Failed!
>     Error: error building at STEP "RUN semodule -B": error while running runtime: exit status 1
>
> With the fix:
>
>     $ podman build -t localhost/selinux2 . --no-cache
>     STEP 2/2: RUN semodule -B
>     libsemanage.semanage_rename: Warning: rename(/var/lib/selinux/targeted/active, /var/lib/selinux/targeted/previous) failed: Invalid cross-device link, fall back to non-atomic semanage_copy_dir_flags()
>
>     COMMIT localhost/selinux2
>     --> d2cfcebc1a1
>     Successfully tagged localhost/selinux2:latest
>     d2cfcebc1a1b34f1c2cd661ac18292b0612c3e5fa71d6fa1441be244da91b1af
>
> Reported-by: Joseph Marrero Corchado <jmarrero@redhat.com>
> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
> ---
>
> v2
> - improve the commit message
> - use WARN() instead of fprintf(stderr,
>
> v3
> - WARN without \n at the end
> - split long line

Acked-by: Ondrej Mosnacek <omosnace@redhat.com>

Note that I didn't give the logic a thorough review, so I'd prefer
someone else to give it a final look and merge it.

-- 
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

[PATCH v2 3/3] mcstrans: Fir RESOURCE_LEAK and USE_AFTER_FREE coverity scan defects

[Petr Lautrbach]

Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
---
 mcstrans/src/mcstrans.c  | 25 ++++++++++++++++++++++++-
 mcstrans/src/mcstransd.c |  4 +++-
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/mcstrans/src/mcstrans.c b/mcstrans/src/mcstrans.c
index d42760fdbfc2..af3f507ef718 100644
--- a/mcstrans/src/mcstrans.c
+++ b/mcstrans/src/mcstrans.c
@@ -632,16 +632,23 @@ add_cache(domain_t *domain, char *raw, char *trans) {
 
 	map->raw = strdup(raw);
 	if (!map->raw) {
+		free(map);
 		goto err;
 	}
 	map->trans = strdup(trans);
 	if (!map->trans) {
+		free(map->raw);
+		free(map);
 		goto err;
 	}
 
 	log_debug(" add_cache (%s,%s)\n", raw, trans);
-	if (add_to_hashtable(domain->raw_to_trans, map->raw, map) < 0)
+	if (add_to_hashtable(domain->raw_to_trans, map->raw, map) < 0) {
+		free(map->trans);
+		free(map->raw);
+		free(map);
 		goto err;
+	}
 
 	if (add_to_hashtable(domain->trans_to_raw, map->trans, map) < 0)
 		goto err;
@@ -1568,6 +1575,7 @@ trans_context(const char *incon, char **rcon) {
 			trans = compute_trans_from_raw(range, domain);
 			if (trans)
 				if (add_cache(domain, range, trans) < 0) {
+					free(trans);
 					free(range);
 					return -1;
 				}
@@ -1579,6 +1587,7 @@ trans_context(const char *incon, char **rcon) {
 				ltrans = compute_trans_from_raw(lrange, domain);
 				if (ltrans) {
 					if (add_cache(domain, lrange, ltrans) < 0) {
+						free(ltrans);
 						free(range);
 						return -1;
 					}
@@ -1597,6 +1606,7 @@ trans_context(const char *incon, char **rcon) {
 				utrans = compute_trans_from_raw(urange, domain);
 				if (utrans) {
 					if (add_cache(domain, urange, utrans) < 0) {
+						free(utrans);
 						free(ltrans);
 						free(range);
 						return -1;
@@ -1636,6 +1646,10 @@ trans_context(const char *incon, char **rcon) {
 		}
 		if (dashp)
 			*dashp = '-';
+		if (trans) {
+			free(trans);
+			trans = NULL;
+		}
 	}
 
 	if (trans) {
@@ -1696,7 +1710,9 @@ untrans_context(const char *incon, char **rcon) {
 					canonical = compute_trans_from_raw(raw, domain);
 					if (canonical && strcmp(canonical, range))
 						if (add_cache(domain, raw, canonical) < 0) {
+							free(canonical);
 							free(range);
+							free(raw);
 							return -1;
 						}
 				}
@@ -1704,6 +1720,7 @@ untrans_context(const char *incon, char **rcon) {
 					free(canonical);
 				if (add_cache(domain, raw, range) < 0) {
 					free(range);
+					free(raw);
 					return -1;
 				}
 			} else {
@@ -1721,6 +1738,7 @@ untrans_context(const char *incon, char **rcon) {
 						canonical = compute_trans_from_raw(lraw, domain);
 						if (canonical)
 							if (add_cache(domain, lraw, canonical) < 0) {
+								free(canonical);
 								free(lraw);
 								free(range);
 								return -1;
@@ -1752,6 +1770,7 @@ untrans_context(const char *incon, char **rcon) {
 						canonical = compute_trans_from_raw(uraw, domain);
 						if (canonical)
 							if (add_cache(domain, uraw, canonical) < 0) {
+								free(canonical);
 								free(uraw);
 								free(lraw);
 								free(range);
@@ -1802,6 +1821,10 @@ untrans_context(const char *incon, char **rcon) {
 		}
 		if (dashp)
 			*dashp = '-';
+		if (raw) {
+			free(raw);
+			raw = NULL;
+		}
 	}
 
 	if (raw) {
diff --git a/mcstrans/src/mcstransd.c b/mcstrans/src/mcstransd.c
index 536c0f32f23a..42262e580386 100644
--- a/mcstrans/src/mcstransd.c
+++ b/mcstrans/src/mcstransd.c
@@ -328,6 +328,7 @@ process_events(struct pollfd **ufds, int *nfds)
 					/* Setup pollfd for deletion later. */
 					(*ufds)[ii].fd = -1;
 					close(connfd);
+					connfd = -1;
 					/* So we don't get bothered later */
 					revents = revents & ~(POLLHUP);
 				}
@@ -341,10 +342,11 @@ process_events(struct pollfd **ufds, int *nfds)
 			/* Set the pollfd up for deletion later. */
 			(*ufds)[ii].fd = -1;
 			close(connfd);
+			connfd = -1;
 
 			revents = revents & ~(POLLHUP);
 		}
-		if (revents) {
+		if (revents && connfd != -1) {
 			syslog(LOG_ERR, "Unknown/error events (%x) encountered"
 					" for fd (%d)\n", revents, connfd);
 
-- 
2.35.1

Re: [PATCH v3] libsemanage: Fall back to semanage_copy_dir when rename() fails

[Petr Lautrbach]

Ondrej Mosnacek <omosnace@redhat.com> writes:

> On Thu, Mar 24, 2022 at 1:01 PM Petr Lautrbach <plautrba@redhat.com> wrote:
>> In some circumstances, like semanage-store being on overlayfs, rename()
>> could fail with EXDEV - Invalid cross-device link. This is due to the
>> fact that overlays doesn't support rename() if source and target are not
>> on the same layer, e.g. in containers built from several layers. Even
>> though it's not atomic operation, it's better to try to copy files from
>> src to dst on our own in this case. Next rebuild will probably not fail
>> as the new directories will be on the same layer.
>>
>> Fixes: https://github.com/SELinuxProject/selinux/issues/343
>>
>> Reproducer:
>>
>>     $ cd selinux1
>>
>>     $ cat Dockerfile
>>     FROM fedora:35
>>     RUN dnf install -y selinux-policy selinux-policy-targeted
>>
>>     $ podman build -t localhost/selinux . --no-cache
>>
>>     $ cd ../selinux2
>>
>>     $ cat Dockerfile
>>     FROM localhost/selinux
>>     RUN semodule -B
>>
>>     $ podman build -t localhost/selinux2 . --no-cache
>>     STEP 2/2: RUN semodule -B
>>     libsemanage.semanage_commit_sandbox: Error while renaming /var/lib/selinux/targeted/active to /var/lib/selinux/targeted/previous. (Invalid cross-device link).
>>     semodule:  Failed!
>>     Error: error building at STEP "RUN semodule -B": error while running runtime: exit status 1
>>
>> With the fix:
>>
>>     $ podman build -t localhost/selinux2 . --no-cache
>>     STEP 2/2: RUN semodule -B
>>     libsemanage.semanage_rename: Warning: rename(/var/lib/selinux/targeted/active, /var/lib/selinux/targeted/previous) failed: Invalid cross-device link, fall back to non-atomic semanage_copy_dir_flags()
>>
>>     COMMIT localhost/selinux2
>>     --> d2cfcebc1a1
>>     Successfully tagged localhost/selinux2:latest
>>     d2cfcebc1a1b34f1c2cd661ac18292b0612c3e5fa71d6fa1441be244da91b1af
>>
>> Reported-by: Joseph Marrero Corchado <jmarrero@redhat.com>
>> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
>> ---
>>
>> v2
>> - improve the commit message
>> - use WARN() instead of fprintf(stderr,
>>
>> v3
>> - WARN without \n at the end
>> - split long line
>
> Acked-by: Ondrej Mosnacek <omosnace@redhat.com>
>
> Note that I didn't give the logic a thorough review, so I'd prefer
> someone else to give it a final look and merge it.

Merged.



> -- 
> Ondrej Mosnacek
> Software Engineer, Linux Security - SELinux kernel
> Red Hat, Inc.