Ytdyklly

The following code attempts to implement memset_s based on the ISO/IEC 9899:201x N1570 draft:

#include <stdio.h>

#include <string.h>

#include <stdint.h>



errno_t memset_s(void *s,rsize_t smax, int c, rsize_t n)

{

    errno_t violation_present = 0;  



    volatile unsigned char * v = s;



    if ( s == NULL )

    {

        fprintf(stderr,"memset_s: Error: void * s == NULL!n");



        return 1;

    }



    if ( n > RSIZE_MAX )

    {

        fprintf(stderr,"memset_s: Warning: rsize_t n > RSIZE_MAX!n");  



        violation_present = 1;

    }



    if ( n > smax )

    {

        fprintf(stderr,"memset_s: Warning: rsize_t n > rsize_t smax!n");   



        violation_present = 1;





    }



    if ( smax > RSIZE_MAX )

    {

        fprintf(stderr,"memset_s: Error: rsize_t smax > RSIZE_MAX!n");



        return 1;

    }



    volatile unsigned char * v_p = &v[0];



    rsize_t i = 0;



    if ( violation_present == 1 ) // && (s != NULL) && (smax <= RSIZE_MAX) )

    {



        i = 0;



        while ( i < smax )

        {

            *v_p++ = (unsigned char)c; 



            i++;

        }   







        return violation_present;



    }





    else // no runtime-constraint violation found       

    {

        i = 0;





        while ( i < n )

        {

            *v_p++ = (unsigned char)c;



            i++;

        }







        return violation_present;



    }





}

I have also made a C source test file for memset_s:

#include <stdio.h>

#include <string.h>

#include <stdint.h>



#define ARRAY_SIZE 8



typedef struct Node

{



    struct Node * link;



    int val;



} Node;





int main(void)

{

    errno_t result = 0; 



    printf("This program only checks for runtime-constraintn"

        "violations in an invocation of memset_s.nn"

          );



    static char test[ARRAY_SIZE];



    printf("Does memset_s return nonzero value when void * s"

        " == NULL?nn"

          );



    result = memset_s(NULL,sizeof(test),0,sizeof(test));



    printf("Return Value: %llunn",result);



    printf("Does memset_s return nonzero value when smax >"

        " RSIZE_MAX?nn"

          );



    result = memset_s(test,RSIZE_MAX+1,0,sizeof(test));



    printf("Return Value: %llunn",result);



    printf("Does memset_s set the inputted char value inton"

        "each of the first smax characters of the objectn"

        "pointed to by void *s only when there is an"

        "violation and void * s != NULl and when rsize_tn"

        "smax is less than or equal to RSIZE_MAX and returnn"

        "nonzero value?nn"

          );



    result = memset_s(test,8*sizeof(char),84,RSIZE_MAX+1);



    printf("Return Value: %llunn",result);



    printf("test string set with memset_s:n%sn",test);



    for ( rsize_t i = 0; i < ARRAY_SIZE; i++)

    {

        test[i] = '';



    }



    printf("Does memset_s correctly set the inputted char valuen"

        "into each of the first n characters of the objectn"

        "pointed to by void *s when there is NO runtimen"

        "constraint violation?nn"

          );



    result = memset_s(test,8*sizeof(char),84,4*sizeof(char));



    printf("Return Value: %llunn",result);



    printf("test string set with memset_s for first four charn" 

        "elements in a char array of 8 elements:n%snn",

        test

          );



    printf("Does memset_s only set the first smax values whenn"

        "rsize_t n > rsize_t smax?nn"

          );



    for ( rsize_t i = 0; i < ARRAY_SIZE; i++)

    {

        test[i] = '';



    }



    result = memset_s(test,8*sizeof(char),84,8*sizeof(char)+1);



    printf("Return Value: %llunn",result);



    printf("test string below:n%snn", 

        test

          );



    printf("Does memset_s correctly allocate unsigned chars to objectsn"

        "that in turn, store other objects, like a struct?n"

        "In the example below, a struct Node of a Linked Listn"

        "is initialized through memset_snn"

          );





    Node * node;



    result = memset_s(node,sizeof(Node),0,sizeof(Node));



    printf("Return Value: %llunn",result);



    printf("node->link == %pn",node->link);



    printf("node->val == %dnn",node->val);



    printf("Does memset_s do what was tested previously except thatn"

        "it initializes with a nonzero unsigned char value? In then"

        "example below, a second struct Node name node_two isn"

        "initialized with the unsigned char value 84nn"

          );



    Node * node_two;



    Node * node_three;



    result = memset_s(node_two,sizeof(Node),84,sizeof(Node));



    printf("Return Value: %llunn",result);



    printf("node_two->link == %pn",node_two->link);



    printf("node_two->val == %dnn",node_two->val);



    printf("node_two->val in Hexadecimal format == 0x%xnn",node_two->val);



    return 0;



}

My only concern is that I forgot to find a security flaw in my implementation of memset_s. Did I forget to test for any values?

I plan to use memset_s in my implementation of http.c from the book "Implementing SSL/TLS Using Cryptography and PKI" by Joshua Davies.

errno_t memset_s(void *s,rsize_t smax, int c, rsize_t n)

You've got a missing space after that first comma. (Sure, inconsistent whitespace doesn't affect functionality; but like any spelling mistake, it indicates that you haven't proofread your code. Which means it probably has some other one-character typos too. Some of which might affect functionality. Always read your code after writing it!)

The typedef errno_t is invariably int. So personally I'd just write int — unless you were planning to use the association with errno in some significant way. Right now your code just returns 0 on success and 1 (which is to say EPERM) on failure. Consider whether memset_s should return a more specific and helpful errno than simply EPERM. For example, maybe if you pass NULL to it, you should get back EINVAL "Invalid argument"?

However, at the same time, YAGNI — it may be counterproductive to spend a lot of time agonizing over what are the proper errno return values, unless you have an actual use-case for these return values. The only reason to return EINVAL for a null argument instead of the generic EPERM is that it enables a caller to detect that specific error case and handle the error accordingly. But the caller can already detect that case! The caller doesn't need your help to detect s == NULL! The caller controls the value of s in the first place, and can easily check for null beforehand, if they think it's possible for s to be null. (And if it's not possible for s to be null, then there's no point checking inside memset_s either. That's just a waste of CPU cycles.)

Can you tell that I think memset_s is largely a waste of time, design-wise? :)

I find your chain of ifs difficult to match up to the specification. The specification is as follows:

Runtime-constraints: s shall not be a null pointer. Neither smax nor n shall be greater than RSIZE_MAX. n shall not be greater than smax.

If there is a runtime-constraint violation, then if s is not a null pointer and smax is not greater than RSIZE_MAX, the memset_s function stores the value of c (converted to an unsigned char) into each of the first smax characters of the object pointed to by s.

Description: The memset_s function copies the value of c (converted to an unsigned char) into each of the first n characters of the object pointed to by s.

So I would naturally implement it something like this:

errno_t memset_s(void *s, rsize_t smax, int c, rsize_t n) {

    bool violation = (s == NULL) || (smax > RSIZE_MAX) || (n > RSIZE_MAX) || (n > smax);

    if (violation) {

        if ((s != NULL) && !(smax > RSIZE_MAX)) {

            for (rsize_t i = 0; i < smax; ++i) {

                ((volatile unsigned char*)s)[i] = c;

            }

        }

        return EPERM;

    } else {

        for (rsize_t i = 0; i < n; ++i) {

            ((volatile unsigned char*)s)[i] = c;

        }

        return 0;

    }

}

That seems to implement the specification 100% correctly, line for line.

You write the second for-loop above as

    i = 0;





    while ( i < n )

    {

        *v_p++ = (unsigned char)c;



        i++;

    }

(yes, with two blank lines between the i = 0 and the rest of the loop). That's definitely too much code for a simple for-loop. Even without doing anything else to your code, you could replace those 9 lines with 3 lines:

    for (int i = 0; i < n; ++i) {

        *v_p++ = (unsigned char)c;

    }

A 66% reduction in lines-of-code is not bad for a day's work!

volatile unsigned char * v = s;

// ...

volatile unsigned char * v_p = &v[0];

You know that &v[0] is the same thing as v, right?

rsize_t i = 0;



if ( violation_present == 1 ) // && (s != NULL) && (smax <= RSIZE_MAX) )

{



    i = 0;

Three things:

• You initialize i to 0, and then again initialize it to 0. Are you worried that the first initialization might not have taken? :)




• The commented-out code is confusing. (I left a comment on the question referring to this and some other commented-out code; you did remove some of it, but left this snippet commented out.) If you meant for this code to take effect, you should uncomment it. If you meant for this code not to take effect, you should just delete it. Don't leave commented-out code hanging around. (If it's for historical interest, you should learn git or some other version-control system.)




• You branch on if violation_present == 1. This suggests that violation_present might take on other values, such as 0 (which it does) or 2 or 42 (which it does not). The compiler will likely generate code to compare violation_present against the constant 1. But actually all you mean here is "If there's a violation present...", which is idiomatically expressed as if (violation_present) .... Furthermore, you should look up the bool type (defined in <stdbool.h>) — it's tailor-made for boolean variables that can only ever take on the value true or false. (Notice the use of bool in my reference implementation above.)

fprintf(stderr,"memset_s: Error: void * s == NULL!n");

Missing whitespace again.

Here you have a memset_s function, whose job is to set a range of bytes to a value... and you've got it pulling in fprintf from the standard library! Does that seem appropriate to you?

memset_s is a very low-level function. It should be usable even on embedded systems that have never heard of fprintf or stderr. You should find a way to report errors that doesn't involve <stdio.h>. (Might I suggest errno_t? :))