Add memset_s(3) doc to memset(3) man page.
ClosedPublic

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

Details

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
cy created this revision.Dec 29 2017, 8:45 PM
cem added inline comments.Dec 29 2017, 8:53 PM
lib/libc/string/memset.3
71–74 ↗(On Diff #37195)

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.Dec 29 2017, 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 ↗(On Diff #37195)

.Dv RSIZE_MAX

cy added inline comments.Dec 29 2017, 9:18 PM
lib/libc/string/memset.3
71–74 ↗(On Diff #37195)

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.Dec 29 2017, 9:21 PM
cem added inline comments.Dec 29 2017, 9:28 PM
lib/libc/string/memset.3
71–74 ↗(On Diff #37195)

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.Dec 29 2017, 9:28 PM
jilles added inline comments.
lib/libc/string/memset.3
60–63 ↗(On Diff #37195)

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.Dec 29 2017, 9:31 PM
lib/libc/string/memset.3
71–74 ↗(On Diff #37195)

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.Dec 29 2017, 9:54 PM
lib/libc/string/memset.3
71–74 ↗(On Diff #37195)

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

71–74 ↗(On Diff #37195)

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

cy updated this revision to Diff 37216.Dec 30 2017, 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.Dec 30 2017, 9:15 AM
lib/libc/string/memset.3
97 ↗(On Diff #37216)

New line for new sentence.

99 ↗(On Diff #37216)

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.Dec 30 2017, 7:49 PM
cy added inline comments.
lib/libc/string/memset.3
99 ↗(On Diff #37216)

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.Dec 30 2017, 8:43 PM
lib/libc/string/memset.3
99 ↗(On Diff #37216)

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.Dec 30 2017, 10:07 PM
cy added inline comments.
lib/libc/string/memset.3
99 ↗(On Diff #37216)

Agreed. Thank you.

cy updated this revision to Diff 37255.Dec 30 2017, 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.Dec 31 2017, 11:48 AM
lib/libc/string/memset.3
61 ↗(On Diff #37255)

resulting *from* storage overlay

67 ↗(On Diff #37255)

is invalid, in particular
.Dv NULL .

88 ↗(On Diff #37255)

What is this ?

90 ↗(On Diff #37255)

.Xr 3 explicit_bzero ,

106 ↗(On Diff #37255)

perhaps
.Xr 3 free .

108 ↗(On Diff #37255)

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

bjk added inline comments.Dec 31 2017, 9:16 PM
lib/libc/string/memset.3
71–74 ↗(On Diff #37195)

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.Dec 31 2017, 10:22 PM
lib/libc/string/memset.3
71–74 ↗(On Diff #37195)

*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 ↗(On Diff #37195)

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

cy marked 4 inline comments as done.Jan 17 2018, 4:44 AM
cy updated this revision to Diff 38075.Jan 17 2018, 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.Jan 17 2018, 5:05 AM
bjk added inline comments.
lib/libc/string/memset.3
61 ↗(On Diff #38075)

or even "resulting from storage overflow"?

71 ↗(On Diff #38075)

maybe s/performs/behaves/?

76 ↗(On Diff #38075)

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 ↗(On Diff #38075)

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

91 ↗(On Diff #38075)

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

96 ↗(On Diff #38075)

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

101 ↗(On Diff #38075)

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

102 ↗(On Diff #38075)

*a* password.

109 ↗(On Diff #38075)

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.Jan 17 2018, 5:05 AM
cy marked 9 inline comments as done.Jan 17 2018, 6:17 AM
cy added inline comments.
lib/libc/string/memset.3
61 ↗(On Diff #38075)

yes, from is better.

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

cy updated this revision to Diff 38076.Jan 17 2018, 6:17 AM
cy marked an inline comment as done.
kib added inline comments.Jan 17 2018, 9:39 AM
lib/libc/string/memset.3
67 ↗(On Diff #37255)

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

61 ↗(On Diff #38075)

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

cy marked 4 inline comments as done.Jan 24 2018, 6:30 AM

I think we might be losing something in describing both functions in the same man page. Formatting it, it should read like this:

cy added a comment.Jan 24 2018, 6:31 AM

As the man page is discussing two functions this may be confusing simply reading the markup. Here it is formatted:

DESCRIPTION

The memset() function writes len bytes of value c (converted to an
unsigned char) to the string dest.  Undefined behaviour from memset(),
resulting from storage overflow, will occur if len is greater than the
the length of buffer dest.  The behaviour is also undefined if the
pointer dest is NULL.

The memset_s() function behaves the same as memset() except that an error
is thrown and constraint handler is called if dest is a null pointer,
destsz or len is greater than RSIZE_MAX, or len is greater than destsz
(buffer overflow would occur).  Like explicit_bzero(3), memset_s() is not
removed through Dead Store Elimination (DSE), making it useful for clear‐
ing sensitve data.  In contrast memset() function may be optimized away
if the object modified by the function is not accessed again.  To clear
memory that will not subsequently be accessed it is advised to use
memset_s() instead of memset().  For instance, a buffer containing a
password should be cleared with memset_s() before free(3).

dest < len <= destsz can cause a buffer overrun.

RETURN VALUES

The memset() function returns its first argument.  The memset_s() func‐
tion returns zero on success, non-zero on error.
cy updated this revision to Diff 38374.Jan 24 2018, 6:32 AM

Minor edits.

Uploaded formatted output as it's easier to read than the markup.

kib added inline comments.Jan 24 2018, 11:03 AM
lib/libc/string/memset.3
67 ↗(On Diff #37255)

I still do not like that the text mention NULL pointer as undefined behavior, but does not say that invalid pointer is an unhandled undefined behavior as well, even for memset_s().

113 ↗(On Diff #38374)

I do not understand this sentence at all. Why do you compare pointers with length, what should it mean etc.

cy planned changes to this revision.Jan 28 2018, 5:06 AM
cy marked an inline comment as done.
cy added inline comments.
lib/libc/string/memset.3
67 ↗(On Diff #37255)

I will reword this for memset().

memset_s() handles a NULL dest (memset_s.c line 48) but it does not handle dest being an invalid pointer. Also documented at http://en.cppreference.com/w/c/string/byte/memset.

I agree that it should be documented that memset_s() will otherwise not handle an invalid pointer.

113 ↗(On Diff #38374)

This would be better said:

A short
.Fa destsz
may result in a buffer overflow.

However I say this in the previous paragraph. I'll simply remove it.

cy updated this revision to Diff 38562.Jan 28 2018, 5:08 AM
cy marked an inline comment as done.

Updated diff.

kib accepted this revision.Jan 28 2018, 9:55 AM
cy updated this revision to Diff 38985.Feb 7 2018, 2:47 AM
cy marked 3 inline comments as done.

Reworded the part discussion when the constraint handler is called.

kib added inline comments.Feb 7 2018, 10:25 AM
lib/libc/string/memset.3
76 ↗(On Diff #38985)

Error handler is called first, and if it returns, the error is returned to the caller. The intent is that the handler does not return but performs the urgent process termination.

cy marked an inline comment as done.Feb 7 2018, 2:13 PM
cy added inline comments.
lib/libc/string/memset.3
71–74 ↗(On Diff #37195)

Can you review my new rewording?

76 ↗(On Diff #38985)

Rather than make that sentence even longer (and more confusing) I've added this sentence:

The runtime-constraint handler is called first and may not return.
If it does return, an error is returned to the caller.

cy updated this revision to Diff 38997.Feb 7 2018, 2:15 PM
cy marked an inline comment as done.

Added sentence to say that the constraint handler doesn't necessarily return and if it does a return code is set.

kib accepted this revision.Feb 7 2018, 2:30 PM
This revision was not accepted when it landed; it landed in state Needs Review.Feb 16 2018, 5:49 AM
Closed by commit rS329361: Document memset_s(3). memset_s(3) is defined in (authored by cy, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.