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.
Details
Details
run checkworld before and after, no failures on either.
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
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.
Comment Actions
Yea, the _s functions are MS' NIH for other interfaces that had pre-dated their crazy by a lot.
| 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); |
Comment Actions
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
Comment Actions
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.
Comment Actions
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 ?
Comment Actions
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.
Comment Actions
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 ?
Comment Actions
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.
Comment Actions
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
Comment Actions
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.
Comment Actions
Add set_contraint_handler_s
Move setting of STDC_WANT_LIB_EXT1 to makefiles
Fix some style issues
| 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); |
| include/stdlib.h | ||
|---|---|---|
| 330 ↗ | (On Diff #26443) | Please use 4-spaces continuation line, there and below. |
| lib/libc/stdlib/set_constraint_handler_s.c | ||
| 55 ↗ | (On Diff #26443) | != NULL still not applied. |
| 39 ↗ | (On Diff #26239) | Not fixed. |
| 47 ↗ | (On Diff #26239) | Not fixed. |
| 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. |
Comment Actions
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 | ||
| 49 ↗ | (On Diff #26639) | return (ret); (still) |
| 61 ↗ | (On Diff #26239) | 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. |
Comment Actions
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 |
Comment Actions
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. |
Comment Actions
Cleanups
Simplify logic for STDC_WANT_LIB_EXT1 in cdefs.h, remove it's CFLAGS in makefiles.
Comment Actions
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.
Comment Actions
My comment is in D10161
Attributes..
Submitted by: trix@juniper.net / Tom Rix
Sponsored by: Juniper Networks
Thanks for your help,
Tom