Page MenuHomeFreeBSD

Add C11 Appendix K function memset_s
ClosedPublic

Authored by trix_juniper.net on Mar 5 2017, 7:23 PM.

Details

Summary

Implements the memset_s function as specified by the c11 spec.
Specifically iso/iec 9899:2011 k 3.7.4.1
Other needed supporting types and defines added as specified in the c11 spec.
Unit tests added to cover new function.

Test Plan

run checkworld before and after, no failures on either.

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.

Event Timeline

trix_juniper.net retitled this revision from to Add C11 Appendix K function memset_s.
trix_juniper.net updated this object.
trix_juniper.net edited the test plan for this revision. (Show Details)
trix_juniper.net added reviewers: kib, ed, emaste, stevek.
trix_juniper.net set the repository for this revision to rS FreeBSD src repository.
imp added a comment.Mar 5 2017, 7:27 PM

The code and the tests look good.
There's no man page.

ed edited edge metadata.Mar 5 2017, 7:44 PM

Though the _s functions did make it into the C11 standard, there is a reason why they are in an appendix and not part of the core specification. They are controversial. I personally find them pretty much useless, and think of them as garbage that Microsoft managed to shove through WG14's throat. I've once read that Microsoft's implementation of these functions is also incompatible with what's described in the C11 appendix, but I have no sources to back that up right now.

In my opinion the only interesting part of the appendix is the definition of errno_t, which I think would improve the readability of code if used consistently. For example, if the man page of posix_fadvise() would use it, it would be more obvious that this function returns an error number, instead of depending on errno. Apart from that, I'm against adding any other features from the appendix. That said, if others disagree with me, then feel free to commit.

imp added a comment.Mar 5 2017, 7:45 PM

Yea, the _s functions are MS' NIH for other interfaces that had pre-dated their crazy by a lot.

kib added inline comments.Mar 5 2017, 8:29 PM
include/stddef.h
75 ↗(On Diff #26009)

How this include brace is useful without additional machinery ? The STD C specifies that users should define _ _STDC_WANT_LIB_EXT1_ _ before including corresponding headers, to get _s symbols. Also, the standard requires that the symbol STDC_LIB_EXT1 was only defined by the _implementation_, and only in case of complete conformance.

lib/libc/string/memset_s.c
36 ↗(On Diff #26009)

This function violates every single rule in style(9). It should start as

errno_t
memset_s()
{
    errno_t ret;
    rsize_t lim;
    unsigned char v;
    ... *dst;

  ret = EINVAL;
38 ↗(On Diff #26009)

Too many ().

If there are runtime constraint violations, corresponding handlers should be called, am I right ? I.e. libc must grow registration interfaces as well.

48 ↗(On Diff #26009)

return (ret);

The reason for adding this function is to improve security.
Memset is used to overwrite sensitive memory.
But the compiler can legally optimize away a memset.
The compiler can not optimize away memset_s.

A link with a deeper explanation
https://www.securecoding.cert.org/confluence/display/c/MSC06-C.+Beware+of+c
ompiler+optimizations

ed added a comment.Mar 6 2017, 3:55 PM

The reason for adding this function is to improve security.
Memset is used to overwrite sensitive memory.
But the compiler can legally optimize away a memset.
The compiler can not optimize away memset_s.
A link with a deeper explanation
https://www.securecoding.cert.org/confluence/display/c/MSC06-C.+Beware+of+c
ompiler+optimizations

In that case I find having memset_s() a better solution than adding some custom extension, like OpenBSD's explicit_bzero(). memset_s() is at least part of some standard. Still, I wonder about its effectiveness. Even these functions don't prevent a compiler from copying around/caching data into registers/memory, meaning parts of data may still be recovered.

That said, I don't want this to block you from adding this function to the C library. If other people think this function is useful, then feel free to commit.

I am working on the style problems.

On the use of STDC_LIB_EXT1
I would rather not have to implement the whole appendix K to just get
memset_s.
What are folks thoughts on removing STDC_LIB_EXT1 use altogether ?

kib edited edge metadata.Mar 6 2017, 6:53 PM

I am working on the style problems.
On the use of STDC_LIB_EXT1
I would rather not have to implement the whole appendix K to just get
memset_s.
What are folks thoughts on removing STDC_LIB_EXT1 use altogether ?

I am fine with putting it into BSD_VISIBLE space. Still, do you plan to implement handlers and registration for handlers ? IMO this better be done, even if only for memset_s.

The handlers certainly would be useful if a full implementation of K was
done.
For a single function I don¹t think it would be very useful.
How much more useful would a handler have over a user just checking the
return ?

kib added a comment.Mar 7 2017, 12:52 PM

The handlers certainly would be useful if a full implementation of K was
done.
For a single function I don¹t think it would be very useful.
How much more useful would a handler have over a user just checking the
return ?

If user must perform pre-call validation, then the function interface is certainly not the interface described in the standard. This means that the function should not be called same as specified in the standard, to prevent both confusion and accidental invalid usage, where consumer expects the handlers to be called, while our implementation does something different.

If we don’t give them the function to register the handler there shouldn’t
be any expectation ;)

The handler interface is a part of the full K support and I believe
overkill if we only have a single K function.
I am in favor of full support.
Is that what everyone wants ?

A new idea..
What do people think about making the pointer access safer ?
It should be possible to look into the processes memory space and tell if
the memory accessed by the function is mostly valid.

Tom

ed added a comment.Mar 7 2017, 2:50 PM

If we don’t give them the function to register the handler there shouldn’t
be any expectation ;)

Exactly. That's also how macOS implements it, apparently.

kib added a comment.Mar 7 2017, 3:38 PM
In D9903#204944, @ed wrote:

If we don’t give them the function to register the handler there shouldn’t
be any expectation ;)

Exactly. That's also how macOS implements it, apparently.

IMO when implementing a part of the standard it is preferable to implement self-contained part. As I noted above, without handlers and registration interfaces, it is not a standard-defined memset_s. It is fine to implement only memset_s, but still everything that is referenced by the std text under this name, should be provided. IMO.

Add set_contraint_handler_s
Move setting of STDC_WANT_LIB_EXT1 to makefiles
Fix some style issues

kib added inline comments.Mar 14 2017, 10:44 AM
include/stddef.h
75 ↗(On Diff #26239)

This is not how the namespace handled on FreeBSD. By default, which is indicated by the __BSD_VISIBLE being defined as non-zero, we spam all existing features into the visible namespace. If the feature has a specific standard-mandated knob to enable, or if it should be disabled if a restrictive subset of features is requested (like _ANSI_SOURCE), we also provide additional feature-test symbol for the libc use.

Please look at the __XXX_VISIBLE symbols in sys/cdefs.h.

I think that the feature requires some similar symbol, e.g. __STDC_LIB_EXT1_VISIBLE to trigger the visibility, and cdefs.h should translate user-supplied WANT into it.

include/stdlib.h
329 ↗(On Diff #26239)

This line and all new lines following it are too long, please wrap at columns ~70-75.

lib/libc/stdlib/Symbol.map
124 ↗(On Diff #26239)

Please order alphabetically.

lib/libc/stdlib/set_constraint_handler_s.c
39 ↗(On Diff #26239)

Style prohibits local declarations with initialization. Also, a blank line is needed between decl section and the first statement.

40 ↗(On Diff #26239)

Use _ch == NULL comparision for pointers. Note that next if() uses style-compliant check for the same condition.

44 ↗(On Diff #26239)

Continuation line should have 4-spaces indent.

45 ↗(On Diff #26239)

Same.

47 ↗(On Diff #26239)

return (ret);

51 ↗(On Diff #26239)

If your intent in selecting the function name was to put it into the implementation-private namespace to minimize issues with statically-linked programs, then __throw_contraint_handler_s is a better name for the function.

53 ↗(On Diff #26239)

There must be an empty line after '{' and the first statement in a function without locals.

if (_ch != NULL && *_ch != NULL)
59 ↗(On Diff #26239)

Continuation line should have 4-spaces ident.

61 ↗(On Diff #26239)

Blank line.

66 ↗(On Diff #26239)

4-spaces indent.

lib/libc/string/Makefile.inc
32 ↗(On Diff #26239)

Why this line appears in diff ?

lib/libc/string/Symbol.map
110 ↗(On Diff #26239)

Order alphabetically.

lib/libc/string/memset_s.c
34 ↗(On Diff #26239)

extern keyword is excessive.

40 ↗(On Diff #26239)

This function still violates almost all style rules.

No initialization in local declaration. Use single space to separate type name from the variable name.

Same for all locals in this function.

44 ↗(On Diff #26239)

Blank line before this one.

48 ↗(On Diff #26239)

4-spaces indent for continuation line.

58 ↗(On Diff #26239)

return (ret);

Add __EXT1_VISIBLE construct to cdefs.h
Fix some style issues

kib added inline comments.Mar 21 2017, 6:03 PM
include/stdlib.h
330 ↗(On Diff #26443)

Please use 4-spaces continuation line, there and below.

lib/libc/stdlib/set_constraint_handler_s.c
39 ↗(On Diff #26239)

Not fixed.

47 ↗(On Diff #26239)

Not fixed.

55 ↗(On Diff #26443)

!= NULL still not applied.

lib/libc/string/memset_s.c
42 ↗(On Diff #26443)

Still tab instead of space.

sys/sys/cdefs.h
778 ↗(On Diff #26443)

This is a generic comment to the change in cdefs.h. I do not think that the proposed setup of visibility would be useful.

As I noted before, by default we expose everything, which in particular means that BSD_VISIBLE is 1. As such, we should define EXT1_VISIBLE to 1 if we are providing the default environments.

For all other cases (of ANSI C and strict posix environments) __EXT1_VISIBLE must be 0 by default, as you defined, but setting it to 1 on STDC_WANT_LIB_EXT1 should be independent of the restriction macros. To say it simple, the check for the _WANT_ should be done in separate #if block and not in #elif.

Move STDC_WANT_LIB_EXT1 to its own if block
Cleanups

kib added a comment.Mar 24 2017, 10:02 PM

The remaining issues are minor, what do you plan about the committing stuff ? Do you have commit bit yourself or somebody with src commit bit who is willing to commit this ?

include/stdlib.h
338 ↗(On Diff #26639)

No need for space between 's' and '('.

341 ↗(On Diff #26639)

Please use _Noreturn instead of directly writing gcc attribute syntax.

lib/libc/stdlib/set_constraint_handler_s.c
61 ↗(On Diff #26239)

still.

49 ↗(On Diff #26639)

return (ret); (still)

lib/libc/string/memset_s.c
42 ↗(On Diff #26639)

Still tab.

sys/sys/cdefs.h
778 ↗(On Diff #26639)

I suspect you want to #undef __EXT1_VISIBLE before redefining. Some warning levels on some compilers catch this.

780 ↗(On Diff #26639)

Why ? This overrides the setting from the _BSD_VISIBLE case. Just drop the #else block.

I do not have commit permissions.

lib/libc/string/memset_s.c
42 ↗(On Diff #26639)

Not sure what you mean here

sys/sys/cdefs.h
780 ↗(On Diff #26639)

There should be a way for the user to explicitly turn this feature off, this would be done if the user did -DSTDC_WANT_EXT1=0,

from K3.1.1
The functions, macros, and types declared or defined in K.3 and its subclauses are not
declared or defined by their respective headers if _ _STDC_WANT_LIB_EXT1_ _ is
defined as a macro which expands to the integer constant 0 at the point in the source file where the appropriate header is first included.

kib accepted this revision.Mar 25 2017, 10:18 AM

Only style issues are left.

I will handle the commit, unless you have another committer willing to care about the patch.

lib/libc/string/memset_s.c
42 ↗(On Diff #26639)

I mean that the type and variable names are still separated by tab and not space.

sys/sys/cdefs.h
780 ↗(On Diff #26639)

I see, thanks.

This revision is now accepted and ready to land.Mar 25 2017, 10:18 AM
trix_juniper.net edited edge metadata.

Cleanups
Simplify logic for STDC_WANT_LIB_EXT1 in cdefs.h, remove it's CFLAGS in makefiles.

This revision now requires review to proceed.Mar 28 2017, 1:47 AM
kib added a comment.Mar 28 2017, 9:17 AM

I noted that set_constrain_handler_s() is not thread-safe. Please see D10161 for my take on it and several other style editings.

If you agree with the change, please provide me with the commit message. Specifically, I want to get from you the attributions (like 'Submitted by: mail / name', 'Sponsored by: company' as applicable).

Thank you.

My comment is in D10161

Attributes..
Submitted by: trix@juniper.net / Tom Rix
Sponsored by: Juniper Networks

Thanks for your help,
Tom

This revision was automatically updated to reflect the committed changes.