8548 want memset_s(3C)
Review Request #640 — Created Aug. 6, 2017 and submitted
Information | |
---|---|
yuripv | |
illumos-gate | |
master | |
8548 | |
df4956e... | |
Reviewers | |
general | |
This adopts the
memset_s()
implementation from FreeBSD.Newly added symbols are visible if
_STRICT_SYMBOLS
is not defined, or__STDC_WANT_LIB_EXT1__
is set to 1 (via__EXT1_VISIBLE
).
__STDC_LIB_EXT1__
is not set as we don't implement all bounds-checking functions at the moment.
- new libc-tests test cases
- compiled a simple test case on updated system, in turn calling
memset()
andmemset_s()
beforefree()
and checked that (both with gcc 4.4.4 and clang 3.8)memset()
gets optimized away with -O >= 1, whilememset_s()
doesn't (exactly what we need)
-
Shouldn't there also be manual page updates for these?
-
usr/src/lib/libc/port/gen/memset_s.c (Diff revision 1) Can the constraint error messages not be in terms of the implementation names of these values? While I know that the standard uses those variable names, it seems like a user whose violated the constraint might not know.
-
usr/src/lib/libc/port/gen/memset_s.c (Diff revision 1) The standard says that 'n shall not be greater than smax.' If it is then it's a runtime-constraint violation. Based on this, don't we still have to throw the runtime-constraint violation? Specifically, section K3.1.4 point 3 indicates that we should still throw the runtime-constraint violation after performing alternate activities. So, it seems while in general we're getting the right behavior, we're not properly throwing the constraint violation which is specified throughout K.3.7.4.1. In fact, it seems like if smax > n should cause a throwing of the constraint solver after we do the copy. Of course, why the copy even makes sense in this case and still returning an error makes sense, I don't know, but that's at least hte words of the standard that I see (as weird as it seems).
-
usr/src/lib/libc/port/gen/set_constraint_handler_s.c (Diff revision 1) Can we use a traditional MUTEX and make sure it's an error check mutex that way we can get errors here?
-
usr/src/lib/libc/port/gen/set_constraint_handler_s.c (Diff revision 1) Do we really need to check these? I think we can just take the uncontended lock and it'll simplify the code. Also, if we do the earlier bits I recommended with the mutex_t then this should just be mutex_enter() and mutex_exit() which will do all the error checking we need.
-
usr/src/lib/libc/port/gen/set_constraint_handler_s.c (Diff revision 1) I think we should use follow the recommendation of K.3.6.1.1 point 4, which says if no constraint handler is defined, it's an implementation defined one which can abort. So in this case if ch is NULL, we should abort().
-
usr/src/lib/libc/port/gen/set_constraint_handler_s.c (Diff revision 1) This isn't sufficient to meet the standard. The standard says that while we can write the message in an implementation defined format, it must include 'msg'. This is defined in K.3.6.1.2 point 3.
-
usr/src/test/libc-tests/runfiles/default.run (Diff revision 1) If you're going to move these into their own directory, can you please make sure that we are still building 32-bit and 64-bit versions of things? I did that rather purposefully for all of the other C11 bits that you've moved.
-
usr/src/test/libc-tests/tests/c11/Makefile (Diff revision 1) I there a reason that the original top-level Makefile didn't work? I changed things there to try and make it so we didn't have to keep adding subdirectories and because it made sure that you always had 32-bit and 64-bit versions of things to make sure that we were always exercising both versions of libc.
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+513 -24) |
-
Footnote 367 in the standard reads "Implementations that do not define __STDC_LIB_EXT1__ are not required to conform to these specifications." Also, the examples from https://www.securecoding.cert.org/confluence/pages/viewpage.action?pageId=121143577 only define __STDC_WANT_LIB_EXT1__ after first asserting that __STDC_LIB_EXT1__ is defined. Therefore for this issue, 8548, what about just adding a conforming memset_s definition (with related, conforming types and macros) but not worrying about handling __STDC_WANT_LIB_EXT1__ until some future when all conforming functions, types, and macros are added and __STDC_LIB_EXT1__ might be defined? Electing to do this will also decide illumos's position w.r.t. K.3.1.1.3, "It is implementation-defined whether the functions, macros, and types declared or defined in K.3 and its subclauses are declared or defined by their respective headers if __STDC_WANT_LIB_EXT1__ is not defined as a macro at the point in the source file where the appropriate header is first included," to be "defined by default." A final question: should an implementation ever define __STDC_WANT_LIB_EXT1__ as you've done here (e.g., in usr/src/lib/libc/amd64/Makefile), or is that definition meant to be left to users of the implementation (e.g., by usr/src/tests as an example)?
Change Summary:
update according to FreeBSD review, always use
abort_handler_s()
.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+507 -24) |
Change Summary:
drop the libc-tests infra changes
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+452 -4) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+449 -6) |
Change Summary:
- use
memset()
internally called via volatile function pointer - fix
memset_s()
logic a bit more -- if boths
andsmax
are valid, we need to fill the memory buffer, previously we wouldn't do that ifn
is greater thanRSIZE_MAX
- add man pages
Description: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||
Commit: |
|
||||||||||||||||||
Diff: |
Revision 6 (+710 -7) |
Change Summary:
minor nits
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+705 -7) |
-
Thanks for all of the work here. I think this is pretty close and have a few additional follow ups.
-
usr/src/lib/libc/port/gen/set_constraint_handler_s.c (Diff revisions 1 - 7) Sorry I should have thought of this sooner. I think we may want to replace this with a call to common_panic(). That will make sure that not only it writes the standard error stuff for you, but it'll also save it to the buffer that it used for assertion failures, which will make it easier to debug. What do you think?
-
usr/src/lib/libc/port/gen/set_constraint_handler_s.c (Diff revisions 1 - 7) It's both if it's never called or it it was set to NULL, right? The implementation matches that, just wanted to mention it for the actual implementation.
-
usr/src/uts/common/sys/feature_tests.h (Diff revisions 1 - 7) I think in this case because there's an explicit macro that we've talked about we want to make this line:
#if !defined(_STRICT_SYMBOLS) || defined(__STDC_WANT_LIB_EXT1__)
Though the latter defined may need to be an == 1 or something depending on the specifics of the spec. But basically even if you've enabled strict symbols, if you've asked for the env with it, then we want it to be there, regardless.
-
usr/src/uts/common/sys/feature_tests.h (Diff revisions 1 - 7) Is there a reason that we're doing a specific #undef? Is this just to protect ourselves? What if someone else defines it because they have a full impl?
-
-
usr/src/man/man3c/set_constraint_handler_s.3c (Diff revision 7) Missing return type for the constraint_handler_t here.
-
usr/src/man/man3c/set_constraint_handler_s.3c (Diff revision 7) We should state what it is in our implementation.
-
usr/src/man/man3c/set_constraint_handler_s.3c (Diff revision 7) We should state that it will cause it to abort.
-
-
usr/src/man/man3c/set_constraint_handler_s.3c (Diff revision 7) Should we mention that it none was previously set that we'll return NULL?
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+709 -8) |
-
-
usr/src/lib/libc/port/threads/assfail.c (Diff revision 8) In exposing common_panic() outside this unit, it might be advisable to revise below to copy `head' into the `msg' buffer more warily to avoid an overrun.
Change Summary:
rebase and finally make lint happy so this can be integrated
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+720 -8) |
Change Summary:
pkgfmt
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 10 (+721 -8) |
-
-
-
usr/src/man/man3c/set_constraint_handler_s.3c (Diff revision 10) Can we add the MT-Level to this manual?
Change Summary:
add mt-level to man pages
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 11 (+727 -10) |