Details

Reviewers

markj
scf
kib

Group Reviewers

manpages
secteam

Commits

rG597b02675751: libc: add clearenv function

Summary

The clearenv(3) function allows us to clear all environment variable in one
shot. This may be useful for security programs that want to control
the environment or what variables are passed to new spawned programs.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 36396
Build 33285: arc lint + arc unit

Event Timeline

oshogbo created this revision.Jan 18 2021, 5:28 PM

Herald added a reviewer: manpages. · View Herald TranscriptJan 18 2021, 5:28 PM

Herald added a subscriber: imp. · View Herald Transcript

oshogbo requested review of this revision.Jan 18 2021, 5:28 PM

Harbormaster completed remote builds in B36318: Diff 82485.Jan 18 2021, 5:28 PM

oshogbo added reviewers: markj, scf, kib.Jan 18 2021, 5:31 PM

Look at src/MAINTAINERS file for the *env(3) line.

lib/libc/stdlib/Symbol.map
130	This should be alphabetically sorted.
lib/libc/stdlib/getenv.3
228	I doubt it ('first appeared in glibc'). Linux man page states CONFORMING TO Various UNIX variants (DG/UX, HP-UX, QNX, ...). POSIX.9 (bindings for FORTRAN77).

Thank you @kib for hints!

oshogbo added a reviewer: secteam.Jan 18 2021, 6:19 PM

Harbormaster completed remote builds in B36321: Diff 82491.Jan 18 2021, 6:19 PM

Looks pretty good. I have some ideas/suggestions.

BTW, good job at stomaching that code! ;)

lib/libc/stdlib/getenv.c
696	Suggestion: `Unset all variables by flagging them ...`
704–715	It has been many years since I have looked at this code, so bear with me. :) There may be two other options, which you may have already looked at: Calling `environ = NULL` and calling `__merge_environ()` may accomplish the same. Calling `__clean_env(false)` and setting `environ = NULL`. Those may work, but you would have to test to make sure.
715	The final call of `__rebuild_environ(0)` at the end of the function will have `environ` set to `intEnviron` which is not `NULL` at that point. The Linux man page says `environ` will be set to `NULL`. I also recommend a unit test for that.

I am not sure but shouldn't we be adding "SPDX-License-Identifier: BSD-2-Clause-FreeBSD" when committing new files?

lib/libc/stdlib/getenv.3
39	Shouldn't the Makefile be edited as well to install a symlink for clearenv.3?

Update diff.

Harbormaster completed remote builds in B36396: Diff 82694.Jan 21 2021, 5:33 PM

@0mp Thanks I addressed that.

lib/libc/stdlib/getenv.c
715	Yea that looks much easier. I did like you suggested and added some more tests.

Ping?

Sorry for the long turnaround time on the review. Thank you for adding more tests.

include/stdlib.h
275	I think this declaration should be placed just above the `daemon()` declaration later in the file to preserve the sorted order. Just a blank line above it and no blank line between it and `daemon()`. It would be nice for someone more knowledgeable to agree or disagree with this. It just looks like it matches the flow better.
lib/libc/stdlib/getenv.3
70	I believe you meant `putenv` instead of `getenv`.
lib/libc/tests/stdlib/clearenv_test.c
131	I am just verifying. Is this last line testing recreation after clearing twice, or is it due to copying another test such as `clearenv__recreated_vars_test()`?
144	This is missing a blank line.
153	I suggest a test or more around `putenv()` too. At least, tests that verify that variables put into `environ` are cleared and the original string passed via `putenv()` is untouched.