cmd/snap-update-ns: add C preamble for setns

[zyga]

This patch adds a preamble/bootstrap code to snap-update-ns so that it
can call setns(..., CLONE_NEWNS) before golang starts to create threads.

The setns(2) manual page states that CLONE_NEWNS fails if there are any
threads present, apart from the main thread.

The preamble has no access to command line arguments so it works around
this by probing /proc/self/cmdline and parsing this manually.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

[niemeyer]

More readable to have the read outside the if. Then, ideally this would also check:

if (num_read < 0 || num_read == sizeof buf_size)

[zyga]

Tweaked

[niemeyer]

If \0 is the last byte, then argv_len < num_read, yet argv0_len+1 is an overrun.

[zyga]

Corrected

[niemeyer]

find_snap_name returns NULL.

Slightly worried about the rate of important errors in this logic. Please make sure to get a good second review.

[zyga]

Ack, thank you for the first review, will do!

[stolowski]

A caveat - I'm not familiar with namespaces nor with the "big picture" for the problem this branch solves, so cannot comment on the approach and overall semantics. The code looks good with some general comments:

• the read_cmdlne, find_snap_name, sanitize_snap_name should have tests (it may be neccessary to make /proc/../cmdline path mock-able/configurable for testing purposes).
• [paranoid mode] if we want to be really paranoid, then we should be checking all system calls for errors, including close()
• again, if we want to be really paranoid, we could cassert (just to be safe) on function arguments where it's critical for correct operation, such as making sure functions don't receive NULL for a buffer it operates on).
  [/paranoid mode]

[morphis]

Not really needed.

[zyga]

Why not? This is standard practice to ensure that headers match implementation.

[morphis]

Ah yeah, we're in C and not in C++, so makes more sense here to spot API/implementation differences.

[morphis]

Please add proper header guards

#ifndef SNAP_UPDATE_NS_BOOTSTRAP_H_
#define SNAP_UPDATE_NS_BOOTSTRAP_H_
[...]
#endif

[zyga]

Ack, will do

[zyga]

Done

[morphis]

This one is only read from other code parts so a _get function is more what we want here.

[zyga]

I decided not to, it's just more code that needs to be written where we want less code to exist.

[morphis]

Same as for bootstrap_errno

[zyga]

Not sure if this buys us anything. Would make the C code longer.

[morphis]

Would just make the API contract clear. We don't support write, just read.

[jdstrand]

I'm struggling to see how update-ns being in Go is really saving us on complexity and maintenance when I see comments like this:

// Use a pre-main helper to switch the mount namespace. This is required as
// golang creates threads at will and setns(..., CLONE_NEWNS) fails if any
// threads apart from the main thread exist.

which is making us have to write C code to do the OS-level operations that are required.

[jdstrand]

In general, cmdline is something that is user-controlled. I understand that snapd is the caller of update-ns, but how are the arguments to update-ns created? Is it possible for a user to invoke a snap run command and manipulate cmdline? If so, this preamble code needs to be carefully written to handled untrusted input via cmdline.

[zyga]

@jdstrand to reply to your concerns:

The amount of system C code we need to write is minimal, it's really just one system call. The only complication is that we have no access to the command line in a convenient way and must execute this function before golang bootstrap begins, at which we no longer have one thread.

This program is not setuid root and does not process user input in C. The golang parts, which are significantly harder to attack, process content generated by snapd (the mount profile) based on what two participating snaps declare. Having said that at this level there is no user input parsed.

[mvo5]

Some questions and suggestions. Sorry that its slightly long :/

[mvo5]

I wonder if it makes sense to have two different error messages here. It seems to me that num_read < 0 is a read releated error so that handling looks appropriate. However num_read == buf_size is a different kind of error, it means something like "internal error: buffer to read cmdline is too small" (right?). So maybe worth reporting separately.

[mvo5]

Actually - I'm slightly confused. In the case of num_read == buf_size the bootstrap_{errno,msg} are set. However in the later code its only an error if num_read is < 0 - is there code missing here that sets num_read = -1 if num_read == buf_size ? Or am I just misreading this code altogether :) ?

[zyga]

Thanks, I'll tweak this accordingly. I think you are right.

[mvo5]

Close may also return an error

[zyga]

In practice it is an useless error that should not be checked for or handled. See: https://lwn.net/Articles/576478/

[mvo5]

Silly(?) question - but why do we need both buf_size and num_read here? Isn't num_read always smaller than buf_size and sufficient for the size checks in here?

[zyga]

Just for simplicity and correctness, buf_size is a constant.

[mvo5]

Honest question, why is it simpler (and more correct) to use buf_size for the first check and num_read for the second? I mean, it seems checking for just num_read in the strnlen() is equally correct (and arguably simpler because when reading the code there is no need to ponder why in one place buf_size is used and in the other place num_read. Don't get me wrong, its fine, I'm just trying to understand the background a bit better.

[zyga]

I think it is simpler because one argument is not a variable but a compile-time constant. I can fuse this into one variable if you want but I like the explicitness of "this is the size of the buffer" in C APIs.

[niemeyer]

Having more logic to think about makes things more complex rather than simpler. If we should never go above the smaller number – and we shouldn't – there's no reason to have another number and make things any more complex.

Trivial and potentially serious (tomorrow) bug to introduce: invert the two parameters.

[mvo5]

This could be written as a single line: if _, err := parser.ParseArgs(args); err != nil {...

[zyga]

Done

[mvo5]

Maybe a small comment here that that the bootstrap C code is automatically called and that we need to check errors for that here?

[zyga]

Will do, thanks

[zyga]

Done

[zyga]

I updated this, tweaking slightly how initialization is done so that we can test more code. I added unit tests for what is reasonable to test this way (skipping the actual setns as that will be covered by spread). I can add a smoke spread test that just checks we setns correctly and reach the "not implemented" trigger (I will do that in a moment) but I'd like to land this and iterate.

[mvo5]

I think this also needs to set num_read = -1 or else read_cmdline() will not fail in https://github.com/snapcore/snapd/pull/3095/files#diff-018c9af8a9874391aef7a2bfa0300d19R142 but carry on with an incorrect buffer.

[mvo5]

Silly(?) question - but why do we need both buf_size and num_read here? Isn't num_read always smaller than buf_size and sufficient for the size checks in here?

[zyga]

Just for simplicity and correctness, buf_size is a constant.

[mvo5]

Honest question, why is it simpler (and more correct) to use buf_size for the first check and num_read for the second? I mean, it seems checking for just num_read in the strnlen() is equally correct (and arguably simpler because when reading the code there is no need to ponder why in one place buf_size is used and in the other place num_read. Don't get me wrong, its fine, I'm just trying to understand the background a bit better.

[zyga]

I think it is simpler because one argument is not a variable but a compile-time constant. I can fuse this into one variable if you want but I like the explicitness of "this is the size of the buffer" in C APIs.

[niemeyer]

Having more logic to think about makes things more complex rather than simpler. If we should never go above the smaller number – and we shouldn't – there's no reason to have another number and make things any more complex.

Trivial and potentially serious (tomorrow) bug to introduce: invert the two parameters.

[mvo5]

Typo: This lets *us* use...

[stolowski]

Looks good, thanks for the tests! Just one nitpick and one question, but not blockers.

[stolowski]

Nitpick: wouldn't "validate" be a better name since this method doesn't actually alter the name in any way?

[zyga]

Yes, good point. How about partially_validate_snap_name (to make it clear it's not a full validation)?

[stolowski]

Is x-... some form of new convention we want to introduce with this PR? Do we use it elsewhere in snapd? Looks a little verbose.

[zyga]

Ha, I was just trying to be generic, theSNAPD_INTERNAL variable could hold a list of comma-separated key-value pairs. I'm fine with reworking this in any way that we find suitable.

[stolowski]

I see. Ok, no arguing with that, just wanted to understand the reason ;)

[niemeyer]

Something more straightforward would be nice indeed. How about

SNAPD_DEBUG_UPDATE_NS="skip-bootstrap"

[pedronis]

why not just have a bit of C code that sets a global no_bootstrap flag, that is compiled in only when building tests?

[zyga]

Because cgo cannot be used in tests.

[pedronis]

you can use a file with a build tag though, no?

[niemeyer]

It looks good. A few more comments, and probably the last ones.

[mvo5]

Silly(?) question - but why do we need both buf_size and num_read here? Isn't num_read always smaller than buf_size and sufficient for the size checks in here?

[zyga]

Just for simplicity and correctness, buf_size is a constant.

[mvo5]

Honest question, why is it simpler (and more correct) to use buf_size for the first check and num_read for the second? I mean, it seems checking for just num_read in the strnlen() is equally correct (and arguably simpler because when reading the code there is no need to ponder why in one place buf_size is used and in the other place num_read. Don't get me wrong, its fine, I'm just trying to understand the background a bit better.

[zyga]

I think it is simpler because one argument is not a variable but a compile-time constant. I can fuse this into one variable if you want but I like the explicitness of "this is the size of the buffer" in C APIs.

[niemeyer]

Having more logic to think about makes things more complex rather than simpler. If we should never go above the smaller number – and we shouldn't – there's no reason to have another number and make things any more complex.

Trivial and potentially serious (tomorrow) bug to introduce: invert the two parameters.

[stolowski]

Is x-... some form of new convention we want to introduce with this PR? Do we use it elsewhere in snapd? Looks a little verbose.

[zyga]

Ha, I was just trying to be generic, theSNAPD_INTERNAL variable could hold a list of comma-separated key-value pairs. I'm fine with reworking this in any way that we find suitable.

[stolowski]

I see. Ok, no arguing with that, just wanted to understand the reason ;)

[niemeyer]

Something more straightforward would be nice indeed. How about

SNAPD_DEBUG_UPDATE_NS="skip-bootstrap"

[pedronis]

why not just have a bit of C code that sets a global no_bootstrap flag, that is compiled in only when building tests?

[zyga]

Because cgo cannot be used in tests.

[pedronis]

you can use a file with a build tag though, no?

[niemeyer]

The logic here looks reversed. Bootstraping is the normal case, so the variable should be used to disable it instead, right?

[zyga]

The problem is that we cannot say "don't bootstrap" from go. By the time go runs it's too late. I think it has to stay reversed.

[niemeyer]

We don't want update-ns to require setting a debug flag to work at all, in the normal case.

[niemeyer]

The second case would be more safely handled inside find_snap_name. If it got an empty string, it didn't actually find a snap name.

[zyga]

+1

[niemeyer]

Nice.

[niemeyer]

That doesn't look safe. That call is returning with ptr pointing to memory managed by Go's GC. If the memory moves, the function below will be accessing unknown memory. Instead, buf should be copied out into a malloc controlled buffer, defer free, and then passed into find_snap_name.

[zyga]

+1

[zyga]

On 2nd look I think this is correct:

The major change is the definition of rules for sharing Go pointers with C code, to ensure that such C code can coexist with Go's garbage collector. Briefly, Go and C may share memory allocated by Go when a pointer to that memory is passed to C as part of a cgo call, provided that the memory itself contains no pointers to Go-allocated memory, and provided that C does not retain the pointer after the call returns. These rules are checked by the runtime during program execution: if the runtime detects a violation, it prints a diagnosis and crashes the program. The checks can be disabled by setting the environment variable GODEBUG=cgocheck=0, but note that the vast majority of code identified by the checks is subtly incompatible with garbage collection in one way or another. Disabling the checks will typically only lead to more mysterious failure modes. Fixing the code in question should be strongly preferred over turning off the checks. See the cgo documentation for more details.

This is from cgo 1.6 release notes

[niemeyer]

What I said above still seems true.

[niemeyer]

Sorry, I think you are right. I was looking at the result of the function as if it was data that wouldn't be moved, but it's already a pointer, so if there's a move the GC will take it along as well.

[niemeyer]

*string is pretty unusual in Go. If we're wrapping, we can use Go's convention and return (string, error).

[zyga]

(this is just for unit testing btw) I did it to be able to differentiate nil and "" as returned from find_snap_name. I wanted to avoid extra wrappers as the real intent is to test the C implementation, not provide a Go-like API for something that is not used in Go otherwise.

[niemeyer]

You have extra wrappers already. It is Go code that is deciding when to return nil or &str.

[zyga]

Thank you for the review. I'll address the trivial bits ASAP but I think the fundamental question about how when to skip the bootstrap code needs to stay as-is. The problem is, as I mentioned above, that we cannot do anything from go to say "don't bootstrap" because the whole bootstrap code runs before any go starts. We need to not bootstrap by default and let snapd set something that would make the bootstrap happen. I made sure we don't miss this by always failing if the bootstrap code is skipped so that in production if we skip bootstrap (for whatever reason) we will fail without doing anything else.

[niemeyer]

Think outside the box. What hints you can use to decide not to bootstrap?

[zyga]

@niemeyer I've updated the bootstrap logic to look at argv0 and skip bootstrap automatically if it ends with ".test". There's no more SNAPD_INTERNAL hack needed.

[niemeyer]

LGTM, thanks for the changes. One trivial for a follow up:

[niemeyer]

It can't possibly be NULL here.

[zyga]

aha, good catch :-) I'll remove that and merge! :)