allow literal string only

[ahmd]

Hmmm I'm actually confused as to why this difference (having or not having 'static') actually matters on a const at function scope...

static keyword on a data storage container (i.e. variable) makes compiler to place it in a different memory segment (i.e. read-only section if 'const' is used or implied) thus I gave you an example above on how to detect it. My code has to be run only in a debugger (or even your special) build. It can point lines of code where your strings are allocated dynamically that will help you change the code. In case of a big development team, you can do such checks once in a while to correct it up to your standards.

Otherwise there's no mechanism that is built in the currently available compiler to tell the difference and all your attempts to circumvent it won't result in a better (or safer) code. I think I heard this one before...

Oh, and const keyword simply instructs your compiler to catch any assignments done to the variable. It does nothing regarding placement in memory. It can be both a local stack/heap or a read-only memory.

Listen, not to be a pill, but why do you need such distinction in strings? And what are your "performance issues"?

[OReubens]

The issue is that this 'Foo' object has many many (several thousand) occurances throughout the code base. It appears so often and appears in vary basic elementary functions (accessors or simple operators) that the constructor literally gets called hundredthousands of times per second. In many cases, the Foo construction/destruction is already a bigger part of the performance than the rest of the work the function is doing. So any small bit of performance change in 'Foo' has a severe impact on overall performance

Changing the class from a const char* member assignment to a CString or std::string member assignment makes the entire program slow to a crawl. A 15 second calculation cycle slows down to a 4 minute calculation cycle. This is unacceptable in a release build (it's even unacceptable in a debug build). Some of the more math heavy stuff using lots of the 'small' operators has an even more dramatic slowdown. A 20second gauss+FFT filter slows down to a 12minute calculation.
The big problem is that allocating a buffer, copying a string, and freeing the buffer are just humongous amounts of code to be executed in comparison to the small functions Foo is located in.

[Paul McKenzie]

and security issue in that the string is assumed to be located in readonly memory.

This is assuming that the compiler that is being used guarantees that this is where string literals are placed (read-only-memory). There really is nothing in the standard that states where string-literals are placed, only that writing to them invokes undefined behaviour.

I remember older versions of Visual C++ did not place string literals in read-only memory, which is why a lot of older C++ programs that used string-literals incorrectly (i.e. writing to them) seemed to work.

Regards,

Paul McKenzie

[ahmd]

and security issue in that the string is assumed to be located in readonly memory.

It's not a cure-all solution, but you can use /NXCOMPAT, and /DYNAMICBASE compiler directives. Also consider employing the Standard Annotation Language (SAL) to guard against possible buffer overruns that can be exploited as a security breach. I believe all that will require a newer compiler and at least Windows Vista.

So any small bit of performance change in 'Foo' has a severe impact on overall performance

First of all, you don't place the DebuggerTestConstStringPtr function into a release build. You don't even have to place it into a debug build.

Again, obviously I'm not aware of the workings of your project but here's how I would do it:

1. Make a define that will control whether that check function is invoked:

#define DO_CONST_STRING_CHECKS    //Comment this line if checks aren't needed

2. Define the check function to be used on a pre-compiler condition:

#ifdef DO_CONST_STRING_CHECKS
	CHECK_STRING(pString, nCodeLine, pCodeFile) CGlobalClass::DebuggerTestConstStringPtr(pString, nCodeLine, pCodeFile)
#else
	CHECK_STRING(pString, nCodeLine, pCodeFile) 
#endif

3. Then use CHECK_STRING throughout your code. It will obviously require you to put those string checks everywhere throughout the code (there's no way around it at this point). But later on you can instruct your team coders to use that simple line in every new method they write. Better still, you can put it into your class members that can be reused throughout the code and avoid the need to use it if the class methods are called instead.

CHECK_STRING(pYourStringVariableToCheck, __LINE__, __FILE__);
//You may be even able to put __LINE__ and __FILE__ into the #define statement itself to simplify it even more. I'll let you check that yourself.

4. If DO_CONST_STRING_CHECKS is not defined, you will get your normal build. But, as I said above, once in awhile to check that the coders in your team are doing a good job you define DO_CONST_STRING_CHECKS and make a test build. I understand that it will make it very slow to run, but what's the problem? You won't be shipping it like that anyway. It will simply tell you which lines are not up to your specification -- both in Release and Debug builds. (If you remove ASSERTs from my post with a complete check function, all bad strings will be dumped into a debugger output window. So all you need to do is to check those code lines and correct the problem. Once corrected you don't need to worry about it again.)


PS. The only reason how this method wouldn't work, is if you have your code perform some time-critical operations, i.e. something that ties up your code performance with some outside hardware peripheral. In that case, I believe you're stuck with using a built-in search engine in MS VC and a lot of tedious work.

[ahmd]

As I come to think about it, you can probably speed up the check function by removing the exception handler and by calling IsBadWritePtr only on the first byte of the string. It will not be quite accurate but it will significantly speed it up.

[OReubens]

This is assuming that the compiler that is being used guarantees that this is where string literals are placed (read-only-memory). There really is nothing in the standard that states where string-literals are placed, only that writing to them invokes undefined behaviour.

I remember older versions of Visual C++ did not place string literals in read-only memory, which is why a lot of older C++ programs that used string-literals incorrectly (i.e. writing to them) seemed to work.

Portability is a non issue in my case. It's Windows only and VC++ only.

You're right about older versions of VC not putting a literal string in the const/readonly segment. This is why the old code still forces it this way by using a
static const char sz[] = "literal";
This particular syntax makes some devs think it's safe to pass other types of strings. We're busy actually phasing out this old construct and going with literals in the constructor only.

The "security" aspect isn't that much of an issue, if nothing else it satisfies the concerns of the tester team that the strings can't be changed in any possible way (i'm not going to tell them you can modify the readonly status via a VirtualAlloc call )

[OReubens]

First of all, you don't place the DebuggerTestConstStringPtr function into a release build. You don't even have to place it into a debug build.

It has to either be arranged at compile time (in which case it's part of the interface contract) or it would have to be in a release build. The developers can only do unit tests to test their own interfaces. It's up to the tester team to go through all the instances and regression testing.

Even if we could do this in a debug build, this sort of test would make debug builds so slow that we'd end up wasting too much developer time. If a unit test ends up taking more than a minute, this typically means the developer will be having a 5+ min break at the coffee machine. If a unit test takes more than 15Mins... god know what they'll go doing... it's the end of the world !!!

Like I said, doing a IsBadWritePtr test is slower than copying the contents, which is already really bad.

once in awhile to check that the coders in your team are doing a good job you define DO_CONST_STRING_CHECKS and make a test build.

The problem here is... Which of the X devs is going to be assigned to doing this ? How are you going to make sure that whomever is testing goes through every single possible case and instance.


The last case we caught (and which was the main trigger to this forum post)
had code like this:
Foo( <test> ? "literal" : CString("otherliteral")+obj.ToString() );
The <test> was true in 99.99% of the cases, which is why it went through the regression testing, and unit testing, and instance testing. And it is why it very very rarely (and not consistently reproducibly) ended up failing horribly at some customers. Where it actually did go wrong wasn't even anywhere near this constructor call, but passing a "dirty" pointer caused memory to be overwritten elsewhere, causing issues in unrelated and random places.

The tester team then told us that we EITHER had to somehow with 100% certainty prevent such constructs from ever being made again. OR we would have to create a framework for doing elaborate instance tests. To try each and every possible combination of parameters. (=pretty much impossible)

Trying to prevent is is easier. If the compiler can make that happen, so much the better, the alternative would have been to write a code parser that catches anything invalid and add that into our pre-compilation of the testing and release candidate builds.


The solution I have now (see above) is adequate. The one problem case left (non static const at function scope) gets cought by the code evaluation tool we have in the pre-compilation. It would have been nice to have a more generic solution though.

[ahmd]

the alternative would have been to write a code parser

Wow, you went the really hard way! I'm not sure if a code parser would be able to catch all the cases of your specification without actually compiling the code. Wouldn't it be easier to assign some interns (you must have some in summer) to go through a read-only version of the code source and point out inconsistencies?

Foo( <test> ? "literal" : CString("otherliteral")+obj.ToString() );

Just curious, why would this cause a problem?

If Foo() uses that string pointer as a read-only string it should not be an issue. If it doesn't, declare it as LPTSTR instead and you'll catch all inconsistencies at a compilation time.