OT Programming: this is why I dislike c++

[grumpydoc]

On an off I have mentioned that I came to dislike C++ after many years of using it - I chanced across an example of why.

This code is from the "pan" newsreader which I sometimes use - and it contains a use-after-free

Here is the code:

   foreach(quarks_t, data.get_servers(), it)
    {
      Data::Server* s(data.find_server(*it));
      //other stuff.
    }


foreach is a macro, so the code actually expands to this


   for (quarks_t::iterator it(data.get_servers().begin()), it_end(data.get_servers().end());
        it != it_end;
        ++it)
    {
      Data::Server* s(data.find_server(*it));
      //other stuff.
    }


Can anyone see the use after free?

Hint: quarks_t is a class.

[alexanderbrevig]

This is old cpp.

std::for_each is the way to go. And no raw pointers unless you have to. Which is rare. (Yes I understand this is not your code.  Every language will have bad code written in it)

[grumpydoc]

std::for_each is the way to go.

Granted, though it would not address the fact that as well as broken the code is naff in terms of efficiency.

But std::for_each is a bit extra work - there is always going to be the temptation to write a "quick" for loop.

And no raw pointers unless you have to.

Agree, mostly, there are non here, as it happens.

Every language will have bad code written in it.

The thing is that it isn't terribly obvious that this is bad code, just to look at it (though now that I do, I think this issue did crop up in several projects that I worked on).

quarks_t is a std::set of Quark (some data type, the details aren't terribly important).

data.get_servers() returns the set by value. So a temporary variable exists to hold that - but it goes out of scope in the for control statement and is not available in the loop body. So the later *it reads memory after it has been freed.

std::for_each works because the temporary (should, I think) remain in scope.

The point is that this is a subtle trap for the unwary and C++ is full of them.

Oh and you're going to get two copes of that set - one in the called function and one in the calling stack frame which is initialised with the copy constructor. Nice for efficiency, not.

Oh, on further examination I think that the begin and end iterators are coming from different instances of the same list - I need to spend some time thinking about whether that is a bug of not (I have a feeling it only works by chance). That reminds me of the fun I had with standard containers in Windows circa 1999. I had one allocated in one dll which got used in another, except that the iterators ran off the end. Turns out each dll had it's own copy of the special "end" node so code in one dll would not iterate correctly over code in another because it was checking for the address of its own marker.

Bloody brilliant! 

[abraxa]

Stuff like this is why I love C++11.

Code: [Select]

for (const auto server : data.get_servers())
{
   Data::Server* s(data.find_server(server));
   //other stuff.
}

No more use-after-free, no bloody iterators and very easy to read. Granted, I'm not a fan of auto, but since the return value isn't known here, I used it

[alexanderbrevig]

I agree with most of what you say and how you feel.
There are raw pointers though. Two of them.

Can't get simpler than for_each or a ranged based for loop can you?
The above definitely is not simpler. And buggy too boost.

C++ 17 is really getting close to a very good language IMHO. What I learned in school cpp98 is shit. So hard that everyone fails at some level.

Copy by value or reference is implicit in good modern cpp and it is explicitly hard to get wrong if you what you're supposed to.
I.e do not ever write your own for loop with raw iterators or raw indexing.

C++17 has a place in my heart.

[grumpydoc]

Stuff like this is why I love C++11.

Code: [Select]

for (const auto server : data.get_servers())
{
   Data::Server* s(data.find_server(server));
   //other stuff.
}

No more use-after-free, no bloody iterators and very easy to read. Granted, I'm not a fan of auto, but since the return value isn't known here, I used it

Looks tidy, I'm not really up to date with anything newer than '98

It's still a bloody quagmire though.

[alexanderbrevig]

C++98 is not C++ 

[grumpydoc]

There are raw pointers though. Two of them.

Sorry, yes - I wasn't thinking about that bit so much, just contributors to the bug.

[grumpydoc]

C++98 is not C++ 

I stopped using C++ daily (which I had been doing for a dozen years) in 2004 so it pretty much is, as far as I'm concerned.

[alexanderbrevig]

Then you should forget everything you know and read up on C++17 (at least 14)
It's a whole new language since '04.

EDIT: I say this with a smile. You might like it! I think you will

[grumpydoc]

Then you should forget everything you know and read up on C++17 (at least 14)
It's a whole new language since '04.

EDIT: I say this with a smile. You might like it! I think you will

I'm sure I would but no longer earn my keep by coding. I wouldn't mind going back but got to a point where I had, within the space of a couple of years been made redundant, then joined another company which was clearly doomed (we wound up grabbing all the hardware and disks in lieu of salary with much owed). I also came to the realisation that to do more interesting stuff I would have to move, possibly to the 'States which wasn't something I wanted to do. So I resumed my initial career which is vastly different to IT. It would now take too much work, probably that move and involve too much of a pay cut to get back into IT.

However I always liked C++, it is a great language for writing really tight code, it always appealed to the inner geek.

But it is too easy to write code which no-one else can read or maintain, debuggers can't cope with the ludicrous names and inlined functions generated by the standard library (try to set a breakpoint in a container function), compilers can't give you sensible error messages when multiple complex nested template expansions are involved and there are just too many subtle things, like this one, which are waiting to bite you.

C++17 may be beautiful to behold but all the accumulated crud will still be there.

[abraxa]

To be fair, I'm totally convinced that they copied a lot from python and other modern languages. I'm not sure C++ would've seen this much improvement if languages like python wouldn't have become so very popular. I feel it's kind of "too late" for C++ since people seem more willing to pick up one of the modern languages instead, where the good parts of the language have been there from the beginning and not tacked on. The reputation and barrier to entry aren't that great either in comparison. And yes, the error messages because the compiler doesn't understand what C++ is trying to do... oh my.

However, I am happy that the language at least is evolving in good ways. It shows someone does care to make it better after all

[Red Squirrel]

I find a lot of stuff is unnecessarily complicated in C++, I often find myself making wrapper libraries around libraries just to make them easier to use.  Mysql++ for example.  It throws exceptions over stupid stuff.  Not finding a result, or even a syntax error in the SQL statement should NOT crash the entire program!   Having to put every SQL query in a try/catch block is quite ridiculous, so my wrapper takes care of that.   That's just an example mind you. I've done wrappers for other stuff too.

[John Coloccia]

IMHO, C++ has become kind of pointless. For really low level stuff, or safety critical code, it's C all the way. For higher level programming, C# is great. C++ from the late 90s would be a perfect bridge between the two today. There are some new features I like, but most of what's been changed/added really just doesn't strike a chord with me. I want C++ to be a nice, low level object oriented language. For anything beyond that, I'm grabbing a modern language.

I'd spent a large chunk of my career in C++. I haven't started a new C++ project in a LONG time. I might use it again if I had to do an embedded project that was complex enough to make C inconvenient, but that's about it at this point.

But the OP's example was more about "Don't do stupid, confusing crap, and you won't end up with stupid, confusing bugs."

[Ampera]

IMHO, C++ has become kind of pointless. For really low level stuff, or safety critical code, it's C all the way. For higher level programming, C# is great. C++ from the late 90s would be a perfect bridge between the two today. There are some new features I like, but most of what's been changed/added really just doesn't strike a chord with me. I want C++ to be a nice, low level object oriented language. For anything beyond that, I'm grabbing a modern language.

I'd spent a large chunk of my career in C++. I haven't started a new C++ project in a LONG time. I might use it again if I had to do an embedded project that was complex enough to make C inconvenient, but that's about it at this point.

But the OP's example was more about "Don't do stupid, confusing crap, and you won't end up with stupid, confusing bugs."

I actually quite like C as a multi purpose language. For my uses, at least, it can perform all the major tasks required by me. C and C++ have two amazing traits that a lot of modern languages don't always have, C especially.

Near universal support. C has compiler support on almost every single platform ever made since the 1970's. The base knowledge you have for C on one platform can be applied to pretty much every platform. Want to write a small C64 app? C can do that. Want to program for a modern x64 machine? C can do that too. Want to program for microcontrollers? That's C's biggest market atm. There are implementation differences, definitely, and most of what you actually would want to do changes completely platform by platform, but it has a leg up on languages like Java and C#, due to the fact that Java and C# has a lot less full implementations full stop.

C++ has for a while now been quite interesting to me. I might pick it up sometime if and when C becomes boring or useless to me. It is an about 46 year old language, designed for archaic minis long gone from our world, but modern implementations make those origins not so difficult.

[Red Squirrel]

To me what I like about C/C++ is that you are not tying yourself to a specific company.  It's basically open.  C# is proprietary and only made to run on Windows.  yeah you can emulate it on other OSes but nothing beats a native app.  "but Mono is not emulation" well it sorta is and it's same idea, either way they had to basically code well, an emulator, to be able to compile the .net based code, and emulate the windows libraries for .net etc.  Either way, it's not OS independent like C/C++ is.

Pure C is kinda garbage though, I really wish system libraries would stop using it.  There's no reason not to use C++ for that stuff. 

[stmdude]

Don't blame the language when the programmers are writing hard-to-understand code.

I'm no fan of C++ (big dangerous gun with a hair-trigger in my opinion), but even plain-old-C can be written in ways that makes you wonder about the sanity of the developer.

My favourite C "have to read it twice to understand":

Code: [Select]

int n = 10;
while(n --> 0) printf("n=%i\n",n);


[glarsson]

The famous unknown "downto" operator. :-)

[alexanderbrevig]

I think it's fun to leave at least one [index]array in my personal projects. Always brings a smile to my face when I stumble across it again.

[grumpydoc]

But the OP's example was more about "Don't do stupid, confusing crap, and you won't end up with stupid, confusing bugs."

No, the OP's point was C++ makes it easy to write perfectly innocent looking code which is as full of bugs as a tramp's bed.

Yes, there are better ways to do this in modern C++ - though a lot of open source projects need to keep compatibility with older compilers.

However writing

for (iterator i = container.begin(); i != container.end(), i++) {}

is pretty idiomatic in C++ and looks very benign, I wouldn't really think twice about writing that.

[jmsc_02]

Is it not a misusing of the ++ operator? it++ will works fine always in the loop.

I think blame C++ due to a programmer misunderstanding is like I say "cars are bad because are driven by drunken people" 

[grumpydoc]

Is it not a misusing of the ++ operator? it++ will works fine always in the loop.

It's actualy frightening how many potential issues there are with this innocuous looking loop.

Yes, the ++it is probably buggy - the iterator itself is in scope in the loop body, but the container over which it is iterating has been destroyed.

I think blame C++ due to a programmer misunderstanding is like I say "cars are bad because are driven by drunken people" 

No.

Not liking C++ because of pitfalls such as this is like not liking a make of car because, although it has a horn button, which most of the time when pushed the horn sounds, but occasionally - if the button is pushed in a particular way on the lower half, which is easy to do by accident, a three foot lokg spike emerges from the centre of the steering wheel and kills the driver.

[jmsc_02]

Is it not a misusing of the ++ operator? it++ will works fine always in the loop.

It's actualy frightening how many potential issues there are with this innocuous looking loop.

Yes, the ++it is probably buggy - the iterator itself is in scope in the loop body, but the container over which it is iterating has been destroyed.

Then is a failure of how and when access the data. An assert checking the class previous to the iteration will solve the issue, don't?

Macros, sometimes, are designed by the evil, but i don't think c++ has the blame...

I've read a book named "writing solid code" by Steve Maguire. It's ancient and C oriented, but has very clever ideas and steps to avoid this kind of things.

[grumpydoc]

Is it not a misusing of the ++ operator? it++ will works fine always in the loop.

It's actualy frightening how many potential issues there are with this innocuous looking loop.

Yes, the ++it is probably buggy - the iterator itself is in scope in the loop body, but the container over which it is iterating has been destroyed.

Then is a failure of how and when access the data. An assert previous the iteration will solve the issue, don't?

Well, being aware that writing functions which return complex types by value will cause the compiler to make temporaries, the lifetimes of which might bite you, or that multiple invocations of such functions will create more than one instance of the value and that might bite you as well is what is needed.

My beef is that the above detail is subtle, too easy to miss, too easy to introduce into working code if a level of indirection is later added and breaks encapsulation (because I have to worry about implementation details of the function I am calling).

I suspect that using std::for_each is the "correct" solution here but the following is simple and correct, and what I suggested to the pan maintainers (we'll see if they want to do it like this or with a more modern method).


   quarks_t server_list = data.get_servers();
   foreach(quarks_t, server_list, it)
    {
      Data::Server* s(data.find_server(*it));
      //other stuff.
    }

[John Coloccia]

Personally, I don't think it's a particularly subtle bug. It's hard to see in the OP because you can't see the .h file, so it's somewhat impossible to figure out other than guessing.

It's been a while since I've played with C++ regularly, but I seem to recall that you should get a warning about taking the address of a temporary. Why was the warning ignored? If this is one of those code bases where every file throws dozens of warnings, I'm not surprised if it's littered with amateurish bugs. I've never understood why warnings are just simply ignored instead of being fixed. We strive for squeaky clean compiles and it's the extremely rare case that can't be reasonably fixed, or where the code is 100% correct and there's no way around the warning. For example, One of my current projects involves a code base that's roughly somewhere between 50,000 and 100,000 lines and a dozen or so separate projects (haven't really counted in a while so I forget). I think the entire code base might have a total of 3 or 4 warnings in it, and they're all at the link stage where we've had to do some funkiness in the memory map.