update nix to 0.27.1 #2369

anti-entropy123 · 2023-09-19T13:28:31Z

I'm currently working on issue #2317.

note: the following content is now outdated

In the code changes I've made, there are two things that might raise questions:

The use of std::mem::forget().
The use of an unsafe function OwnedFd::from_raw_fd().

I use forget() because some functions in the nix library now return an OwnedFd. This helps avoid accidentally closing the file descriptor too soon by preventing its automatic drop. Maybe fully relying on OwnedFd to manage the fd's lifetime instead of manually calling close on RawFd is a better approach, but I think this could require quite a bit of code changes.

I also use OwnedFd::from_raw_fd() because of the nix library. The nix::sched::setns() function now requires an Fd: AsFd as one of its parameters, and I couldn't find a better way to convert an i32 into Fd: AsFd.

Additionally, this is my first time contributing code to an open-source repository. If there are any important details I've missed, please let me know.

codecov-commenter · 2023-09-20T00:12:18Z

Codecov Report

Merging #2369 (951e5c6) into main (fee4641) will increase coverage by 0.02%.
Report is 8 commits behind head on main.
The diff coverage is 91.83%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2369      +/-   ##
==========================================
+ Coverage   65.98%   66.01%   +0.02%     
==========================================
  Files         133      133              
  Lines       16814    16832      +18     
==========================================
+ Hits        11095    11111      +16     
- Misses       5719     5721       +2

YJDoc2 · 2023-09-20T14:56:18Z

Hey, thanks for the PR. I think there was someone else also assigned to this you should also ping them .
As there are a lot of changes, I'll need some more time to review this, probably over the weekend.

utam0k · 2023-09-23T13:05:23Z

I'll take a look at this PR tomorrow.

utam0k

Thanks for your first contribution, and congrats your first OSS contribution! Awesome.
I left some comments.

utam0k · 2023-09-24T05:05:31Z

crates/libcontainer/src/channel.rs

+    std::mem::forget(os_sender);
+    std::mem::forget(os_receiver);


I use forget() because some functions in the nix library now return an OwnedFd. This helps avoid accidentally closing the file descriptor too soon by preventing its automatic drop. Maybe fully relying on OwnedFd to manage the fd's lifetime instead of manually calling close on RawFd is a better approach, but I think this could require quite a bit of code changes.

What should we do to support OwnedFd? I feel it is too dangerous for low-level container runtime because we must take care of the security risk.

For Sender and Receiver, the problem is relatively easy. I noticed that Sender::close() will closes the fd.

I don't have a solid plan for fully supporting OwnedFd. I can only say that I've tried to make my PR as compatible as possible with the previous implementation that used rawfd, and reuse the code that previously managed file descriptors.

The main difficulty right now is that if we changes entirely to OwnedFd, the console_socket in struct ContainerArgs (crates/libcontainer/src/process/args.rs:20) won't satisfy the Clone trait. Maybe need to introduce Rc<T>.

Should I avoid forget() in this PR, or address it in a separate issue? I'd like to hear your opinion.

Should I avoid forget() in this PR, or address it in a separate issue? I'd like to hear your opinion.

Thanks for your detailed investigation. This problem relates to the security issue. So I can't merge it before removing forget().

I agree with this cautious decision. I have some ideas for removing forget(), but it will take some time to implement.

Thanks for understanding 🙏

but it will take some time to implement.

NP 👍 Let's give it a try!

utam0k · 2023-09-24T05:08:20Z

crates/libcontainer/src/container/init_builder.rs

+                fd
+            }
+            None => -1,
+        });


What happens when it returns -1?

Apparently, we can combine L90-L94 into L81-89. Is there a reason why you separate them?

What happens when it returns -1?

It doesn't seem to make any difference compared to the past.
The previous setup_console_socket() would return Ok(-1) under certain conditions. Now, under the same conditions, it returns Ok(None).

Apparently, we can combine L90-L94 into L81-89. Is there a reason why you separate them?

There's nothing special. I can make the modification as you suggested.

utam0k · 2023-09-24T05:31:36Z

crates/libcontainer/src/process/fork.rs

@@ -164,12 +164,12 @@ fn clone(cb: CloneCb, flags: u64, exit_signal: Option<u64>) -> Result<Pid, Clone
    // do not use MAP_GROWSDOWN since it is not well supported.
    // Ref: https://man7.org/linux/man-pages/man2/mmap.2.html
    let child_stack = unsafe {
-        mman::mmap(
+        mman::mmap::<File>(


May I ask you to write a comment that File doesn't have any meaning because we won't use it.

it's related to the change in the function signature of nix::sys::mman::mmap(). The current signature of the mmap() function is:

pub unsafe fn mmap<F: AsFd>( addr: Option<NonZeroUsize>, length: NonZeroUsize, prot: ProtFlags, flags: MapFlags, f: Option<F>, offset: off_t) -> Result<*mut c_void>

If you remove ::<File>, it will result in a compilation error:

type annotations needed; cannot infer type of the type parameter F declared on the function mmap

Is there any other way to avoid the compilation error?

I understood it, but it's unclear to the reader of this code. May I ask you to write a comment about it?

utam0k · 2023-09-24T05:38:38Z

crates/libcontainer/src/container/tenant_builder.rs

        let csocketfd = self.setup_tty_socket(&container_dir)?;
+        let csocketfd = csocketfd.map(|sockfd| {
+            let fd = sockfd.as_raw_fd();
+            forget(sockfd);
+            fd
+        });


Suggested change

let csocketfd = self.setup_tty_socket(&container_dir)?;

let csocketfd = csocketfd.map(|sockfd| {

let fd = sockfd.as_raw_fd();

forget(sockfd);

fd

});

let csocketfd = self.setup_tty_socket(&container_dir)?.map(|sockfd| {

let fd = sockfd.as_raw_fd();

forget(sockfd);

fd

});

utam0k · 2023-09-24T05:40:27Z

crates/libcontainer/src/process/seccomp_listener.rs

-        },
-    )?;
-    // The spec requires the listener socket to be closed immediately after sending.
-    let _ = unistd::close(socket);


Why we can remove close()

I think OwnedFd will automatically call libc::close() when it goes out of function body.

The spec requires the listener socket to be closed immediately after sending.

We need to close it as soon as possible.

L106 is the return statement, so sendmsg() will be immediately followed by the drop of the socket.
However, considering that the spec emphasizes this point, I believe it's necessary to explicitly close the socket. I plan to call drop(socket) on L105. Do you think that's okay?

I believe it's necessary to explicitly close the socket. I plan to call drop(socket) on L105. Do you think that's okay?

I prefer it because there is a possibility that we might change the code around there.

utam0k · 2023-09-24T05:44:09Z

tests/rust-integration-tests/integration_test/src/tests/seccomp_notify/seccomp_agent.rs

@@ -47,14 +50,12 @@ pub fn recv_seccomp_listener(seccomp_listener: &Path) -> SeccompAgentResult {
        Ok(msg) => msg,
        Err(e) => {
            let _ = unistd::close(conn);
-            let _ = unistd::close(socket);


We can remove unisted::close because OwnedFd is automatically closed by Drop, can't we?

Is there a way to check if fd is leaking?

yihuaf · 2023-09-24T17:41:52Z

I will take a look this week.

utam0k · 2023-09-25T10:54:37Z

@yihuaf Thanks for taking your time 🙏 I want you to review it, too.

anti-entropy123 · 2023-09-26T03:13:14Z

Hi, I've resubmitted the new code, and I've removed all forget() (except for one in a test function, crates/libcontainer/src/tty.rs:211 ).
In my downstream repository, all CI tests have already passed.
Please review it again when you have time. Thank you very much. @utam0k

utam0k · 2023-09-28T12:24:12Z

@anti-entropy123 May I ask you to resolve the conflict? 🙏

crates/libcontainer/src/channel.rs

anti-entropy123 · 2023-09-29T04:52:57Z

May I ask you to resolve the conflict?

👌 Done. @utam0k

utam0k · 2023-10-01T12:40:29Z

@anti-entropy123 Thanks. Please tell me why we can't use try_clone()?
https://doc.rust-lang.org/nightly/std/os/fd/struct.OwnedFd.html#method.try_clone

anti-entropy123 · 2023-10-01T13:24:32Z

Please tell me why we can't use try_clone()?

Hi, thanks for your advice.

Honestly, I didn't initially think of trying try_clone(). But I think using try_clone() might cause the file descriptor to be accidentally closed.

For example, in the container_main_process() (crates\libcontainer\src\process\container_main_process.rs:49), the channels are cloned into closures on the heap. For the parent process, this closure will be destructed after clone() finishes, causing all the channels within it to be closed.

Is my understanding correct?

utam0k · 2023-10-03T11:42:32Z

@anti-entropy123 I assume that try_clone() creates a new file descriptor with DUPFD. so there is no problem I think...

anti-entropy123 · 2023-10-04T05:07:17Z

I assume that try_clone() creates a new file descriptor with DUPFD. so there is no problem I think...

@utam0k Indeed, you are right. So, do you think we shouldn't use Rc? 🤔

Although you might already be aware, I'd like to mention that try_clone() might also result in the need to use close() to cleanup those old FDs after the child process is created.

utam0k · 2023-10-04T12:18:40Z

@anti-entropy123 I think Rc is one of a complex category. I would avoid using it if possible.

utam0k · 2023-10-04T12:19:12Z

I wish I had noticed that first. I'm sorry it took me so long to notice.

anti-entropy123 · 2023-10-05T01:54:26Z

I would avoid using it if possible.

@utam0k Okey, I will update the relevant code.

YJDoc2 · 2023-10-05T05:48:26Z

Hey, I took a look at the changes and the conversation that is happening, and

I don't think at this point youki can completely switch from RawFd to OwnedFd, because at several places we are opening sockets or connections, and then passing them around, or sharing them across forks, for which (as of now) we need them to be treated as i32
Given that till now we were using RawFds, we have taken care to make sure everything that was opened is closed properly. I think in the upgrade to nix 0.27.1, we should only update types so that they compile and propagate properly, and while doing that we can see where RawFds are being used, and check that they are properly being closed. Then in a separate PR, we can do the change from RawFd to (maybe) OwnedFd , and see how complex that becomes.
I agree that using Rc/Arc would not be a good idea, they have their own handling complexity, and I would prefer not to use ref-counted things in this context as much as possible due to potential overhead they have.
I think that instead of using std::mem::forget , using std::mem::ManuallyDrop would be a better idea, as that fits here better semantically. https://doc.rust-lang.org/std/mem/struct.ManuallyDrop.html
I have opened a PR on my fork Nix upgrade YJDoc2/youki#260 , where I have only done type changes and used ManuallyDrop, so you can see how that looks for reference.

@utam0k 👀

utam0k · 2023-10-05T11:54:15Z

@anti-entropy123 Sorry for confusing you, but I agree with @YJDoc2 . We should take care of OwnedFd in another PR if possible.

Signed-off-by: anti-entropy123 <1348651580@qq.com>

anti-entropy123 · 2023-10-07T03:35:36Z

We should take care of OwnedFd in another PR if possible.

I understand. I referenced @YJDoc2 's fork, and using ManuallyDrop seems to be a better approach.
Please re-review again. @utam0k

utam0k

@anti-entropy123 May I ask you to create the issue to fully support OwnedFd in another PR

utam0k · 2023-10-09T00:46:54Z

crates/libcontainer/src/channel.rs

+    let f1 = std::mem::ManuallyDrop::new(f1);
+    let f2 = std::mem::ManuallyDrop::new(f2);


nits: Could you leave a comment on why we need to ManuallyDrop?

Signed-off-by: anti-entropy123 <1348651580@qq.com>

anti-entropy123 · 2023-10-09T02:31:47Z

May I ask you to create the issue to fully support OwnedFd in another PR

No problem. 👌 @utam0k

YJDoc2

Hey, some minor comments, but nothing blocking. So if everything else is ok, we can go ahead and merge, and address these later

tests/rust-integration-tests/integration_test/src/tests/seccomp_notify/seccomp_agent.rs

Signed-off-by: anti-entropy123 <1348651580@qq.com>

YJDoc2

LGTM 👍
nits by utam0k have been implemented, so going ahead and merging this.

YJDoc2 · 2023-10-09T15:17:29Z

@anti-entropy123 thank you for following on this and implementing 🙏 Apologies for the back-and-forth that was done from our side, and you had to change the PR 😓
Congrats on your first contribution to Youki ! 🎉 🎉

anti-entropy123 · 2023-10-09T15:42:32Z

@YJDoc2 You're welcome, and I greatly appreciate your reviews and suggestions. I've learned a lot from them 👍. I'm looking forward to making more contributions to Youki !

By the way, I have a question. I noticed that there are many individual commits in the main branch after merging this PR. Why not use 'Squash and merge'? Or should I aim to keep the number of commits as small as possible?

YJDoc2 · 2023-10-10T10:36:51Z

I'm looking forward to making more contributions to Youki !

🙌

By the way, I have a question. I noticed that there are many individual commits in the main branch after merging this PR. Why not use 'Squash and merge'? Or should I aim to keep the number of commits as small as possible?

So there is no hard-and-fast rule regarding this. We usually try to keep the main branch clean, and keep the commits in PR, so we can refer back to specific commit in history if needed. However, if there are lots of commits, or many commits which are just fixing lints / minor changes, we ask dev to rebase and squash, or merge the PR in squash-and-merge fashion. For this particular PR, 5 commits are not too much, and even though there are couple of lint/comment fix commits, I felt that would be fine.

Overall, if possible one should aim to keep number of commits small, so the main branch stays clean ; but if it is required to have large number of commits (eg step-wise changes, separating each in a commit would make review easier) then one can request on PR to sqash as well (or we'd do so when we merge).

anti-entropy123 mentioned this pull request Sep 19, 2023

Update nix to 0.27.* #2317

Closed

anti-entropy123 force-pushed the main branch from 6653a7a to 4cd83b1 Compare September 19, 2023 13:47

YJDoc2 added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Sep 20, 2023

utam0k requested review from utam0k, yihuaf and YJDoc2 September 23, 2023 13:05

utam0k requested changes Sep 24, 2023

View reviewed changes

anti-entropy123 closed this Sep 26, 2023

anti-entropy123 force-pushed the main branch from 261e103 to 055f483 Compare September 26, 2023 02:33

anti-entropy123 reopened this Sep 26, 2023

anti-entropy123 force-pushed the main branch from 033a34f to c8ac6a7 Compare September 26, 2023 02:57

utam0k reviewed Sep 28, 2023

View reviewed changes

crates/libcontainer/src/channel.rs Show resolved Hide resolved

anti-entropy123 force-pushed the main branch from c8ac6a7 to d816cd7 Compare September 29, 2023 04:43

bryantbiggs mentioned this pull request Oct 5, 2023

Update nix crate to 0.27 containerd/rust-extensions#211

Merged

update nix to 0.27.1

59937cf

Signed-off-by: anti-entropy123 <1348651580@qq.com>

anti-entropy123 force-pushed the main branch from d816cd7 to 59937cf Compare October 7, 2023 03:13

utam0k approved these changes Oct 9, 2023

View reviewed changes

utam0k reviewed Oct 9, 2023

View reviewed changes

anti-entropy123 added 3 commits October 9, 2023 10:17

add comments

705b17a

Signed-off-by: anti-entropy123 <1348651580@qq.com>

resolve conflicts

d07a538

Signed-off-by: anti-entropy123 <1348651580@qq.com>

fix lint error

326aea1

Signed-off-by: anti-entropy123 <1348651580@qq.com>

YJDoc2 reviewed Oct 9, 2023

View reviewed changes

anti-entropy123 mentioned this pull request Oct 9, 2023

Fully support OwnedFd #2417

Open

remove let socket = socket;

951e5c6

Signed-off-by: anti-entropy123 <1348651580@qq.com>

YJDoc2 approved these changes Oct 9, 2023

View reviewed changes

YJDoc2 merged commit b968ac5 into youki-dev:main Oct 9, 2023
13 checks passed

		let f1 = std::mem::ManuallyDrop::new(f1);
		let f2 = std::mem::ManuallyDrop::new(f2);

update nix to 0.27.1 #2369

update nix to 0.27.1 #2369

Conversation

anti-entropy123 commented Sep 19, 2023 • edited Loading

codecov-commenter commented Sep 20, 2023 • edited Loading

Codecov Report

YJDoc2 commented Sep 20, 2023

utam0k commented Sep 23, 2023

utam0k 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

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

anti-entropy123 Sep 24, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuaf commented Sep 24, 2023

utam0k commented Sep 25, 2023

anti-entropy123 commented Sep 26, 2023

utam0k commented Sep 28, 2023

anti-entropy123 commented Sep 29, 2023

utam0k commented Oct 1, 2023

anti-entropy123 commented Oct 1, 2023 • edited Loading

utam0k commented Oct 3, 2023

anti-entropy123 commented Oct 4, 2023

utam0k commented Oct 4, 2023

utam0k commented Oct 4, 2023

anti-entropy123 commented Oct 5, 2023

YJDoc2 commented Oct 5, 2023

utam0k commented Oct 5, 2023

anti-entropy123 commented Oct 7, 2023

utam0k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anti-entropy123 commented Oct 9, 2023

YJDoc2 left a comment

Choose a reason for hiding this comment

YJDoc2 left a comment

Choose a reason for hiding this comment

YJDoc2 commented Oct 9, 2023

anti-entropy123 commented Oct 9, 2023

YJDoc2 commented Oct 10, 2023

anti-entropy123 commented Sep 19, 2023 •

edited

Loading

codecov-commenter commented Sep 20, 2023 •

edited

Loading

anti-entropy123 Sep 24, 2023 •

edited

Loading

anti-entropy123 commented Oct 1, 2023 •

edited

Loading