replace strncpy with strncpy_s

[jherico]

Use of strncpy generates warnings

[Ybalrid]

Isn't strncpy_s a "Microsoft only thing"?

I tend to consider this very warning bullying from their part to use their non-compatible non-standard API, and should be disregarded by defining the _CRT_SECURE_NO_WARNINGS toggle. And that's also pretty dumb considering that strncpy itself is already a "more secure" alternative to strcpy 🤷‍♂

[jherico]

Isn't strncpy_s a "Microsoft only thing"?

No. It's core C++. There are a bunch of MS specific functions dealing with string manipulation in various encodings and character widths, but for null-terminated C strings, strncpy_s and strncat_s are the functions that should be used and strncat and strncat are considered unsafe.

[Ybalrid]

I've gone down quite the rabbit hole, but TL;DR we should not use it, this function is not "C++ core" and is in practice not portable because major compiler vendor/stdlib implementer is against shipping it (for reasons).

---

I never actually checked, because string operations, if you have access to C++, really should not be made with theses functions anyway. (Just use std::string!)

So, I googled around quite a bit here's how I understand the situation : It's not core C++. It doesn't exist on GNU's glibc (aka, the whole Linux world?), it should not be used in cross platform code, or make a macro that only calls it when the compiler is MSVC. Funnily enough, VS2017 doesn't answer "yes" when you ask it if these functions are available

The details

• It's C11 (note: that's not C++, C++ is not a superset of C, they diverged quite a bit now) standard, yes. In Annex K. That is normative, but is considered "optional" and not implemented by at least one vendor (GNU)
• It's safer, yes... maybe? It can cause other problems by truncating data... And will not fix the actual underlying problem that is "programmers can be sloppy".
• The standard (the actual C++ one) mention this symbol existence only in one place as a reserved name a translation unit including headers from the C++ standard library should not use for any purpose, as it may already exist, (§17.5.1.2.10, in the ref. table 18 on this working draft of C++17; The actual standard document from iso is under a paywall, but working draft are "public" tho)

The actual problem here is that this function doesn't exist in glibc, here's what they have to say about it:

The strlcpy and strlcat functions have been promoted as a way of copying strings more safely, to avoid buffer overruns when retrofitting large bodies of existing code without understanding the code in detail. Annex K of the C11 standard defines optional functions strcpy_s and strcat_s that serve a similar need, albeit less efficiently and with different calling conventions. Unfortunately, in practice these functions can cause trouble, as their intended use encourages silent data truncation, adds complexity and inefficiency, and does not prevent all buffer overruns in the destinations. New standard library functions should reflect good existing practice, and since it is not clear that these functions are good practice they have been omitted from glibc.

The quotation is from this FAQ

As far as I know, that function was pushed (invented?) by Microsoft in their CRT. They started putting these "deprecated" messages in like, VS2005? maybe older... You can use this function in MSVC specific code, sure, but I don't think this exist in many compilers, at least not gcc or clang under Linux.

Just to check some of the odd behavior, Visual Studio's compiler doesn't advertise itself as being compatible with send extension, so, the thing you need to check as per the C11 standard Annex K doesn't even check good on MSVC. see in practice

I spent quite some time removing calls to this thing in any software using it while either porting it to Linux or un-breaking Linux source compatibility and always assumed that for this thing was Microsoft specific, and was always skeptical about any security benefits from "just using this handy non compatible replacement of that POSIX one" that MSVC users are strongly suggested to use by their compilers, and foolishly trust...

What I take form this is, Don't blindly trust your compiler's warnings, also don't trust any user inputs. Use better tools to assess the correctness of your low level buffer manipulation. And, IMHO just avoid doing it altogether while copying a string when std::string::operator=() exist.

[jherico]

Well researched. My take on it is that we can't include anything in the header that's going to generate warnings on a large subset of clients. Likewise we can't unilaterally alter the client's warning settings by defining _CRT_SECURE_NO_WARNINGS in the header.

I'm open to whatever solution is required to resolve the warnings that doesn't screw a bunch of users or diminish the functionality.

[Ybalrid]

Well, most of the API's not about copying string, that's only used a few times to fill in some struct members, during the initialization, so we pretty much don't care about any performance argument there.

Practical solution number 1 add an implementation of _s functions when they are missing:

• use strncpy_s (and other "_s" familly of functions) on platform where they exist (MSVC)
• on others, either substitue directly (via macro) to the non "_s" version, or (maybe simple) define a free function with that signature, that will call the so-called "unsafe" one, maybe with additional checks. edit: Well, a single line macro's not really doable, the _s version uses the return value as an error code with value 0 when everything works. the normal one just return a pointer to the string, so normal return is non-zero...

We also should define that additional strncpy_s function in our own namespace not to polute the global namespace by creating a function with that name.

There's a library called "safeclib" that implement them, it's open-source by Cisco. I don't know if we want to add a dependency to it. It seems that the license is permissive enough for us to borrow their implementation of said functions without any problem (do not quote me on that tho).

https://github.com/coruus/safeclib/blob/master/src/safeclib/strncpy_s.c#L100

I personally thing that's a bit counter productive to do so, as this increase complexity and bloat for what's just suppoed to be a header. I think we should limit ourselve

Practical solution number 2 (the most compatible one) only use the functions available everywhere:

To just define that _CRT_SECURE_NO_WARNINGS token, and then #undef it at the end in case it wasn't defined in the first place (but keep it in case the user had it in the first place). This is more of "the ugly hack" solution

//at top of header file
#ifndef _CRT_SECURE_NO_WARNINGS 
#define _CRT_SECURE_NO_WARNINGS 
#define XR_HPP_DO_UNDEF_CRT_SECURE_NO_WARNINGS
#endif

//code that call non-secure version of C functions

//At bottom of header file
#ifdef XR_HPP_DO_UNDEF_CRT_SECURE_NO_WARNINGS
#undef _CRT_SECURE_NO_WARNINGS
//Just to cleanup after our mess, remove that define from preprocessor
#undef XR_HPP_DO_UNDFEF_CRT_SECURE_NO_WARNINGS
#endif

Practical soltuion number 3 (the middle ground) wrap the strncpy function in a way that will invoke either strncpy_s or strncpy depending on platform

Something like that (it's 2 AM and I'm too lazy to test it, but that should work just fine)

errno_t copy_string(char* dst, size_t dstsz, const char* src, size_t count)
{
#if  defined(_MSC_VER) || defined(__STDC_LIB_EXT1__)
    return strncpy_s(dst, dstsz, src, count);
#else 
    strncpy(dst, src, std::min(count, dstsz));
    if(count < dstsz)
    {
        return 0;
    }
    else
    {
        return 80; //STRUNCATE error code in errno.h from windows
     }
#endif
}

I think solution 2 is reasonable, solution 3 is probably cleaner and you may prefer to do that. Solution 1 should be avoided.