8548 want memset_s(3C)

Reviews
Diff

- Download Diff

Information
Submitter:	Yuri Pankov
Repository:	illumos-gate
Branch:	master
Bugs:	8548
Depends On:
Commit:	df4956e...
Reviewers
Groups:	general
People:

Description

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.

Testing Done


new libc-tests test cases
compiled a simple test case on updated system, in turn calling memset() and memset_s() before free() and checked that (both with gcc 4.4.4 and clang 3.8) memset() gets optimized away with -O >= 1, while memset_s() doesn't (exactly what we need)

Issues

0
19
3
All Issues: 22

Description	From	Last Updated
Can the constraint error messages not be in terms of the implementation names of these values? While I know that ...	Robert Mustacchi	Aug. 7, 2017, 4:39 p.m.
The standard says that 'n shall not be greater than smax.' If it is then it's a runtime-constraint violation. Based ...	Robert Mustacchi	Aug. 7, 2017, 4:39 p.m.
Can we use a traditional MUTEX and make sure it's an error check mutex that way we can get errors ...	Robert Mustacchi	Aug. 7, 2017, 4:40 p.m.
Do we really need to check these? I think we can just take the uncontended lock and it'll simplify the ...	Robert Mustacchi	Aug. 7, 2017, 4:41 p.m.
I think we should use follow the recommendation of K.3.6.1.1 point 4, which says if no constraint handler is defined, ...	Robert Mustacchi	Aug. 7, 2017, 4:42 p.m.
This isn't sufficient to meet the standard. The standard says that while we can write the message in an implementation ...	Robert Mustacchi	Aug. 7, 2017, 4:43 p.m.
If you're going to move these into their own directory, can you please make sure that we are still building ...	Robert Mustacchi	Aug. 12, 2017, 10:11 a.m.
I there a reason that the original top-level Makefile didn't work? I changed things there to try and make it ...	Robert Mustacchi	Aug. 12, 2017, 10:12 a.m.
Should extensions turn this on by default?	Robert Mustacchi	Aug. 12, 2017, 12:20 p.m.
Sorry I should have thought of this sooner. I think we may want to replace this with a call to ...	Robert Mustacchi	Aug. 14, 2017, 1:41 p.m.
It's both if it's never called or it it was set to NULL, right? The implementation matches that, just wanted ...	Robert Mustacchi	Aug. 14, 2017, 1:05 p.m.
I think in this case because there's an explicit macro that we've talked about we want to make this line: ...	Robert Mustacchi	Aug. 14, 2017, 12:59 p.m.
Is there a reason that we're doing a specific #undef? Is this just to protect ourselves? What if someone else ...	Robert Mustacchi	Aug. 15, 2017, 4:11 p.m.
Missing the word 'the' before .Fn	Robert Mustacchi	Aug. 14, 2017, 1:06 p.m.
Missing return type for the constraint_handler_t here.	Robert Mustacchi	Aug. 14, 2017, 1:08 p.m.
We should state what it is in our implementation.	Robert Mustacchi	Aug. 14, 2017, 1:22 p.m.
We should state that it will cause it to abort.	Robert Mustacchi	Aug. 14, 2017, 1:24 p.m.
s/implementation/C library?	Robert Mustacchi	Aug. 14, 2017, 1:29 p.m.
Should we mention that it none was previously set that we'll return NULL?	Robert Mustacchi	Aug. 14, 2017, 1:30 p.m.
In exposing common_panic() outside this unit, it might be advisable to revise below to copy `head' into the `msg' buffer ...	C Fraire	Aug. 17, 2017, 10:36 p.m.
Can we add the MT-Level to this manual?	Robert Mustacchi	Aug. 8, 2018, 1:07 a.m.
Can we add the MT-Level to this manual?	Robert Mustacchi	Aug. 8, 2018, 1:07 a.m.

Ship it!

Ship It!

Shouldn't there also be manual page updates for these?

Yuri Pankov Aug. 7, 2017, 2:31 p.m.

Will do.

And thank you very much for taking the time to review this.

Yuri Pankov Aug. 12, 2017, 8:16 p.m.

Man pages added. I think this looks functionally complete now.

usr/src/lib/libc/port/gen/memset_s.c (Diff revision 1)

Show all issues

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.

Yuri Pankov Aug. 7, 2017, 2:31 p.m.
```
Do you have any proposals here?
```

Yuri Pankov Aug. 7, 2017, 4:43 p.m.

I've updated the argument names to be the same as in documentation, messages should make more sense now.

Yuri Pankov Aug. 12, 2017, 8:17 p.m.

Reverted this change, these names are in standard, and now in man page as well, so it shouldn't be really hard to find out :-)

usr/src/lib/libc/port/gen/memset_s.c (Diff revision 1)

Show all issues

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).

Yuri Pankov Aug. 7, 2017, 2:31 p.m.

I'll check the standard (again) and will fix this.

Yuri Pankov Aug. 7, 2017, 4:43 p.m.

You are right, updated the code to throw the constraint handler if count > destsz, and we now correctly return EINVAL, test case updated as well.

Robert Mustacchi Aug. 8, 2017, 11:33 a.m.

Might be worth seeing if FBSD agrees on that reading of things, so we are all on the same page.

Yuri Pankov Aug. 12, 2017, 8:44 a.m.

Done, the change was accepted with modifications to abort_handler_s() - used write() instead of fprintf() to not introduce more non-async-signal-safe code.

usr/src/lib/libc/port/gen/set_constraint_handler_s.c (Diff revision 1)

Show all issues

Can we use a traditional MUTEX and make sure it's an error check mutex that way we can get errors here?

Yuri Pankov Aug. 7, 2017, 2:31 p.m.
```
Will do.
```
Yuri Pankov Aug. 7, 2017, 4:43 p.m.
```
Done.
```

usr/src/lib/libc/port/gen/set_constraint_handler_s.c (Diff revision 1)

Show all issues

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.

Yuri Pankov Aug. 7, 2017, 2:31 p.m.

This was done this way in original implementation to not do any locking if we aren't multithreaded -- I'll remove the checks if you think the benefit is not worth it.

Will change the calls as well.

Yuri Pankov Aug. 7, 2017, 4:43 p.m.
```
Done, checks removed.
```

usr/src/lib/libc/port/gen/set_constraint_handler_s.c (Diff revision 1)

Show all issues

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().

Yuri Pankov Aug. 7, 2017, 2:31 p.m.
```
I'll check the standard and will fix.
```

Yuri Pankov Aug. 7, 2017, 4:43 p.m.

Done, now we choose what to do based on build type - for DEBUG, we call abort_handler_s(), for non-DEBUG - ignore_handler_s(), the reason for calling the "ignore" one at all is that I guess we could eventually enhance it to do something.

Robert Mustacchi Aug. 8, 2017, 11:33 a.m.

I would probably always call the abort handler by default, regardless if it's a debug or non-debug build, as it represents a constraint violation.

Yuri Pankov Aug. 12, 2017, 8:44 a.m.
```
Done.
```

usr/src/lib/libc/port/gen/set_constraint_handler_s.c (Diff revision 1)

Show all issues

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.

Yuri Pankov Aug. 7, 2017, 2:31 p.m.
```
OK, will check the standard and fix.
```
Yuri Pankov Aug. 7, 2017, 4:43 p.m.
```
Done.
```

usr/src/test/libc-tests/runfiles/default.run (Diff revision 1)

Show all issues

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.

Yuri Pankov Aug. 7, 2017, 2:31 p.m.

Yeah, will fix all that to include both 32- and 64-bit versions, sorry, wanted to hear if we want this at all (at least done this way) before trying to make it look ideal :-)

Yuri Pankov Aug. 12, 2017, 10:12 a.m.
```
Dropped the changes.
```

usr/src/test/libc-tests/tests/c11/Makefile (Diff revision 1)

Show all issues

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.

Yuri Pankov Aug. 7, 2017, 2:31 p.m.

I'll fix all that once we are good with the code itself.

Yuri Pankov Aug. 12, 2017, 10:12 a.m.
```
Dropped the changes.
```

usr/src/uts/common/sys/feature_tests.h (Diff revision 1)

Show all issues

Should extensions turn this on by default?

Yuri Pankov Aug. 7, 2017, 2:31 p.m.

That's exactly the question I do NOT want to answer myself -- TBH, I like how it's done in FreeBSD - unless we require strict conforming environment, we have everything visible; I think Garrett did a lot of related work in his fork for this as well. But as it is now, I want someone else (you?) to decide on that :-)

Robert Mustacchi Aug. 8, 2017, 11:33 a.m.

Right, I agree entirely that unless you request something strictly confirming, that it should be visibile. It wasn't clear to me from that the current thing was visible by default.

Yuri Pankov Aug. 12, 2017, 8:44 a.m.

Right now it needs consumers to define __STDC_WANT_LIB_EXT1__ for these symbols to be visible. I'm all for making them visible by default, just not sure what exactly I need to check to hide them in strict environments -- is it _STRICT_STDC, or _STRICT_SYMBOLS, or __XOPEN_OR_POSIX -- I'm really lost in our ifdefs spaghetti...

Yuri Pankov Aug. 12, 2017, 12:20 p.m.

Looking at the feature_tests.h, I've used the _STRICT_SYMBOLS (and fixed few typos there).

Commit:

-	0140178b77c6361485fafb6e2b947e9556dc5b46
+	c570808238ba972cc1372d86b92891024a701b1b

Diff:

Revision 2 (+513 -24)

Show changes

	usr/src/head/stddef.h
	usr/src/head/stdint.h
	usr/src/head/stdlib.h
	usr/src/head/string.h
	usr/src/lib/libc/amd64/Makefile
	usr/src/lib/libc/i386/Makefile.com
	usr/src/lib/libc/inc/libc.h
	usr/src/lib/libc/port/mapfile-vers
	16 more

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)?

Yuri Pankov Aug. 11, 2017, 12:08 p.m.

I wanted the memset_s() and friends to be guarded by something until we switch to "everything is visible unless strict conforming environment is requested" model, even if we don't define __STDC_LIB_EXT1__.

re final question: implementation defines __STDC_WANT_LIB_EXT1__ for its own needs (because of the previous answer), i.e. the definition is not visible to consumers.

Yuri Pankov Aug. 12, 2017, 12:19 p.m.

Hopefully, I've addressed all this in the latest update.

Change Summary:

update according to FreeBSD review, always use abort_handler_s().

Commit:

-	c570808238ba972cc1372d86b92891024a701b1b
+	4b62e0a8f46c9be006c192d5c7e91daa873ef4f7

Diff:

Revision 3 (+507 -24)

Show changes

	usr/src/head/stddef.h
	usr/src/head/stdint.h
	usr/src/head/stdlib.h
	usr/src/head/string.h
	usr/src/lib/libc/amd64/Makefile
	usr/src/lib/libc/i386/Makefile.com
	usr/src/lib/libc/inc/libc.h
	usr/src/lib/libc/port/mapfile-vers
	16 more

Change Summary:

drop the libc-tests infra changes

Commit:

-	4b62e0a8f46c9be006c192d5c7e91daa873ef4f7
+	a4ac05cd228896479405805f26ab0a3aac34f91d

Diff:

Revision 4 (+452 -4)

Show changes

	usr/src/head/stddef.h
	usr/src/head/stdint.h
	usr/src/head/stdlib.h
	usr/src/head/string.h
	usr/src/lib/libc/amd64/Makefile
	usr/src/lib/libc/i386/Makefile.com
	usr/src/lib/libc/inc/libc.h
	usr/src/lib/libc/port/mapfile-vers
	11 more

Commit:

-	a4ac05cd228896479405805f26ab0a3aac34f91d
+	6fd01878e44bc9e41ec2a6426eee08cad7117b04

Diff:

Revision 5 (+449 -6)

Show changes

	usr/src/head/stdint.h
	usr/src/head/iso/stddef_iso.h
	usr/src/head/iso/stdlib_c11.h
	usr/src/head/iso/string_iso.h
	usr/src/lib/libc/amd64/Makefile
	usr/src/lib/libc/i386/Makefile.com
	usr/src/lib/libc/inc/libc.h
	usr/src/lib/libc/port/mapfile-vers
	8 more

Ship it!

Looks good to me.

Change Summary:


use memset() internally called via volatile function pointer
fix memset_s() logic a bit more -- if both s and smax are valid, we need to fill the memory buffer, previously we wouldn't do that if n is greater than RSIZE_MAX
add man pages

Description:

		This adopts the `memset_s()` implementation from FreeBSD.

~		The symbol is guarded by `__STDC_WANT_LIB_EXT1__`, and `__STDC_LIB_EXT1__` is not set as we don't implement all bounds-checking functions at the moment.
	~	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.

Testing Done:

~		See added test cases.
	~	new libc-tests test cases
	+	compiled a simple test case on updated system, in turn calling `memset()` and `memset_s()` before `free()` and checked that (both with gcc 4.4.4 and clang 3.8) `memset()` gets optimized away with -O >= 1, while `memset_s()` doesn't (exactly what we need)

Commit:

-	6fd01878e44bc9e41ec2a6426eee08cad7117b04
+	6d1a6460c8911d4f747aa91dd63b7c633183bce3

Diff:

Revision 6 (+710 -7)

Show changes

	usr/src/head/stdint.h
	usr/src/head/iso/stddef_iso.h
	usr/src/head/iso/stdlib_c11.h
	usr/src/head/iso/string_iso.h
	usr/src/lib/libc/amd64/Makefile
	usr/src/lib/libc/i386/Makefile.com
	usr/src/lib/libc/inc/libc.h
	usr/src/lib/libc/port/mapfile-vers
	12 more

Change Summary:

minor nits

Commit:

-	6d1a6460c8911d4f747aa91dd63b7c633183bce3
+	1ecbadeb3dd5e660f3b9bfd57653e104e2f3211f

Diff:

Revision 7 (+705 -7)

Show changes

	usr/src/head/stdint.h
	usr/src/head/iso/stddef_iso.h
	usr/src/head/iso/stdlib_c11.h
	usr/src/head/iso/string_iso.h
	usr/src/lib/libc/amd64/Makefile
	usr/src/lib/libc/i386/Makefile.com
	usr/src/lib/libc/inc/libc.h
	usr/src/lib/libc/port/mapfile-vers
	12 more

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)

Show all issues

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)

Show all issues

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)

Show all issues

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.

Yuri Pankov Aug. 14, 2017, 2:09 p.m.

There's an override below depending on the value of __STDC_WANT_LIB_EXT1__ -- bringing both checks in one block doesn't make it more readable exactly because we need to check the value and not just if it's set.

Robert Mustacchi Aug. 14, 2017, 2:28 p.m.

I somehow missed that in the incremental. My mistake.

usr/src/uts/common/sys/feature_tests.h (Diff revisions 1 - 7)

Show all issues

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?

Yuri Pankov Aug. 14, 2017, 2:09 p.m.

I meant it as a placeholder -- if it or the entire block doesn't make sense, I'll remove it.

usr/src/man/man3c/memset_s.3c (Diff revision 7)

Show all issues

Missing the word 'the' before .Fn

Yuri Pankov Aug. 14, 2017, 2:09 p.m.
```
Fixed.
```

usr/src/man/man3c/set_constraint_handler_s.3c (Diff revision 7)

Show all issues

Missing return type for the constraint_handler_t here.

Yuri Pankov Aug. 14, 2017, 2:09 p.m.
```
Fixed.
```

usr/src/man/man3c/set_constraint_handler_s.3c (Diff revision 7)

Show all issues

We should state what it is in our implementation.

Yuri Pankov Aug. 14, 2017, 2:09 p.m.
```
Always NULL.
```

usr/src/man/man3c/set_constraint_handler_s.3c (Diff revision 7)

Show all issues

We should state that it will cause it to abort.

Yuri Pankov Aug. 14, 2017, 2:09 p.m.

Simply removed that sentence, abort_handler_s() describes itself as default below.

usr/src/man/man3c/set_constraint_handler_s.3c (Diff revision 7)

Show all issues

s/implementation/C library?

usr/src/man/man3c/set_constraint_handler_s.3c (Diff revision 7)

Show all issues

Should we mention that it none was previously set that we'll return NULL?

Yuri Pankov Aug. 14, 2017, 2:09 p.m.
```
Done.
```

Commit:

-	1ecbadeb3dd5e660f3b9bfd57653e104e2f3211f
+	cf7105e071cebdbb2c291966e5ab9c725fd12cd1

Diff:

Revision 8 (+709 -8)

Show changes

	usr/src/head/stdint.h
	usr/src/head/iso/stddef_iso.h
	usr/src/head/iso/stdlib_c11.h
	usr/src/head/iso/string_iso.h
	usr/src/lib/libc/amd64/Makefile
	usr/src/lib/libc/i386/Makefile.com
	usr/src/lib/libc/inc/libc.h
	usr/src/lib/libc/port/mapfile-vers
	13 more

Ship it!

Ship It!

usr/src/lib/libc/port/threads/assfail.c (Diff revision 8)

Show all issues

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.

Yuri Pankov Aug. 17, 2017, 10:43 p.m.

Well, there's still no user input involved, and it's only about the head, so let's leave it as it is.

Ship it!

Ship It!

Change Summary:

rebase and finally make lint happy so this can be integrated

Commit:

-	cf7105e071cebdbb2c291966e5ab9c725fd12cd1
+	86545b0631b86e60545a9db204aa151951838b11

Diff:

Revision 9 (+720 -8)

Show changes

	usr/src/head/stdint.h
	usr/src/head/iso/stddef_iso.h
	usr/src/head/iso/stdlib_c11.h
	usr/src/head/iso/string_iso.h
	usr/src/lib/libc/amd64/Makefile
	usr/src/lib/libc/i386/Makefile.com
	usr/src/lib/libc/inc/libc.h
	usr/src/lib/libc/port/mapfile-vers
	13 more

Change Summary:

pkgfmt

Commit:

-	86545b0631b86e60545a9db204aa151951838b11
+	5d89d91604d6a63660ec588278d9ce8224953776

Diff:

Revision 10 (+721 -8)

Show changes

	usr/src/head/stdint.h
	usr/src/head/iso/stddef_iso.h
	usr/src/head/iso/stdlib_c11.h
	usr/src/head/iso/string_iso.h
	usr/src/lib/libc/amd64/Makefile
	usr/src/lib/libc/i386/Makefile.com
	usr/src/lib/libc/inc/libc.h
	usr/src/lib/libc/port/mapfile-vers
	13 more

usr/src/man/man3c/memset_s.3c (Diff revision 10)

Show all issues

Can we add the MT-Level to this manual?

usr/src/man/man3c/set_constraint_handler_s.3c (Diff revision 10)

Show all issues

Can we add the MT-Level to this manual?

Change Summary:

add mt-level to man pages

Commit:

-	5d89d91604d6a63660ec588278d9ce8224953776
+	df4956e657fd8036ac02942d0a2c051bf9234c61

Diff:

Revision 11 (+727 -10)

Show changes

	usr/src/head/stdint.h
	usr/src/head/iso/stddef_iso.h
	usr/src/head/iso/stdlib_c11.h
	usr/src/head/iso/string_iso.h
	usr/src/lib/libc/amd64/Makefile
	usr/src/lib/libc/i386/Makefile.com
	usr/src/lib/libc/inc/libc.h
	usr/src/lib/libc/port/mapfile-vers
	13 more

Ship it!

Ship It!

You have a pending review.

Review Board 2.5.16

Screenshots

Files

Issues

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Commit:

Diff:

Change Summary:

Description:

Testing Done:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Status: Closed (submitted)