Page MenuHomeFreeBSD

Add kernelspace and userspace parts of ktrargs()
Needs ReviewPublic

Authored by artemhevorhian_gmail.com on Tue, Oct 15, 2:25 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Oct 17, 5:37 AM
Unknown Object (File)
Tue, Oct 15, 4:23 PM
Unknown Object (File)
Tue, Oct 15, 4:22 PM
Subscribers

Details

Reviewers
None
Group Reviewers
Contributor Reviews (src)
Summary

The problem. Currently the usage of ktrace is limited in the way that it is not possible to see the environment variables and command line arguments of execve() system call.

Solution. A new ktrace event is proposed for writing the information that is currently hidden behind the pointers of SYSCALL execve() with code 59. For that, kern_execve() is injected with a call to ktrargs(image_args*).

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 60053
Build 56937: arc lint + arc unit

Event Timeline

You should retitle the review and replace "the new function" with what function it is so people know.

artemhevorhian_gmail.com retitled this revision from Add kernelspace and userspace parts of the new function. to Add kernelspace and userspace parts of ktrargs().Tue, Oct 15, 3:51 PM

Remove the useless includes + other minor changes.

markj added inline comments.
sys/kern/kern_ktrace.c
564

IMO this name is too vague. ktrexecveargs() or similar would be better.

568

You're allocating a buffer here and then another for the env, then copying those buffers into a third buffer. But, the first two buffers are leaked, and there's no reason to allocate them that I can see - why not just copy everything into the final destination buffer?

sys/sys/ktrace.h
270

Same comment regarding the name - "ARGS" is too vague.

usr.bin/kdump/kdump.c
264

Please group this with the other ktr*() functions further down below main().

266

Please format this more like other ktrace records. Using english to describe the fields is not very useful, especially without quotes.

At the very least, something like printf("{argv='%s', envp='%s'}\n"). Though, perhaps we want to do something to show whether individual args/envvars contain whitespace?

705–708

Missing an entry here for the new record type.

Because of the need to include the whole imgact.h, remove the forward declaration
of it.

First working version. To be reviewed.

sys/kern/kern_ktrace.c
568

Hello Mark. Please check the new version. Do I still need to run a free() call at the end of the function, right after ktr_submitrequest()?

usr.bin/kdump/kdump.c
266

Is it possible that argvs and/or envv contain whitespace characters?

sys/kern/kern_ktrace.c
569

Please put a newline between variable declarations and the rest of the function. See the style.9 man page for more guidance: https://man.freebsd.org/cgi/man.cgi?style(9)

571

This can simply be buf[argc] = '\0';, no need to use bcopy().

573

Similarly, buf[argc + 1 + envc] = '\0'; is a bit simpler.

576

It's impossible to have buf == NULL here, this check can be removed.

584

Extra newline here.

sys/sys/ktrace.h
43

You don't need these includes here. Just declare struct image_args in the same place where we already declare struct ktr_io_params.

345

There should be a space before *. See the style.9 man page I linked above.

usr.bin/kdump/kdump.c
266

Yes. Try tracing a shell, and run something like

$ awk '{ print }' /dev/null

The argv reported by ktrace will look like awk { print } /dev/null, which is a bit confusing.

usr.bin/ktrace/ktrace.h
35

The ktrace man page needs to be updated as well. We should add a new flag to -t to optionally disable execve tracing, and the documented list of default tracepoints also needs to be updated.