VC++: Safe Libraries with Martyn Lovell

You may want to change the name of the library, SecureCRT is the name of a very well known SSH client.

The title of this thread should really say 'Safe Libraries'. Sorry about that, there was confusion about the naming and an 'internal codename' got used for the thread.

Safe Libraries (Safe CRT, Safe ATL, Safe MFC) is the general initiative for all this stuff.

Martyn

martynl wrote:

The title of this thread should really say 'Safe Libraries'. Sorry about that, there was confusion about the naming and an 'internal codename' got used for the thread.

Safe Libraries (Safe CRT, Safe ATL, Safe MFC) is the general initiative for all this stuff.

Martyn

Hi, I have one question on Minimize CRT Use in ATL, that's how many CRT functions are 'compatible' with ATL? because sometimes got a link error on MainCRTStartup when I was remarked the Minimize CRT option as Yes, and I don't know what functions I used that are not 'compatible' with ATL, you know, a VC++ link error with the meaningless error message is hardly go to find where the problem is.

errno_t strcpy_s(
char *strDestination,
size_t sizeInBytes,
const char *strSource
);
I think the sizeInBytes and _countof() is unnecessary, because we  already have sizeof operator, that also means strcpy_s is unnecessary too.

note that _countof and sizeof are different things. if you have a static array somewhere like char test[10] you could have _countof defined to sizeof(test) / sizeof(test[0]).
personally i think it's at least questionable to use compiler specific stuff like _countof (and especially the native for each). that's something i don't encourage to use. if there's a consensus amongst all the major compiler vendors to support these custom compiler macros then this might be a different thing. otherwise you're just locked into a specific (compiler) platform.

one thing i liked about this interview was that martyn didn't downplay the importance of native code. i would even argue that the vast majority of code in existance is native c/c++. i also think there are quite valid reasons to write (at least the most resource critical parts) in native c++. i also like the effort to have a verifiable stl implementation.. on the other hand.. stl is nice, but without boost it is not so nice. unfortunately there's very little interest in the boost community to make parts of the boost library verifiable - apart from the problems that arise from the clr type system that is inherently incompatible with the boost way of doing things. i need boost. so for me it's still writing the resource heavy parts in native c++ as i would previously have done and then compile the thing with /clr. That's nothing microsoft can be blamed for, i know, it's just underlining the fact that people still love unmanaged c++ and are very productive (and secure) with it.

good video.
- Martin

Ion,

That's a great question. We worked really hard to make the cost of these things at runtime in retail code very low. We've enabled them for our large codebases (VS, CLR, Office, SQL) without seeing major perf regressions.

Generally, the overhead of checking and safety can be amortised so that it costs a lot less than doing the actual work of the function. You have to focus on calling a function lots of times to detect the cost of checking in general.

For the Standard C++ Libraries, we put checking into iterators, which can have some significant perf cost in some cases (because iterators get used so often in a loop). But we made smart changes to our algorithms so that generally these costs are again amortised to once for the whole loop. As a result, std::for_each will be faster than for.

We're working to further improve performance for future versions, but working with customers since our product released we haven't found a large number of perf problems, and have mostly been able to fix them with small tweaks.

If you find a specific perf problem with our changes, please do get in touch with us.

Martyn

Leigh,

I wish I could give you a better answer to this question, but we don't have a good list of which ATL functions work right with ATL_MIN_CRT. The original intention of the ATL_MIN_CRT feature was that you could only use a 'minimal' subset of ATL that had no CRT dependencies, but this term 'minimal' was not strictly defined. As a result, as ATL has evolved, we've occasionally accidentally broken people with changes we've made. If you report these, we'll  try to fix them.

Additionally, ATL has picked up new unavoidable CRT dependencies in some cases, where you now need the CRT.

If you have specific cases you're seeing and need help with, do let me know

Personally, I generally recommend against the use of ATL_MIN_CRT. It's still appropriate for a small number of situations where you're still dealing with very small redists, but for lots of cases it's an easier develop with a CRT DLL dependency. What's making you choose ATL_MIN_CRT?

Martyn

The first thing to say about _countof is that in C it has a simple definition ( sizeof(a)/sizeof(a[0]) ). But in C++ code it has a template magic definition which detects errors like

char *p;
_countof(p)

which are a common source of buffer overruns in real code.

From my perspective, this is enough reason to never use sizeof on arrays again. I've seen too many real issues result from it.

If you build portable code, it's pretty easy to provide a trivial definition of _countof for other platforms. Even the template magic is portable, so you can do that too. There's no compiler magic here - you can check out the definition of _countof in the CRT headers.

Martyn

In the presentation you mentioned the fact the complier switch in VS2005 you could convert unsafe method calls to the newer safe ones and you also give an example of using current complier settings and setting the size of dest by calling size of 'dest'.
I have heard a number of views about the "_s" extentions to the API (not ANSI C+++ complaint etc) and the complaints about the warnings the complier gives you about depricated CRT API calls (the method calls are not depriated accrdnig to the standards). So why could you have not under the covers converted the users code to the newer methods without a complier switch or converted the complier to check method calls to check the size of the destination buffer being used and then provide a user high level warning?

I have a question (I haven't yet watched the video so maybe it's answered there, but here goes anyway).

The C++ standard specifies that std::vector::operator[] does not do bounds checking. Instead if you want bounds checking, you should use std::vector::at().

However, using the new secure libraries, operator[] will do bounds checking (unlike at, it will not throw an exception but instead fail an assert check, which in a release build means the app will crash immediately rather than allow heap corruption), unless you specifically "#define _SECURE_SCL 0"

In my experiments, leaving _SECURE_SCL at the default value (thus using bounds checking) will cause vector to perform up to three times slower than without the bounds checking. Some tests have shown that in this case, vector is actually much slower than .Net's System.Array, which also does bounds checking.

To me, this seems a little extreme for a simple bounds check. What is the cause of (and justification for) this extreme performance difference?

Martyn:  You need a good reason to set up five monitors?  Next time you happen to have 'em all set up on one system, launch ssstars.scr .

My multimon sys: http://www.realtimesoft.com/multimon/gallery_browse.asp?ID=601
http://www.flickr.com/photos/damnednice/tags/computer/

Hi Cairn,

Good questions. Let me take the overloads point first.

First, the 'automatic conversion feature'. The goal here is to reduce the cost for teams that want to migrate from existing risky code to safer code using our new functions. If you have code like this

[all these code samples are typed live, so please excuse any errors]

void foo(char *d, char *p)
{
   // stuff

   strcpy(d,p);

   // ... stuff
}

then this is obviously risky. Our recommended mitigation changes the function to

void foo(char *d, size_t ds, char *p)
{
   // stuff

   strcpy_s(d, ds, p);

   // ... stuff
}

There's no way to do this kind of transformation automatically - the function calling convention had to change too.

But for other kinds of function

void foo(char *p)
{
   char d[200];
   // stuff

   strcpy(d,p);

   // ... stuff
}

the mitigation is pretty trivial

void foo(char *p)
{
   char d[200];
   // stuff

   strcpy(d, _countof(d), p);

   // ... stuff
}

We noticed that if you are a C++ template programmer, you can write a 'clever' overload of strcpy that detects the use of a static array, and automatically works out the _countof the array and then calls strcpy_s. Defining

_CRT_SECURE_CPP_OVERLOAD_STANDARD_NAMES

in C++ code will engage this mapping. You can do similar mappings for functions taking counts (strncpy) with

_CRT_SECURE_CPP_OVERLOAD_STANDARD_NAMES_COUNT

Obviously, both these extensions provide template overloads of standard functions, and so aren't strictly conformant to the standard. So we turn them off by default. But you can turn them on when if you want them. We foudn that the significantly reduced the effort to secure existing code.

There's no general way for us to correctly determine at compile time the intended buffer size of a buffer passed to strcpy, so we can't apply these techniques all the time. But when C++ is used, we can do some cases.

You also mentioned the issue of the warnings using the word "deprecated", even though these functions are not deprecated in the relevant standard. We never intended this to imply deprecation in the standard sense, but it's caused a lot of concern and confusion, so in service pack 1 of our product we've removed the word 'deprecate' from the warnings associated with this, and replaced it with a text message that explains in more detail what we're doing here.

Although the _s functions are not part of the C standard, we have worked very closely with the C committee on their design for the past 3 years, and a technical report of the C committee that includes many of them is under voting right now. I also know that a couple of other library implementers have portable implementations of these functions, so they should start turning up on other platforms. We wanted to try to help secure C code across the whole development community (not just Windows), which is why we worked so closely with the C committee on this issue.

If you get a chance, take a  look at the latest C committee document and let me (or the editor) know what you think:
http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1146.pdf

Martyn

Sven,

Thanks for following up.

First, my reading of the standard is that the function at is required to do bounds checking, but operator at is also allowed to, because the behaviour of operator [] is said to be undefined outside of the valid ranges for the container. If I'm missing a standard paragraph, perhaps you can point me to it?

Second, on behaviour when invalid situations occur.

By default, when we find ourselves about to have an invalid condition, we call the operating system and ask it to collect a crash dump (this puts up the 'send error report' dialog or brings up a debugger). This can look like a "crash", but it is in fact a deliberate call to a specific OS API with known parameters designed to collect information about the fault. If you are an ISV, you can even sign up with Microsoft to get these crash dumps for your application, I believe.

We recommend this approach because we generally think it's unsafe to continue running code in a process once that process is in an unknown state - lots of security exploits use such lax-ness to their advantage.

However, we also know that developers have different preferences and opinions on what should happen. So you can use _set_invalid_parameter_handler to override the behaviour to cause something different to happen. And in the SCL, we added another option. If you'd like to get a SCL range exception when you hit this kind of condition, you can just define _SECURE_SCL_THROWS and then any of these range conditions will throw an exception.

Your final point was about performance. I encourage you to post (or send me) your test code. Our experience was that most code recompiled with very little performance loss, and cases where there was perf loss were pretty easy to resolve. Because we put checking in every iterator operation, it's definitely possible to build 'worst-case' tiny examples where the cost looks very high. But for real world code I've worked with, the cost has mostly been pretty small, and certainly worth the improved safety. Of course, we provide _SECURE_SCL=0 for those cases where you really need all the perf and can do the error-checking yourself up front. But in our internal code we've rarely needed to use it.

A key technique for making things perform well is to make sure you use std:: algorithm's to manipulate your container, since all of these have been perf optimised. If you write a manual for loop, the iterator has to check at each iteration. We are working on further optimisations for these cases in future versions, so any information you can give us about problems you're hitting would be very useful.

I'd be glad to work with you to understand your perf situation in more detail. Similarly, if the error handling customisaion described above doesn't meet your needs, I'd be interested to understand that more.

Thanks for the feedback, we appreciate it.

Martyn

Slackmaster,

Neat monitors I've never had more than 2 at once on one machine - both for desk space and angle reasons. Plus, my display card budget is limited... I'm looking forward to the improved multimon support in Remote Desktop in Windows Vista.

Martyn

martynl wrote:

Leigh,

I wish I could give you a better answer to this question, but we don't have a good list of which ATL functions work right with ATL_MIN_CRT. The original intention of the ATL_MIN_CRT feature was that you could only use a 'minimal' subset of ATL that had no CRT dependencies, but this term 'minimal' was not strictly defined. As a result, as ATL has evolved, we've occasionally accidentally broken people with changes we've made. If you report these, we'll  try to fix them.

Additionally, ATL has picked up new unavoidable CRT dependencies in some cases, where you now need the CRT.

If you have specific cases you're seeing and need help with, do let me know

Personally, I generally recommend against the use of ATL_MIN_CRT. It's still appropriate for a small number of situations where you're still dealing with very small redists, but for lots of cases it's an easier develop with a CRT DLL dependency. What's making you choose ATL_MIN_CRT?

Martyn

Thanks, MS seems evil, but their employees have always been care about their customers.

"What's making me choose ATL_MIN_CRT?", I was received a runtime error on win98, it’s not the bug of msvcrt.dll, but an incompatible version problem, only 6.10.8637.0 version msvcrt.dll does work (win98 has an older one, but I have tried to replace with a higher 7.0.2600.2180 version, still doesn’t work), that makes me realized its better without any depends. Of course, it’s not the main reason, you know, C++ developers like to make their code as fast as possible, as mini as they can, and as robust as rocket, so if there are only one or three 'incompatible' CRT functions we used in our code, then we may consider about it whether worth depend a ‘huge’ dll or not.

Here I have a problem on sizeof. the following foo shows szDen[] and strDen[] both are the same array, why sz != sz1? And what’s the difference between strcpy_s and strncpy and memcpy?   

char* strcopy(char strDen[]/*char* strDen*/, const char* strSou)
{
     size_t sz1 = sizeof(strDen);  //sz1 always be 4?
     _ASSERT(szDen && sizeof(strDen) >= strlen(strSou));
     char* p = NULL;
     for(p = strDen;*strDen++ = *strSou++; );
     return p;
}

int _tmain(int argc, _TCHAR* argv[])
{
     char szDen[6];
     size_t sz = sizeof(szDen); //sz == 6
     memset(szDen, 0, sizeof(szDen))
     strcopy(szDen, "hello");
     return 0;
}

leighsword wrote:

Thanks, MS seems evil, but their employees have always been care about their customers.

Martyn, I think that's a compliment.

leighsword wrote:

Here I have a problem on sizeof. the following foo shows szDen[] and strDen[] both are the same array, why sz != sz1?

sz is the size of the character array you've declared on the stack. sz1 is the size of the char* you've passed to the function. You've declared it as char[], but the compiler really passes a char*, not the full array. Someone (Sven?) can probably put that in C++ specification-compliant terms.

leighsword wrote:

And what’s the difference between strcpy_s and strncpy and memcpy?

It's probably better to compare strncpy_s to strncpy since they do the same thing. (strcpy_s copies a whole string, strncpy copies N characters.)

strncpy_s returns error information where strncpy does not. It's difficult to write "good" code with strncpy (where "good" means checking for errors). strncpy does not return an error code because it doesn't know the size of the output buffer.

EDIT: Oh, and memcpy is ignorant of types and their sizes. It copies bytes, not characters. With it you can do things like:

    int i;

    memcpy(&i, "Hello, world", sizeof(i));

Which you just about never want to do. If you know you're copying strings, use the string functions--the compiler can do more type checking for you.

leighsword wrote:

char* strcopy(char strDen[]/*char* strDen*/, const char* strSou)
{
     size_t sz1 = sizeof(strDen);  //sz1 always be 4?
     _ASSERT(szDen && sizeof(strDen) >= strlen(strSou));
     char* p = NULL;
     for(p = strDen;*strDen++ = *strSou++; );
     return p;
}

int _tmain(int argc, _TCHAR* argv[])
{
     char szDen[6];
     size_t sz = sizeof(szDen); //sz == 6
     memset(szDen, 0, sizeof(szDen))
     strcopy(szDen, "hello");
     return 0;
}

leighsword wrote:
errno_t strcpy_s(
char *strDestination,
size_t sizeInBytes,
const char *strSource
);
I think the sizeInBytes and _countof() is unnecessary, because we  already have sizeof operator, that also means strcpy_s is unnecessary too.
--------------------------------------------------------------

It is so unfortunate to read someone write such nonsense so bluntly and with so much confidence when just a few messages later that very same person writes something like:

leighsword wrote:
"Here I have a problem on sizeof. the following foo shows szDen[] and strDen[] both are the same array, why sz != sz1? And what’s the difference between strcpy_s and strncpy and memcpy?"
--------------------------------------------------------------

Kind of reminds me when some famous girl said once:
"I know it's Tuna.. but is it a chicken?"

Please.. please.. back your comments with knowledge.

Great interview! I think that C++ will be around for quite some time due to performance "issues" with .NET. I don't mean in business apps but applications like games and apps that use alot of system resources.

When computers are fast enough that the performance hit of managed code is neglectable (is that the word I'm looking for? ) probably most development will change to c# or whatever is available then.

martynl wrote:

First, my reading of the standard is that the function at is required to do bounds checking, but operator at is also allowed to, because the behaviour of operator [] is said to be undefined outside of the valid ranges for the container. If I'm missing a standard paragraph, perhaps you can point me to it?

That's not what I was saying really. I can't find any indication of what operator[] is supposed to do in an out-of-bounds situation except that the explicit mention of bounds-checking for at() implies that operator[] does not do so. I wasn't disagreeing with what you chose to do per se, I simply pointed out the performance implications of such. Which, now that I've seen the video, I know you've also considered (not that I expected otherwise ).

martynl wrote:

We recommend this approach because we generally think it's unsafe to continue running code in a process once that process is in an unknown state - lots of security exploits use such lax-ness to their advantage.

Agreed 100%. I'd prefer a good crash over heap corruption with unknown side-effects any day.

martynl wrote:

Your final point was about performance. I encourage you to post (or send me) your test code. Our experience was that most code recompiled with very little performance loss, and cases where there was perf loss were pretty easy to resolve. Because we put checking in every iterator operation, it's definitely possible to build 'worst-case' tiny examples where the cost looks very high. But for real world code I've worked with, the cost has mostly been pretty small, and certainly worth the improved safety. Of course, we provide _SECURE_SCL=0 for those cases where you really need all the perf and can do the error-checking yourself up front. But in our internal code we've rarely needed to use it.

The code in question is from this thread, where CannotResolveSymbol has written a simple test case for .Net vs. JVM performance using bubblesort. I wanted to add a native version to the mix, so I wrote a version of the algorithm using vector, which ended up being much slower than any of the others (if you compare the table in the post I linked to, the vector version had an average sort time of 700ms (the native version I ended up using in that table uses a simple int array, not an STL container).
This is the code that uses the vector. Both the iterator and the (commented out) operator[] version of the bubblesort inner loop have the same problem: if you leave the #define _SECURE_SCL 0 commented at the top, it's slower than any other variant of the test.

I realize that this isn't real-world code at all, even if I'd need to manually implement a sort algorithm I'd use quicksort, not bubble sort, and usually I'd use std::sort from <algorithm>.

It is however still interesting that .Net's System.Array (and even java) actually outperforms the std::vector in this case, even though it too employs range checking. Now I'm not sure how the range checking for System.Array is done, whether it's the class itself (similar to vector) or the runtime that does the check. If the latter the JIT compiler might be deciding to optimize away the range checks because the loop bounds are within the array bounds. I'm not sure.

Hi Leigh,

You don't mention which version of VC or ATL you're using, but it sounds like you are somehow trying to target msvcrt.dll without using VC6. The only way to target msvcrt.dll is using VC6 (which is now out of support, and I strongly recommend against using).

You should never copy a version of msvcrt.dll from a one version of the OS (in your example, Windows XP) to another. These OS-distributed msvcrt.dll are tightly bound to the OS in question, and must never be redistributed. They are also not licensed for redistribution, so you would be breaking your license if you did so.

I think you'll find the value you get from msvcr80 (in terms of improved security and robustness) is worth the cost of redisting this DLL in all but the most bandwidth-constrained situations. I don't think it should stop you getting rocket-level performance

> Here I have a problem on sizeof. the following foo shows szDen[]
> and strDen[] both are the same array, why sz != sz1?

Your question about sizeof illustrates one of the reasons we prefer _countof for array usage.

> char* strcopy(char strDen[]/*char* strDen*/, const char* strSou)
> {
>      size_t sz1 = sizeof(strDen);  //sz1 always be 4?

sizeof is a compile time language construct, so it can only tell you about the type information the language knows about during compilation. In the example you gave, strDen is a character array of unknown size, which in C is very similar to a char *. Your strcopy function can be called with any length of array, and C language semantics say that this kind of array doesn't come along with its size. So all that sizeof can tell you is the size of the pointer in question (which is obviously 4 on the CPU architecture you're targetting).

Remember that sizeof can ultimately only tell you as much about a variable as the compiler knows during compilation. If you need more information than that, you'll have to add it as a parameter to the function.

> And what’s the difference between strcpy_s and strncpy and
> memcpy?

memcpy: copies specified number of bytes without regard for their value. Just copies arbitrary data. Generally should not be used to copy strings.

strncpy: copies up to 'n' characters from a NUL terminated string. If it finds a NUL in the source, it fills the rest of the 'n' with NUL. If it does not find a NUL, it fails to NUL-terminate the destination. Very hard to use this function safely and correctly. Theoretically good for truncation. In practice, strncpy_s with _TRUNCATE is much easier to use and known safe.

strcpy_s: copies from source to a destination whose size is given. Attempts to copy whole of source. If whole of source can't be copied, empties contents of destination (NUL in position 0), and returns error/calls invalid parameter handler. Use strcpy_s when you know for sure that destination is big enough for source. If you don't know for sure, use strncpy_s with _TRUNCATE.

Martyn

Sven,

Your bubblesort example is very interesting. It illustrates exactly the challenges with having checked iterators and using general algorithms.

Essentially, the checking inside the iterator can fire 'too often' in situations where algorithms work on understood bounds. Checking all boundaries once in a loop is overkill if you know that many initial conditions of the algorithm do not change.

Turning off _SECURE_SCL isn't the right solution to this problem (though it's obviously a good short-term workaround). Instead, you can very easily create a both efficient and safe version of the algorithm.

If you look at the code in our copy of <algorithm> you can see how this works - the algorithm code takes a checked iterator, does appropriate initial condition checking, and then gets an unchecked iterator from that checked iterator. It can then pass this unchecked iterator on to the core of the algorithm.

If you need help, I'd be glad to work with you to build a more directly comparable version of the example.

Martyn

martynl wrote:

If you look at the code in our copy of <algorithm> you can see how this works - the algorithm code takes a checked iterator, does appropriate initial condition checking, and then gets an unchecked iterator from that checked iterator. It can then pass this unchecked iterator on to the core of the algorithm.

Yup, I rewrote the code to use _CHECKED_BASE which sped it up greatly. However, now it's no longer standards compliant code, I'd need to work with #ifdef etc to make a version that will compile on other compilers.

Sven,

You're absolutely right, and this is why we have worked with the C standard committee on the C functions, and hope, based on experience gained from this relase, to work with the C++ committee on improving the safety of those libraries.

For the default containers, algorithms and iterators, you end up being safer, performant and portable with little or no code change.

However, if you start using custom algorithms or iterators, you end up with these choices, as the standard is currently. We support all three:

You can be safer and portable, with performance loss in some cases. (usual constructs -- default is safe and portable)
You can portable and performant, with a loss of safety (_SECURE_SCL=0)
You can be safer and performant, with a portability loss (use _CHECKED_BASE, stdext::checked_array_iterator and so on).

Though we can't always deliver all three simultaneously, we did work hard to make sure you could choose any point in the solution space you prefer, and to make sure that our default was both safe and portable as far as possible.

Martyn

Martyn,

Can you confirm that crashes can occur if code with one setting of _SECURE_SCL is linked to DLLs with a different setting?  I can't seem to find any official documentation related to this.

If so, are there any known work-arounds?  Is it a possibility that Microsoft will issue a patch that provides cross-compatibility?  Would it at least be possible to document the exact usages that don't work?

I would think pretty much any library vendor is greatly inconvenienced by this -- you either have to make a choice and force it down your customers throats or release YET ANOTHER complete set of DLLs so the customer can choose which one to link to.

Thanks.

Hey, do not try to “civilize” C too much! It _should_ be a knife. Everyone admires performances right, so if you make it too “safe” you may kill some of it.

 

I think this effort to enhance standards is a good one. Especially nowadays when there is a lot of mediocre code accessible via internet.

 

Regarding the functions like _countof, I don’t see it as a problem, we can always create something similar (re-write LOL) should we decide move to other platforms etc. it’s not a black-box, that’s a beauty of native code.

 

I think many people are interested to hear about the plans for Visual Studio IDE.  Right now, it’s not religiously C/C++-centric.

 

Can you make EXCLUSEIVE C/C++/MFC IDE, super fast on usage (it is not only compilation time that matters – but how fast it responses to our inputs), keyboard-usage friendly and “smart”. “Smart” means – when you hit Project/Settings -> do not display the first panel (which reads NET) but the last panel used, and stuff like that.

I think many people would like to see that.