Add helper functions to copy strings into struct image_args.

[brooks]

Summary

Given a zeroed struct image_args with an allocated buf member,
exec_args_add_fname() must be called to install a file name (or NULL).
Then zero or more calls to exec_args_add_env() followed by zero or
more calls to exec_args_add_env(). exec_args_adjust_args() may be
called after args and/or env to allow an interpreter to be prepended.

To allow code reuse when adding arg and env variables, begin_envv
is renamed to _begin_envv and an accessor is provided to handle the
case when no environment entries have been added.

Use these functions to simplify exec_copyin_args() and
freebsd32_exec_copyin_args().

Test Plan

Tested on amd64 with 32-bit FreeBSD and 64-bit linux binaries.

[brooks]

I've tested this on CheriBSD (mips64), amd64, and with i386 binaries on amd64 so it should be ready to go.

[kib]

sys/kern/kern_exec.c

1496 ↗

(On Diff #42676)

If you move the final assignment to begin_envv after the last add_arg_str() call, the functions become almost identical, am I right ? The common code can be moved in the helper.

The begin_envvp member is managed outside the functions already, so I do not see a problem with an encapsulation.

[brooks]

• Allow begin_envv to be NULL and support prepending argv entries.

[brooks]

• Remove a nonsensical assert.

[jhb]

sys/compat/freebsd32/freebsd32_misc.c

372 ↗

(On Diff #43182)

The (void *) cast seems redundant? argp is a char * and the second argument to exec_args_add_arg() is a char *.

390 ↗

(On Diff #43182)

Same with this (void *) cast.

sys/kern/imgact_shell.c

205 ↗

(On Diff #43182)

imgact_binmisc.c removed the bcopy() and all the math here, but imgact_shell.c is keeping it?

sys/kern/kern_exec.c

1204 ↗

(On Diff #43182)

Should the (uintptr_t) cast be kept since argp is u_long? (The freebsd32 approach was a bit cleaner and would have a single 'u_long arg' that fueword() read that was then converted to a 'char *' argp or envp before this function removing the need for the cast in the function call, but possibly requiring a (uintptr_t) cast of the u_long before it was assigned to the (char *).)

1438 ↗

(On Diff #43182)

s/possiably/possibly/

[brooks]

• Remove/cleanup casts.
• Spelling.
• Remove code subsumed by exec_args_adjust_args. (needs further testing)

[brooks]

I think I've addressed @jhb's feedback now.

sys/kern/imgact_shell.c

205 ↗

(On Diff #43182)

Thanks for catching this one. The broken code booted to multi-user, but was subtly broken (dhclient failed to configure the interface). I guess that goes to who that booting is less of a test then one might thing.

[brooks]

• Rebase

[kib]

sys/kern/kern_exec.c

1464 ↗

(On Diff #50434)

() are excessive.

1476 ↗

(On Diff #50434)

Assert this ?

1204 ↗

(On Diff #43182)

This seems to be not handled (or answered). Even with argp becoming char *, why do we need to cast between pointers using intermediate integer type ?

sys/sys/imgact.h

49 ↗

(On Diff #50434)

This is actually the file-scope reserved identifier, according to C11 7.1.3 Reserved identifiers.

[brooks]

• Remove unneeded temporaries to follow the pattern in freebsd32.
• Const poision arguments.
• Remove extra parens.
• Document the argument array size with an assert rather than a comment.

[brooks]

sys/kern/kern_exec.c

1204 ↗

(On Diff #43182)

C requires the cast through uintptr_t as non-intptr_t types may not be cast to pointers. To the extent that it's possible to read the standard another way, WG14 is working to correct that in future standards.

sys/sys/imgact.h

49 ↗

(On Diff #50434)

This a reasonably common idiom in our kernel for "internal use only." Does a struct member even count as an identifier?

[kib]

sys/kern/kern_exec.c

1204 ↗

(On Diff #43182)

6.3.2.3 Pointers states directly 'An integer may be converted to any pointer type.' I somehow doubt that this can be changed without breaking too much of compatibility.

sys/sys/imgact.h

49 ↗

(On Diff #50434)

6.2.1 Scopes of identifiers
1 An identifier can denote an object; a function; a tag or a member of a structure, union, or enumeration; a typedef name; a label name; a macro name; or a macro parameter.
So from my reading of the standard, member names are definitely identifiers.

We are sloppy with the proper namespacing, but I do not see a reason to add more of that.

[brooks]

sys/sys/imgact.h

49 ↗

(On Diff #50434)

Do you an alternative suggestion for marking members "internal use only?"

[kib]

sys/sys/imgact.h

49 ↗

(On Diff #50434)

Just state so in the comment perhaps ? After all, this is kernel and the set of consumers of the structure is controllable. We do not need to make it controlled access like c++.

[brooks]

• Use a comment rather than reserved identifier.

[brooks]

sys/kern/kern_exec.c

1204 ↗

(On Diff #43182)

I'd prefer to keep the (uintptr_t) cast here as it was in the original. It doesn't matter in practice since C isn't used to manipulate the pointer in question, but it's the right thing and consistent with the style in this file

Re 'An integer may be converted to any pointer type.' future C standards will likely include a notion of pointer provenance where a pointer must be relative to some object and without out the cast through (uintptr_t) the compiler will be free to assume that random integers cast to pointers have no object backing and to treat access as UB. There will be ways to switch this off (particularly since Linux relies heavily on long->void * casts, but we should avoid adding dependencies on this).

[kib]

sys/kern/kern_exec.c

1204 ↗

(On Diff #43182)

How can generic-purpose allocator on normal machine (AKA flat byte addressable address space) written in C without machine-depended but well-defined isomorphism between void * and some integer type ? Image execution is one prominent example of integer arithmetic applied to the user pointers. Another example is vm/vm_mmap.c, where void * and vm_offset_t are interchangeable or the VM cannot work.

It seems that modern C compiler authors has some deep psychological issues with allowing other people to use computers. Is this because they really want to write an Idris compiler, but the problem is that nobody use that ?

I will not block this change due to extra cast, but also I would not mind removing it if I have to change the affected line, in the future.