8548 want memset_s(3C)

Review Request #640 — Created Aug. 6, 2017 and submitted

yuripv
illumos-gate
master
8548
df4956e...
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() 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)
  • 0
  • 0
  • 19
  • 3
  • 22
Description From Last Updated
igork
  1. Ship It!
  2. 
      
rm
  1. Shouldn't there also be manual page updates for these?

    1. Will do.

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

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

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

    1. Do you have any proposals here?

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

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

  3. 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).
    1. I'll check the standard (again) and will fix this.

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

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

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

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

  5. 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.
    1. 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.

    2. Done, checks removed.

  6. 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().
    1. I'll check the standard and will fix.

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

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

    4. Done.

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

    1. OK, will check the standard and fix.

    2. Done.

  8. 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.
    1. 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 :-)

    2. Dropped the changes.

  9. 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.
    1. I'll fix all that once we are good with the code itself.

    2. Dropped the changes.

  10. usr/src/uts/common/sys/feature_tests.h (Diff revision 1)
     
     
    Should extensions turn this on by default?
    1. 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 :-)

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

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

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

  11. 
      
yuripv
seeemef@mac.com
  1. 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)?
    1. 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.

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

  2. 
      
yuripv
yuripv
yuripv
seeemef@mac.com
  1. Looks good to me.
  2. 
      
yuripv
yuripv
rm
  1. Thanks for all of the work here. I think this is pretty close and have a few additional follow ups.

  2. 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?
  3. 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.
  4. 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.

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

    2. I somehow missed that in the incremental. My mistake.
  5. 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?
    1. I meant it as a placeholder -- if it or the entire block doesn't make sense, I'll remove it.

  6. usr/src/man/man3c/memset_s.3c (Diff revision 7)
     
     
    Missing the word 'the' before .Fn
  7. Missing return type for the constraint_handler_t here.
  8. We should state what it is in our implementation.
  9. We should state that it will cause it to abort.
    1. Simply removed that sentence, abort_handler_s() describes itself as default below.

  10. s/implementation/C library?

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

  12. 
      
yuripv
rm
  1. Ship It!
  2. 
      
seeemef@mac.com
  1. 
      
  2. 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.
    1. Well, there's still no user input involved, and it's only about the head, so let's leave it as it is.

  3. 
      
kmays
  1. Ship It!
  2. 
      
yuripv
yuripv
rm
  1. 
      
  2. usr/src/man/man3c/memset_s.3c (Diff revision 10)
     
     
    Can we add the MT-Level to this manual?
  3. Can we add the MT-Level to this manual?
  4. 
      
yuripv
rm
  1. Ship It!
  2. 
      
yuripv
Review request changed

Status: Closed (submitted)

Loading...