fix(3207, 3209) Difference between the exec command in runc and youki

[tommady]

Description

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update
• Refactoring (no functional changes)
• Performance improvement
• Test updates
• CI/CD related changes
• Other (please describe):

Testing

• Added new unit tests
• Added new integration tests
• Ran existing test suite
• Tested manually (please provide steps)

Related Issues

Fixes #3207 #3209

Additional Context

[saku3]

Sorry for the delayed review.

I left comments on with_apparmor and ignore_paused_test.rs. Could you please take a look?

Also, since this(998d1c2 and 691e046) change is unrelated to the issue we’re addressing here, let’s handle it in a separate PR.

[saku3]

with_apparmor seems to be missing.

[saku3]

Sorry, I understand now.

In this case, I think the previous implementation (adding a short sleep) is acceptable.
It aligns with runc’s behavior, so I don’t expect it to cause issues in practice.

Also, since the spawn order doesn’t guarantee the actual execution order, resume may run first, meaning the test might not actually verify that exec works while the container is paused.

[saku3]

I think everything looks mostly fine aside from the parts I commented on.

PTAL @utam0k

[tommady]

it’s strange that both integration verifications failed

# Start group example
1 / 1 : hello world : not ok
	container stderr was not empty, found : 
thread 'main' (1) panicked at library/std/src/io/stdio.rs:1165:9:
failed printing to stdout: Broken pipe (os error 32)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I couldn’t reproduce this on my local machine

[saku3]

This is tough...

I checked it in my repository’s GitHub Actions as well.

• Running only the example and exec tests → fails.
• Running the tests with batch_size set to 1 → succeeds.
  • batch_size

It’s possible the exec tests aren’t cleaning up properly.

[tommady]

This is tough...

I checked it in my repository’s GitHub Actions as well.

• Running only the example and exec tests → fails.

• Running the tests with batch_size set to 1 → succeeds.

  • batch_size

It’s possible the exec tests aren’t cleaning up properly.

Hi @saku3, @utam0k,

You were absolutely spot on with your observation about batch_size=1.
The issue was indeed caused by tests interfering with each other when run concurrently!

I've pushed a fix for this. The root cause was that preserve_fds_test was modifying the global file descriptor table in the parent contest test runner process by manually clearing the FD_CLOEXEC flag and running dup2(fd, 3).

Because contest runs tests concurrently by default, this led to two distinct issues:

1. Race condition in hello_world: Overwriting fd=3 in the parent silently closed the pipe stdout used by other concurrently running tests (like hello_world), causing them to panic with a "Broken pipe" (OS error 32).
2. FD Leak causing EPERM: Clearing the O_CLOEXEC flag on the host file descriptor caused it to leak into child processes spawned by other tests, which interfered with mount/remount operations in new namespaces and caused EPERM errors.

To fix this, the file descriptor modifications have been confined entirely to the child process specifically spawned for preserve_fds_test by using CommandExt::pre_exec.
This allows us to run dup2(fd, 3) safely after fork() but before exec(), preventing any side effects on the test runner or other concurrent tests.

Please feel free to review the changes when you have time.
Thanks.

[saku3]

@tommady
Sorry for the delay.
I haven’t had much time since my last review.
I’ll review this within the next three days.
In the meantime, could you please resolve the conflicts?

[saku3]

Thank you for working through this patiently.

I added one more comment.

I think the preserve_fds_test looks good.

[saku3]

I missed that.

With the current condition, statuses that should not allow exec, such as Stopped, would end up allowing exec.

I think it would be better to handle it like this.
(I wrote it this way for readability first, though it may be possible to simplify it further.)

        if container.status() == ContainerStatus::Paused {
            if !self.ignore_paused {
                return Err(...);
            }
        } else if !container.can_exec() {
            return Err(...);
        }

[tommady]

Great catch on that logic bug!

I think using match is the way to go here. It’s a lot more readable than the nested if statements and makes it much easier to see exactly which statuses allow execution and which don't.

Thanks for pointing that out!

[saku3]

LGTM.

Thank you so, so much!