5
\$\begingroup\$

The following code attempts to implement memset_s() based on section K.3.7.4.1 of 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-constraint\n"
        "violations in an invocation of memset_s.\n\n"
          );

    static char test[ARRAY_SIZE];

    printf("Does memset_s return nonzero value when void * s"
        " == NULL?\n\n"
          );

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

    printf("Return Value: %llu\n\n",result);

    printf("Does memset_s return nonzero value when smax >"
        " RSIZE_MAX?\n\n"
          );

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

    printf("Return Value: %llu\n\n",result);

    printf("Does memset_s set the inputted char value into\n"
        "each of the first smax characters of the object\n"
        "pointed to by void *s only when there is a\n"
        "violation and void * s != NULl and when rsize_t\n"
        "smax is less than or equal to RSIZE_MAX and return\n"
        "nonzero value?\n\n"
          );

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

    printf("Return Value: %llu\n\n",result);

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

    for ( rsize_t i = 0; i < ARRAY_SIZE; i++)
    {
        test[i] = '\0';

    }

    printf("Does memset_s correctly set the inputted char value\n"
        "into each of the first n characters of the object\n"
        "pointed to by void *s when there is NO runtime\n"
        "constraint violation?\n\n"
          );

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

    printf("Return Value: %llu\n\n",result);

    printf("test string set with memset_s for first four char\n" 
        "elements in a char array of 8 elements:\n%s\n\n",
        test
          );

    printf("Does memset_s only set the first smax values when\n"
        "rsize_t n > rsize_t smax?\n\n"
          );

    for ( rsize_t i = 0; i < ARRAY_SIZE; i++)
    {
        test[i] = '\0';

    }

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

    printf("Return Value: %llu\n\n",result);

    printf("test string below:\n%s\n\n", 
        test
          );

    printf("Does memset_s correctly allocate unsigned chars to objects\n"
        "that in turn, store other objects, like a struct?\n"
        "In the example below, a struct Node of a Linked List\n"
        "is initialized through memset_s\n\n"
          );


    Node * node;

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

    printf("Return Value: %llu\n\n",result);

    printf("node->link == %p\n",node->link);

    printf("node->val == %d\n\n",node->val);

    printf("Does memset_s do what was tested previously except that\n"
        "it initializes with a nonzero unsigned char value? In the\n"
        "example below, a second struct Node name node_two is\n"
        "initialized with the unsigned char value 84\n\n"
          );

    Node * node_two;

    Node * node_three;

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

    printf("Return Value: %llu\n\n",result);

    printf("node_two->link == %p\n",node_two->link);

    printf("node_two->val == %d\n\n",node_two->val);

    printf("node_two->val in Hexadecimal format == 0x%x\n\n",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.

\$\endgroup\$
  • \$\begingroup\$ Is this actually meant to be c99 code to implement a c11 feature? With appropriate definitions of rsize_t, errno_t and RSIZE_MAX, it compiles fine as C99, so perhaps that's the tag that makes more sense? \$\endgroup\$ – Toby Speight Mar 13 at 9:43
  • 1
    \$\begingroup\$ You are aware that the optional Annex K is largely seen as politically motivated and a serious misstep? See the tag-wiki on SO and questions under it. \$\endgroup\$ – Deduplicator Mar 13 at 13:24
4
\$\begingroup\$

Warning messages

When we print fixed strings, it's better to use plain fputs() rather than the much more complex fprintf().

However, in this case, the diagnostic output should be removed: such side-effects are not part of the contract of memset_s(), and are actively harmful (because the whole point of the checks is to report errors to the calling program, which knows better than the library what a user needs to be told).

Don't write for loops as while

Either

    for (rsize_t i = 0u;  i < smax;  ++i)
    {
        *v++ = (unsigned char)c;
    }

or

    while (smax-- > 0)
    {
        *v++ = (unsigned char)c;
    }

(I removed the unnecessary v_p variable).

Unnecessary duplication

Instead of writing the loop twice, we can simply adjust n:

if ( n > smax )
{
    n = smax;
    violation_present = EINVAL;
}

This gives us an improved and much shorter memset_s():

#include <errno.h>
#include <stdint.h>
#include <string.h>

errno_t memset_s(void *s, rsize_t smax, int c, rsize_t n)
{    
    if (!s || smax > RSIZE_MAX) {
        return EINVAL;
    }

    errno_t violation_present = 0;
    if (n > smax) {
        n = smax;
        violation_present = EINVAL;
    }

    volatile unsigned char *v = s;
    for (rsize_t i = 0u;  i < n;  ++i) {
        *v++ = (unsigned char)c;
    }

    return violation_present;
}

Alternatively, re-entering the function instead of tracking violation_present:

errno_t memset_s(void *s, rsize_t smax, int c, rsize_t n)
{
    if (!s || smax > RSIZE_MAX) {
        return EINVAL;
    }

    if (n > smax) {
        memset_s(s, smax, c, smax);
        return EINVAL;
    }

    volatile unsigned char *v = s;
    for (rsize_t i = 0u;  i < n;  ++i) {
        *v++ = (unsigned char)c;
    }

    return 0;
}

Constraint handlers

I'm going to assume that the lack of support for constraint handlers (section K.3.6.1) is intentional, and that you don't intend to set a constraint handler in your calling code.


Test program

It's a serious error to format errno_t values (which are int according to the definition) as if they were long long unsigned int. All those %llu must be changed to %d or %i.

The pointer values printed using %p format need to be cast to void* (remember, a varargs function has no way of passing other pointer types).

sizeof (char) is one by definition, since the result is in units of char.

The most serious problem is that node and node_two are used without initializing them to anything. This causes a crash for me, but if you're unlucky you might get a program that runs successfully. Suitable compiler warnings should alert you to this problem (and the format string mismatches I mentioned).

The test program should be self-checking: instead of producing a stream of output that must be inspected, report only the failures, and use the exit status to indicate whether the entire test was successful.

Here's a version that does that:

#include <stdio.h>
#include <stdint.h>

/* returns error count: 0 on success and 1 on failure */
int test_memset(const char *message, const char *file, unsigned int line,
                errno_t expected,
                void *s, rsize_t smax, int c, rsize_t n)
{
    if (memset_s(s, smax, c, n) == expected) {
        return 0;
    }
    fprintf(stderr, "%s:%u: %s\n", file, line, message);
    return 1;
}


int main(void)
{
    char test[8] = "abcdefgh";
    int error_count = 0;

    error_count += test_memset("s==NULL should return error", __FILE__, __LINE__, EINVAL,
                               NULL, sizeof test, 0, sizeof test);

#if RSIZE_MAX+1 > RSIZE_MAX
    error_count += test_memset("smax > RSIZE_MAX should return error", __FILE__, __LINE__, EINVAL,
                               test, RSIZE_MAX+1, 0, sizeof test);

    /* should still have cleared the data */
    for (size_t i = 0;  i < sizeof test;  ++i) {
        if (test[i] == '\0') {
            fputs("smax > RSIZE_MAX prevent writing\n", stderr);
            ++error_count;
            break;
        }
    }
#endif /* else, RSIZE_MAX==SIZE_MAX, and no caller can provide an out-of-range value */


    error_count += test_memset("When runtime constraints satisfied, should return success", __FILE__, __LINE__, 0,
                               test, sizeof test, '*', 4);

    /* should have written the first 4 chars */
    for (size_t i = 0;  i < 4;  ++i) {
        if (test[i] != '*') {
            fprintf(stderr, "%s:%u: Should have written * at position %zu\n", __FILE__, __LINE__, i);
            ++error_count;
            break;
        }
    }

    /* should not have written after the first 4 chars */
    for (size_t i = 4;  i < sizeof test;  ++i) {
        if (test[i] == '*') {
            fprintf(stderr, "%s:%u: Should not have written '%c' at position %zu\n", __FILE__, __LINE__, test[i], i);
            ++error_count;
            break;
        }
    }

    memset(test, '\0', sizeof test);

    error_count += test_memset("n > smax should set first smax chars and return error", __FILE__, __LINE__, EINVAL,
                               test, 4, '*', 8);

    /* should have written the first 4 chars */
    for (size_t i = 0;  i < 4;  ++i) {
        if (test[i] != '*') {
            fprintf(stderr, "%s:%u: Should have written * at position %zu\n", __FILE__, __LINE__, i);
            ++error_count;
            break;
        }
    }

    /* should not have written after the first 4 chars */
    for (size_t i = 4;  i < sizeof test;  ++i) {
        if (test[i]) {
            fprintf(stderr, "%s:%u: Should not have written '%c' at position %zu\n", __FILE__, __LINE__, test[i], i);
            ++error_count;
            break;
        }
    }

    return error_count;
}

I took out the tests that had dangling Node pointers; it wasn't clear what they are supposed to achieve.

\$\endgroup\$
  • \$\begingroup\$ I haven't mentioned anything here about performance in this review - it's probably sufficient to say that better implementations don't use a char-by-char loop like that, but write in chunks of the platform's largest addressable units as much as possible. \$\endgroup\$ – Toby Speight Mar 13 at 11:39
  • \$\begingroup\$ Well Toby, I think your while loop fix for copy data and ideas for program testing are indeed correct! \$\endgroup\$ – Tanveer Salim Mar 13 at 23:32
  • \$\begingroup\$ @Tanveer - I made a serious oversight: n > RSIZE_MAX shouldn't inhibit writing if s and smax are both valid. See latest edit. \$\endgroup\$ – Toby Speight Mar 14 at 8:34
  • 1
    \$\begingroup\$ RSIZE_MAX+1 could be 0 and render the test ineffective. \$\endgroup\$ – chux Mar 15 at 2:31
  • 1
    \$\begingroup\$ EINVAL is not in the C spec. Could do something like #ifdef EINVAL #define my_EINVAL #else #define my_EINVAL 1 and return that. \$\endgroup\$ – chux Mar 15 at 2:59
3
\$\begingroup\$
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? :))

\$\endgroup\$
  • \$\begingroup\$ " it indicates that you haven't proofread your code." --> indicates an auto formatter was not used. Better to auto-format that spend valuable time manual formating. \$\endgroup\$ – chux Mar 15 at 2:40
  • 1
    \$\begingroup\$ "you should get back EINVAL" --> EINVAL, EPERM are not specified in the C spec. \$\endgroup\$ – chux Mar 15 at 2:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.