undefined behaviour in theory and in practice

Question

I have large C++ project written by someone else long time ago. It contains code like:

string CVersion::GetVersionStr() const
{
  string ret;
  char VersionStr[100];
  DWORD v1, v2, v3, Build;
  GetVersion(&v1, &v2, &v3, &Build);
  sprintf(VersionStr, "%d.%d.%d.%d", v1, v2, v3, Build);
  return string(VersionStr);
}

Now I think because of wrong format specifier (%d) this code has undefined behaviour.

DWORD on my PC is declared as

typedef unsigned long DWORD;

My questions are:

• does code contain undefined behaviour?
• Is there any platform/situation where it would not be undefined behaviour? Maybe it is fine for some values of v1?
• The software has been working correctly for long time, so can it happen that in practice, despite above is undefined behaviour, the software still works fine?

PS. This software was written something like 10 years ago using Visual Studio

Answer 1

Yes, the behaviour is undefined if DWORD is an unsigned long. The correct format specifier is %lu.

Because undefined behaviour is exactly that, your second and third questions are not meaningful.

Why not use something like a std::stringstream and exploit <<?

Answer 2

Others have reported that this is indeed Undefined Behaviour and as such, should be fixed. However, I understand that in a large project, fixing things which have apparently woked for ages can be costly and hard to justify.

Why it's works for you: when using Visual Studio on Windows, int and long are the same size, and 2's complement is used for signed numbers.

If you're willing to live with having UB in your code as long as that UB seems to be doing what you want, you can add a safeguard so that the compiler tells you when the UB results might change. Add this assertion:

static_assert(sizeof(DWORD) == sizeof(int), "Fix the format specifiers NOW");

somewhere in your code (ideally near such a printf). As long as this assertion holds, the risk of things blowing up are fairly small.

They're non-zero because the compiler could say "UB isn't supposed to happen" and optimise the whole thing out or something like that. But that doesn't seem to be happening with your compiler, and is not something I would expect Visual Studio's compiler to do. But if you go this route, you should definitely rigorosuly test the relevant parts of code each time you change compiler versions or settings.

In the end, it's your call: as it's been working, the danger of something going wrong out of the blue is fairly small (especially if you add the size assertion). But it is a potential problem swept under a rug.

---

To summarise my view: yes, it can work in practice in your situation (VS on Windows only), but you have to be careful. Rigorously test anything before shipping it. The chance of something breaking is fairly small. Staying with the same compiler version & settings helps to keep it as small as it can be.

Answer 3

does code contain undefined behaviour?

Yes, as already pointed out.

Is there any platform/situation where it would not be undefined behaviour?

UB is defined by the Standard, not by implementations. So no, unless it's nonconforming (but then it wouldn't be C++).

The software has been working correctly for long time, so can it happen that in practice, despite above is undefined behaviour, the software still works fine?

A valid consequence of UB is working fine. So yes.

Answer 4

I'm probably going against the grain a bit with this answer but I would say testing is king.

From an engineering perspective I would say that as long as your software operates according to spec during rigorous testing then its good to go. That would be my definition of "good to go".

HOWEVER you have identified some coding practices that could trip this software up in environments your testing does not cover or that may even give rigorous testing regimes false positives.

The code needs to be fixed but you also have to weigh up the cost of potentially introducing new bugs when you go through fixing all the old bugs.

First I would institute a coding practice to end the practice that produces undefined behavior.

Then I would think about incrementally fixing the old bugs so you don't make too many changes to the code base all at once. You can then concentrate on rigorously testing the new code for new bugs before moving on to a new section of the code base.

What you don't want to end up with is a code base that is less stable than you began with because you introduced new bugs all over the place while trying to fix the old bugs.

Answer 5

It is undefined behavior as per the C++11 standard (more precisely, the portion of the standard "inherited" from the C99 standard, for the actual wording you would need to look mostly at the latter). However, you said it's been working fine, which, like any other behavior, is perfectly compatible with the notion of UB. Some compilers generate code that generates a "hard" run time error on specific types of (detectable) UB. Just today I saw that Clang may generate a UD2 opcode (which is undefined and guaranteed to cause a processor exception) on x86-64 for code like this (debug, no optimizations):

int &f()
{
// no "return"
}

GCC, on the other hand, will generate a no-op function and accessing the return value will cause an access to a "random" memory location, probably leading to a segfault. Both compilers may print a warning, Clang by default, GCC when -Wall is specified. So, as you can see, the practical consequences of UB differ.

You said that the printf has worked for years. Modern compilers generally may produce warnings for mismatching or outright invalid printf format string literals. I can only make guesses about the code that VC++ generates, however I do know that int and long have the same size on both x86 and x86-64 because VC++ uses the "LLP64" memory model (in which (unsigned) long long` and pointers are 64-bit, but long is only 32-bit). So, if the compiler just follows the "do what I say" approach, the worst that can happen in practice is displaying a very large unsigned value as a negative value because of the sign mismatch, and you have probably not used very large values.

GCC and Clang on x86-64 use the "LP64" model in which both long and long long are 64 bit wide, which is more likely to cause practical problems here.

Answer 6

This tries to answer your question in the comments whether you should in my opinion fix the code.

The answer in my opinion depends on:

• Whether there are good tests with extreme values which cover the code paths in question, and the tests are performed whenever the compiler or the CRT changes (the links go to articles about the major overhauls that happen(ed)).

• The actual type/format mismatch. Cf. also the answers to my follow-up question, especially Jonathan's whose opinion I always regard valuable. I'd not be concerned if in order of importance

  • no string conversions are present for non-addresses or potentially non-null-terminated data;
  • all formats are integer types (because some bit patterns for example are NaNs when interpreted as floats, but all integer bit patterns are legal);
  • the summed-up formats "promise" not more data than is actually passed as arguments (e.g. "%ld" with int args would be bad); that is because printf would access memory beyond its call stack. Unless it's Friday the 13th reading beyond printf's stack shouldn't be a problem because it's a read access and almost certainly within the programs's allocated memory, but still.

• The target architecture and build system.

  • Many caveats in the standard do not apply to an off-the-shelf x86 PC (which you may be running). This is the case with reading uninitialized data which works like a charm unless your register has a "you made a mistake" bit, cf. Trap Representation, unsigned char and IA64 NaT.
  • On a little endian system like an x86 you'll read the same small values from a memory location independent of the integer type as which you interpret it, but not so on a big endian architecture.
  • Protect against porting (portions) of the code, however small the chances seem. I liked Angew's assert.
  • While formally the compiler is free to do what it wants -- the code is, after all, invoking undefined behaviour --, the Microsoft people are less likely to say "nyaah, nyaah, the standards people said we can do this" than the gcc people.
  • To get an idea why everybody is so picky about UB these days read this fairly popular and instructive LLVM blog article about interactions between UB and optimization which lead to unexpected results. The bottom line is that modern compilers may "see" more of the code from both the called and the calling side than they did when each translation unit was compiled separately and there was no link time optimization. For the old model it's fair to say "printf necessarily uses va_arg, the code cannot know anything about the actual arguments, and therefore we are shielded and safe". These days printf's code may be considered at link optimization time and suddenly be open to a whole new class of optimizations like circumvening va_arg altogether, using intrinsics or subsequent "code poisoning" because the type mismatch is suddenly visible (without parsing the format string).

• The semantics. The output of printf with a conversion which does not match the exact argument type may be "wrong", i.e. interpret a negative value as a large positive unsigned etc. If a build version number in a message box of an in-house program becomes negative, who cares, really. But if the version number is on an ATM screen or on a test report submitted to the FAA it's unacceptable.

Therefore: For the example given I'd technically not be concerned, because the arguments and the expected arguments are integers of the same size (32 bit). That is true even for 64 bit architectures, probably for reasons similar to yours. And the Microsoft compiler is less likely to do surprising things.

That said, I like a clean code base and would make it a long-term goal to clean it up, but without haste, with reviews and with good tests. That can happen file by file, there is no need to fix all or nothing for a given release.

Answer 7

First of all, yes, in general this is undefined behavior according to the standard.

As to whether or not it is safe on your platform:
I can't promise you anything, but on Windows, a unsigned long is actually the same as unsigned int (even on x64), so - assuming your values are non-negative numbers (which is a reasonable assumption for version numbers) - this should be ok.

I'd still recommend that you start fixing that code. It is still a bug and especially if you ever want to run it on a different platform it might explode in your face at some time.

Answer 8

It used to be that in cases where the Standard would impose no requirements about the consequences of an action, but on some particular platform there would be a clear and obvious meaning, compiler writers were expected to generate code to yield that meaning. In cases where there were several plausible meanings, compiler writers were expected, absent a compelling reason to do otherwise, to write code which would, at worst, select among them in Unspecified fashion [e.g. on many platforms where "int" is 16 bits, given:

long mul(int a, int b) { return a*b; }

calling mul(5, 16384); might arbitrarily yield 81920 or 16384, but would be unlikely to do anything else]. This philosophy allowed C to thrive, since it meant that code which wouldn't need to run on obscure platforms could use features and guarantees which would work on everything else, but still allowed people to use C on platforms which couldn't honor those guarantees to accomplish tasks which didn't need them.

For some reason, that view has become unfashionable. If code doesn't care whether a function like the above returns 81920 or 16384 (e.g. because its purpose is to identify "potentially-interesting" objects, objects which are interesting won't cause overflow, and excluding 95% of uninteresting objects quickly is better than excluding 100% more slowly) being able to let the compiler return whichever result it can do more easily may allow for more efficient code generation than would be possible in a modern "Undefined Behavior must be avoided at all costs even if it makes code slower" compiler which requires a programmer to indicate which result must be returned, but there's no way a modern C program can leave that choice to the compiler.