7

I'm sure there are a lot of buffer overflow risks in coding, many of which are addressed by standard library's "_s" safe functions. Nonetheless, I find myself confused, from time to time, on some of them.

Let's say I have some fragment like this

uint8_t a[5];
...
size_t  z = 6;
...
memset(a, 0, z);  // Overflow!

Some compilers (C11) may advise better use of memset_s; as I'm a lousy programmer I just updated my code to this brand new thing, my way:

uint8_t  a[5];
...
rsize_t  max_array = 56;  // Slipped finger, head in the clouds, etc.
rsize_t  z = 6;
...
memset_s(a, max_array, 0, z);  // So what?

How would memset_s be any better than memset if I'm just adding the error to another parameter?

Where is the added safety, if adding a new parameter simply adds a new parameter that can be wrong. I could have corrected my code in the first revision, and still have a legal call to a well-defined operation on a buffer.

Left apart the case with an unchecked zero pointer, how could memset be that much worse than memset_s?

[EDIT]

With a little effort, I also found what setting was causing the warning in my setup. This may be of help for someone.

The warning comes from Clang-tidy, invoked in the Visual Studio extension "Clang power tools". In its default setup, it enables for Clang-tidy the checker "security.insecureAPI.DeprecatedOrUnsafeBufferHandling" which mimics the default MSVC warnings. Note that this isn't available from command line options (or at least is not listed among them), but read from a .clang-tidy file.
In the first runs, I didn't even have a .clang-tidy file, although that was enforced by default.

In my case, the reason for the message appearing in Clang-tidy and not with MSVC is straightforward.
I defined in my code

#define  _CRT_SECURE_NO_WARNINGS

to not be infested by MSVC warnings, but this method is ineffective with Clang-tidy, which kept nagging me (due to Clang power tools defaults)

LuC
  • 347
  • 2
  • 10
  • It's probably meant to be used like `memset_s(a, sizeof a, 0, z);` but I agree that it doesn't add much safety. – Ted Lyngmo Jul 28 '23 at 09:56
  • 3
    Only one compiler vendor pushes for those functions - MSVC, and it seems gnu libc doesn't support them at all. And you're [not alone in thinking they're useless](https://stackoverflow.com/a/50921096/2752075). This just another data point in support of MSVC being a weird compiler. – HolyBlackCat Jul 28 '23 at 09:58
  • @HolyBlackCat, actually it came from Visual Studio, but not from MSVC: only activating additional analysis with **Clang-tidy**. The analysis with MSVC alone doesn't notify it. – LuC Jul 28 '23 at 13:42
  • @LuC This check doesn't seem to be enabled in clang-tidy itself by default. Perhaps something that VS enables. – HolyBlackCat Jul 28 '23 at 17:20
  • @HolyBlackCat I agree that there's an option activated by VS. In fact, Clang-tidy invoked **manually** by the VS extension **doesn't** raise the warning (and that's how I use it normally), while the auto scanner produces the mentioned message. To be clear, I disabled the automatic Clang-tidy *messed* invocation, and I never thought to substitute my `memset`: my argumentation was hypothetical. – LuC Jul 31 '23 at 15:12
  • 1
    @LuC: excellent edit! Let's all fight this infestation :) – chqrlie Aug 04 '23 at 17:46
  • `_s` stands for "strange", meaning that the function is strange, non-portable and best to avoid all together. Maybe with the sole exeception of `strcpy_s` which is definitely preferable over abusing `strncpy`. Similarly, MS in the context of C programming stands for "Mighty Strange". – Lundin Aug 09 '23 at 14:23

1 Answers1

6

I would not advocate the use of any of the controversial secure functions from Annex K because of portability issues: the original implementation on Microsoft platforms does not conform to the Standard specification and many other platforms deliberately chose not to implement them at all.

Here are some features of memset_s that memset lacks:

  • null pointer check on the destination array.
  • partial sanity check on the block size: detecting sizes larger than RSIZE_MAX handles the case of erroneous sizes computed from data producing negative values. This alone should not have mandated a change of type from size_t to rsize_t, but does catch some programming errors, albeit at run time only.
  • memset_s() can not be elided: K.3.7.4.1: Unlike memset, any call to the memset_s function shall be evaluated strictly according to the rules of the abstract machine as described in (5.1.2.3). That is, any call to the memset_s function shall assume that the memory indicated by s and n may be accessible in the future and thus must contain the values indicated by c.

I agree with you that the first two features offer limited protection as both cases would produce segmentation faults on protected memory architectures anyway and the error handling scheme can hardly do better than exit the program in such cases too.

The third one is useful to ensure sensitive information contained in a local array is erased before the function returns preventing the compiler from optimizing out the code that clears the array. As memset_s support was kept optional, a new function memset_explicit was added with this exact purpose in C23 with the same prototype as memset. Similar functions were available under different names in some targets: explicit_memset (Oracle), or secure alternatives to clear an array: explicit_bzero (BSD), memzero_explicit (Linux) and SecureZeroMemory (Windows).

Size errors such as the one you exhibit will go undetected and cause hard to find bugs. They typically occur when the target array is not defined locally and its size is passed separately from the pointer, otherwise sizeof array is a safer choice by any means.

A typical error that bites even the most savvy programmers is this:

void handle_array(some_type *dest, size_t size) {
    /* clear the array */
    memset(dest, 0, size);  // BUG: using number of elements instead of byte size
    for (size_t i = 0; i < size; i++) {
        /* do other stuff... */
    }
}

The compiler cannot detect this programming error and using memset_s would not catch it either.

Another classic programming error is swapping the 0 and the size arguments.

I personally use macros to avoid these pitfalls:

#define array_clear(a, n)  memset(a, 0, sizeof(*(a)) * (n))

Adding tagged pointers to the language, that would combine the type, address and length of the object and would be constructed automatically from array arguments or assignments would help avoid many such problems. Such departures from the current definition of the C language that are not present in C++ either are alas very unlikely to ever make it into a next version of the C Standard.

chqrlie
  • 131,814
  • 10
  • 121
  • 189
  • If you use visual studio, you don't need that macro because you get to see the function signature as you input the arguments. – machine_1 Jul 28 '23 at 10:47
  • @chqrlie I totally agree with you, and my _question_ is maybe more a _request for comments_. The complaint promoting `memset_s` came from Visual Studio, but only indirectly: activating the additional analysis with Clang-tidy, which eventually, in this setup suggested the change. – LuC Jul 28 '23 at 13:39
  • @machine_1: a good IDE does help avoid some problems, but the confusion between number of elements and byte size is not completely removed by this feature, especially when the variables are poorly named such as in my example: `size` is more ambiguous than `count` or `element_count` or even `n`. – chqrlie Jul 28 '23 at 13:57
  • 1
    The best thing about this Q&A is that not even one single person voted to close it as "opionion based" which is kind of amazing. :-) Got my vote too! – Ted Lyngmo Jul 28 '23 at 18:45
  • @TedLyngmo: I tried to stay as neutral and factual as possible, which took some effort... I cannot believe clang-tidy actually suggested this. – chqrlie Jul 28 '23 at 19:06
  • @chqrlie I'm 100% behind your approach! – Ted Lyngmo Jul 28 '23 at 20:23
  • @TedLyngmo Really, I did start it with a pure feeling of ignorance. Only the **answers** I found here confirmed it to be an opinion, after all. Then I'm happy with how my question got answered and educated me. I think this is the correct concept of asking and being taught. – LuC Jul 31 '23 at 10:39
  • 1
    @LuC I understand and agree. This _could_ have been closed as opinion-based if people had just been a little more trigger happy that day, but I'm glad it didn't. – Ted Lyngmo Jul 31 '23 at 11:03
  • @chqrlie *Here are some features of `memset_s` that `memset` lacks* You left out the most important: [`memset_s()` can not be elided](https://port70.net/~nsz/c/c11/n1570.html#K.3.7.4.1p4): "Unlike memset, any call to the memset_s function shall be evaluated strictly according to the rules of the abstract machine as described in (5.1.2.3). That is, any call to the memset_s function shall assume that the memory indicated by s and n may be accessible in the future and thus must contain the values indicated by c." – Andrew Henle Aug 07 '23 at 17:02
  • @AndrewHenle: good point. I added a third item for this useful feature with an explanation for its purpose and the alternative solution standardized in C23. – chqrlie Aug 07 '23 at 22:28