Add memset_s(3) doc to memset(3) man page.
Needs ReviewPublic

Authored by cy on Fri, Dec 29, 8:45 PM.

Details

Reviewers
imp
jhb
glebius
kib
bjk
cem
Group Reviewers
manpages
Summary

This diff adds memset_s(3) doc, which was discovered missing while working on another revision. It's been in two of my trees for a while and should be committed before inadvertently committed with something else.

Test Plan

This diff has been in a couple of my trees, including my "prod" tree, for a while.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 14430
cy created this revision.Fri, Dec 29, 8:45 PM
cem added inline comments.Fri, Dec 29, 8:53 PM
lib/libc/string/memset.3
71–74

This case is a little weird and probably should be fully documented.

If the overflow would occur, the memset() is still applied to the region of length destsz. Afterwards, the constraint handler / error is thrown.

set_constraint_handler_s is probably worth documenting and Xring here.

kib added a comment.Fri, Dec 29, 9:16 PM

memset_s() is part of the C11 Annex K, which should be referenced there if man page describes the function.

lib/libc/string/memset.3
70

.Dv RSIZE_MAX

cy added inline comments.Fri, Dec 29, 9:18 PM
lib/libc/string/memset.3
71–74

Agreed that buffer overrun should probably be documented in memset.3. Trying to keep the description as brief as possible, does this look good?

Access beyond the end of the b array will result in corruption of neigbouring memory (stack or heap).

Unfortunately set_constraint_handler_s is not documented in our tree -- and yes it should be documented. Should this revision and the revision implementing gets_s (D12785) block pending documentation of set_constraint_handler_s?

cy marked an inline comment as done.Fri, Dec 29, 9:21 PM
cem added inline comments.Fri, Dec 29, 9:28 PM
lib/libc/string/memset.3
71–74

Access beyond the end of the b array will result in corruption of neigbouring memory (stack or heap).

No, that's inaccurate.

Something like: If .Fn memset_s is called with len greater than destsz, the memset is first applied as if the len parameter was destsz. Then the error condition is thrown and the constraint handler invoked.

Unfortunately set_constraint_handler_s is not documented in our tree -- and yes it should be documented.

Right.

Should this revision and the revision implementing gets_s (D12785) block pending documentation of set_constraint_handler_s?

No, I don't think there's any particular reason to force an ordering. I'd just like it to be documented eventually.

jilles added a subscriber: jilles.Fri, Dec 29, 9:28 PM
jilles added inline comments.
lib/libc/string/memset.3
60–63

memset_s() also differs from memset() in that it is not removed as a dead store, making it useful for clearing sensitive data such as a password, like explicit_bzero().

cem added inline comments.Fri, Dec 29, 9:31 PM
lib/libc/string/memset.3
71–74

FYI http://en.cppreference.com/w/c/string/byte/memset describes it well, I think:

[Memset_s is the] Same as [memset], except that the following errors are detected at runtime and call the currently installed constraint handler function after storing ch in every location of the destination range [dest, dest+destsz) if dest and destsz are themselves valid: ...

cy added inline comments.Fri, Dec 29, 9:54 PM
lib/libc/string/memset.3
71–74

Agreed. It should be documented. I'll put it on my list of things to do.

71–74

I think I'll revise memset and add memset_s doc.

cy updated this revision to Diff 37216.Sat, Dec 30, 4:28 AM

All requested changes have been incorporated.

Unfortunately I might not be able to separate out memset_s changes from general memset doc changes prior to commit.

Let me know what you think.

kib added inline comments.Sat, Dec 30, 9:15 AM
lib/libc/string/memset.3
97

New line for new sentence.

99

I would say that this claim is too wide. If you say ' ... clear memory not referenced after the memset call, e.g. immediately freed' it would be less eye-scratching.

cy marked an inline comment as done.Sat, Dec 30, 7:49 PM
cy added inline comments.
lib/libc/string/memset.3
99

I can soften it a bit. How does this sound?

For this reason it is not advised to use
.Fn memset
to clear subsequently unreferenced memory, such as immediately freed memory (i.e used for passwords).

kib added inline comments.Sat, Dec 30, 8:43 PM
lib/libc/string/memset.3
99

I think it is better to give advise what to do instead of what to avoid.

For this reason it is advised to use memset_s(), instead of memset(), to clear subsequently unreferenced memory. For instance, a buffer containing the password should be cleared with memset_s() before free().

cy marked 3 inline comments as done.Sat, Dec 30, 10:07 PM
cy added inline comments.
lib/libc/string/memset.3
99

Agreed. Thank you.

cy updated this revision to Diff 37255.Sat, Dec 30, 10:09 PM
cy marked an inline comment as done.

This should be the last update. Can you check it over please?

kib added inline comments.Sun, Dec 31, 11:48 AM
lib/libc/string/memset.3
61

resulting *from* storage overlay

67

is invalid, in particular
.Dv NULL .

88

What is this ?

90

.Xr 3 explicit_bzero ,

106

perhaps
.Xr 3 free .

108

This and the next sentences are out of place, incomplete, and/or duplicates the previous text.

bjk added inline comments.Sun, Dec 31, 9:16 PM
lib/libc/string/memset.3
71–74

Is "thrown" really a conventional word to use in this context? To me it has more connotations of (e.g., C++) exceptions than a C library's internals.

cem added inline comments.Sun, Dec 31, 10:22 PM
lib/libc/string/memset.3
71–74

*Shrug*. constraint_handlers are really weird C APIs that no one uses, AFAIK.

C11:

Implementations shall verify that the runtime-constraints for a function are not violated by the program. If a runtime-constraint is violated, the implementation shall call the currently registered runtime-constraint handler (see set_constraint_handler_s in <stdlib.h>).

...

The runtime-constraint handler might not return.

...

The abort_handler_s function writes a message on the standard error stream in an implementation-defined format. The message shall include the string pointed to by msg. The abort_handler_s function then calls the abort function. 393)

rpokala added inline comments.
lib/libc/string/memset.3
71–74

"except that an error is returned, and the constraint handler is called, if"

cy marked 4 inline comments as done.Wed, Jan 17, 4:44 AM
cy updated this revision to Diff 38075.Wed, Jan 17, 4:49 AM

Can you please review this updated diff? (Hopefully I haven't lost track of all the changes.)

bjk requested changes to this revision.Wed, Jan 17, 5:05 AM
bjk added inline comments.
lib/libc/string/memset.3
61

or even "resulting from storage overflow"?

71

maybe s/performs/behaves/?

76

What is the .Sp macro? I don't think you need a macro to put a space after the comma; that should be the default behavior (multiple instances).

89

The order of arguments to .Xr is the other way -- .Xr explicit_bzero 3

91

I might put a comma after "(DSE)".

96

"On the other hand" might be too informal; an alternative would be "In contrast, .Fn memset may be optimized away if [...]"

101

"subsequently unreferenced memory" is an unusual phrasing; I might say "to clear memory that will not subsequently be accessed".

102

*a* password.

109

This ("Undefined behavior...value of destsz") is not even a complete sentence. I agree with kib; just remove it and the next sentence (which is at best confusing).

This revision now requires changes to proceed.Wed, Jan 17, 5:05 AM
cy marked 9 inline comments as done.Wed, Jan 17, 6:17 AM
cy added inline comments.
lib/libc/string/memset.3
61

yes, from is better.

overlay == overflow ... my exposure to IBM is beginning to show.

cy updated this revision to Diff 38076.Wed, Jan 17, 6:17 AM
cy marked an inline comment as done.
kib added inline comments.Wed, Jan 17, 9:39 AM
lib/libc/string/memset.3
61

overlay has very different meaning for me, referencing long time obsolete run-time linking technology.

67

This is not handled. Claiming that only NULL is undef is not correct.