-1
\$\begingroup\$

As a spare-time personal project, I'm implementing a set of symmetric-key cryptographic primitives.

One thing I've left vacant for a long time, is the key/state eraser, which I decide to add right now.

I intend it to be a macro, so that it doesn't require any additional object file; the macro should be statement-like (as per this question), so that its usage is consistent with those from other lines.

Here's my current code:

#define ERASE_STATES(buf, len)                  \
    do {                                        \
        for(uintptr_t i=0; i<len; i++)          \
            ((char *)buf)[i] = 0;               \
    } while(0)

There's a few things I'm not entirely confident about.

  1. len is a cardinal and it'd probably be a size_t, but i is an ordinal, should I also use size_t, or change it to some other type for semantic consistency? (I'm using uintptr_t right now.)

  2. The signedness of the type char is explicitly undefined according to standard, but could it nontheless be used without sacrificing semantic consistency, as a generic type representing a byte when assigning non-negative values to it?

  3. Anything else I've missed?

Update

I'm not using memset because I designed my code to be compilable in free-standing environment.

\$\endgroup\$
  • \$\begingroup\$ Doing things you know are undefined behaviour and expecting defined behaviour isn't going to work. What is your concern in this regard? Have you tested the current code in multiple situations and does it appear to function correctly? \$\endgroup\$ – Mast Oct 14 at 6:28
  • \$\begingroup\$ @Mast I assume you mean my Q2, as it's the only Q with the word "undefined" in it. My concern is that (assuming I've casted type correctly) the content of buf would end up with non-zero bits, or observable gap didn't get re-initialized. \$\endgroup\$ – DannyNiu Oct 14 at 6:32
  • \$\begingroup\$ @Mast, I've tested the code on my 64-bit macOS Catalina, but I don't currently have a big-endian or 32-bit environment available to me right now. \$\endgroup\$ – DannyNiu Oct 14 at 6:40
  • \$\begingroup\$ What's with the loop? A plain old braced block is entirely valid C. \$\endgroup\$ – Reinderien Oct 14 at 18:21
  • 1
    \$\begingroup\$ @Reinderien As mentioned in the OP, "per this question". \$\endgroup\$ – DannyNiu Oct 15 at 4:27
2
\$\begingroup\$

There is not much code so I won't write much review

The issue that makes me most uneasy is the interface decision, specifically requiring calling code to calculate len in bytes.

If I am in "trying my best mode" as a coder, late at night with a stressful customer deadline hanging over me, it'll be all I can manage to remember to clear memory. I will inevitably try to clear an int array of 8 elements with ERASE_STATE(arr, 8). If I am lucky I will remember to use 32 and add a comment about the 4x8 calculation. I am unlikely to remember to account for the fact that other architectures could have a 16 or 64 bit int.

I would very much prefer a macro that took care of the type sizes for me and left the interface as passing the the natural array size.

\$\endgroup\$
1
\$\begingroup\$

Q1

uintptr_t is used for representing pointers numerically, so it's not suitable even if the underlying representation is identical to that of size_t, the same can be said for ptrdiff_t.

On the other hand, it's common knowledge that the set of finite ordinals is isomorphic to the set of finite cardinals, so size_t can be your friend here. What's more, the loop condition may be improved by adding a cast to size_t to len to ensure the loop always ends even when the original type of len has greater width than size_t although one should always ensure len has a sane value. Also, the cast would be necessarily present had you actually implement ERASE_STATES as a function.

So use size_t.

Q2

If you're really concerned that assigning the literal 0 to a char would get you a "negative zero" in one's complement or signed magnitude representation, then use unsigned char (which is just the same intermediate type used when the standard defined memset). Otherwise, the a world proliferous of two's complement signed integers, a plain char offers better clarity when the value is non-negative.

Also, there's guarantee that char[] is gap-free (unlike uint8_t).

Q3

Since it's a macro and not a function, buf and len will be evaluated multiple times during the loop. This limits its usage to lvalues and constant expressions.

Unless you assign arguments to temporary variables.

Improved Code

#define ERASE_STATES(buf, len)          \
    do {                                \
        char *ba = (void *)buf;         \
        size_t l = (size_t)len;         \
        for(size_t i=0; i<l; i++)       \
            ba[i] = 0;                  \
    } while(0)
\$\endgroup\$
  • 1
    \$\begingroup\$ I don't know the answer to this. In the improved code, is the compiler guaranteed to spot that the cast of len to size_t only has to happen once? Or will this spend half its time casting for the check? \$\endgroup\$ – Josiah Oct 14 at 7:17
  • \$\begingroup\$ @Josiah, good point, changed answer. \$\endgroup\$ – DannyNiu Oct 14 at 7:25
1
\$\begingroup\$

There's not much to review here, since it's only a single macro, but there's still room for improvement.

Don't use a macro

I intend it to be a macro, so that it doesn't require any additional object file

If that's the motivation for using a macro, it's based on a misconception. You can get that and avoid the problems of macros, by defining a static function instead. Consider this code:

#include <stdint.h>

#define ERASE_STATES(buf, len)                  \
    do {                                        \
        for(uintptr_t i=0; i<len; i++)          \
            ((char *)buf)[i] = 0;               \
    } while(0)

unsigned buffer1[11];
unsigned buffer2[13];

static void memclr(void *buf, unsigned len) {
    for (char *b = buf ;len; --len, ++b) {
        *b = 0;
    }
}

void foo1() {
    ERASE_STATES(buffer1, sizeof(buffer1)*sizeof(*buffer1));
}

void foo2() {
    ERASE_STATES(buffer2, sizeof(buffer2)*sizeof(*buffer2));
}

void bar1() {
    memclr(buffer1, sizeof(buffer1)*sizeof(*buffer1));
}

void bar2() {
    memclr(buffer2, sizeof(buffer2)*sizeof(*buffer2));
}

int main() {
    foo1();
    foo2();
    bar1();
    bar2();
}

Does a modern compiler generate different code for foo1 versus bar1? No. It generates the exact same code without requiring any additional libraries or translation units, and without any runtime overhead for function calls. It also does not sacrifice type safety and is easier to debug and maintain.

\$\endgroup\$
  • \$\begingroup\$ Isn't it against convention to define functions in header files? Also, what about those "unused static function" warnings in object files that don't use it? \$\endgroup\$ – DannyNiu Oct 20 at 2:12
  • \$\begingroup\$ Don’t put a static function in a header file. If it is needed in multiple C files, duplicate it in multiple C files. \$\endgroup\$ – Edward Oct 20 at 3:09

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.