Aargh! Part Seven

Aargh! Part Seven

  • Comments 23

Q: How do pirates keep their socks from falling down?

A: Thumbtacks.

I am insanely busy with bug fixing and performance testing today, so once more I'll dip into my endless archive of rants about irksome coding practices I've seen one time too many.

Gripe #8: Assert the truth, the whole truth, and nothing but the truth

Debug assertions should always describe invariants -- conditions that should always happen no matter what. They both help document your program, by making your invariants clear, and help catch bugs by alerting you when those invariants are violated. But assertions must describe what you believe to be always true about your algorithm, not what you hope is true:

pv = malloc(10);
Assert(NULL != pv);

Aargh! That's drivin' me nuts!

NULL is a legal return value of malloc and you therefore can't assert that malloc didn't return it. That condition, rare though it might be, has to be tested like any other. You can't simply declare that the world is going to turn out the way you'd like it.

Sometimes you want to pop up warnings in your debug build when incredibly rare things happen. That's a great idea -- but create a little function called CreateDebugWarning or some such thing, so that it does not get confused with Assert .

Gripe #9: Don't be so darn friendly

In C++ you can hide implementation details with the private keyword, but sometimes you want to have two classes which have the ability to party on each other's internals. For example, you might have a collection class and an enumerator class that need to have a private way to communicate with each other. Such classes are called "friendly classes". One day I grepped the headers of an unnamed Microsoft application for the word friend. It appeared over 600 times! Aargh!

This is a bad sign. One class had eleven different friend classes. When you have that many friends, there really is no difference between the private interface and the public interface. Visibility modifiers exist in the first place so that you can do information hiding and clean polymorphism. If you have to allow friend access to so many different classes then your public interfaces are not clean. You have a bunch of classes depending on each other's implementation details in order to work properly. The information hiding afforded by classes and interfaces is a feature of C++ specifically designed to reduce the complexity inherant in large software projects -- friend is a way to work around that limitation when necessary. And there certainly are times when it is necessary, but overuse makes code more and more complex, intertwined, buggy and unmaintainable.

Use friends very sparingly in C++ -- enumerators should be friends of collections, yes, but if documents are friends of views, you might have a problem on your hands.

In C#, JScript .NET and VB.NET there are no friendly classes. Rather, there is private, public and internal -- in C#, private really is private, but any class in the same assembly can party on internal data. This gives you a lot of the benefits of friendly classes while still being able to restrict access to stuff that is truly private.

 

  • I had this doubt for quite some time. Maybe it is obvious, but still. Shouldn't implementations of strcpy() etc. have appropriate assert statements to check whether the pointer is valid? I never found them in textbooks or in the glibc.


    Thanks,
    Vishal.
  • DangerMike: To be honest, I really don't worry at all about incidental misuse by undisciplined developers, and it's the sort of thing that's probably a warn-once (if that) offense in many shops. However, I do worry a great deal about little things like deliberate bypassing of input boundaries when code is running in environments that are outside of my control. There are ways to raise the bar against this sort of thing, but they are tedious and time-consuming, and I'm more than a little fed up of designing and implementing to accomodate the workarounds.

    Eric: Actually, I wasn't confusing the implementation with the spec--I was just being a wee lot too terse. Of course there's no way to use the language itself to do much of anything at all, including inappropriate invocation of low-accessibility members. <gdr> However, most of us don't spend our days compiling and running against the spec, and I'm a little more concerned about the behaviour of the "defining" production implementation than I am about the specification since it's the former that causes me far more pain.
  • Right, I take your point. My point is simply that your complaint is about the design of the .NET Framework reflection classes, not about the design of the C# language.

    What I don't understand is the nature of your complaint. Obviously, violating encapsulation is in general a bad thing to do, but there are some rare times when you need to do so. (In fact, the VSTO2 runtime does some private reflection, though I would like to eliminate it if possible.)

    But anyone who reflects upon a private should know that they're playing with fire. What kinds of scenarios are you worried about?
  • > Shouldn't implementations of strcpy() etc. have appropriate assert statements to check whether the pointer is valid? I never found them in textbooks or in the glibc.

    Yes, I think that debug builds should have some assertions to trap errors in callers. Retail builds, no.

    (Of course, a big part of the problem is that the standard library has a lot of problems with its function interface design. Another big part of the problem is that C's memory model allows you to pass in bad pointers in the first place. Fix those two things and a lot of the need for assertions goes away.)

  • Not all limitations of accessibility are mere matters of convenience. Some are meant to limit the set of actions available to external code for reasons of security or IP protection. One fairly common example is the establishment of an input boundary. Direct manipulation of private fields allows for trivial bypass of input boundaries. This can be countered in some cases via a large number of tedious and annoying CAS verifications, but it's a lot of trouble, and there are cases where there's no workaround short of a fairly major design change.

    There are really two main problems here:

    1. Many developers do not realize that reflection on supposedly low accessibility members can expose their code to risks they thought they had eliminated by declaring the low accessibility in the first place. These folks are shipping applications that are not as robust to attack as they might believe, even after they've potentially expended a great deal of effort to implement a variety of protective measures.

    2. Some are aware of the problem, but that almost only serves to make things worse. Client expectations sometimes require us to do whatever is possible to minimize the potential security holes opened by reflection into low accessibility members. There are essentially three choices: do a rather large amount of work to raise the bar a bit, pick a different platform, or target clients who don't care if their internal users and/or administrators can hack your application in a trivial manner.

    (If you can't see why scenarios like bypassing of input boundaries by legitimate users might pose serious security risks, or if you're not aware of the painful steps required to prevent this, let me know, and I'll post a private message. I'm just not very comfortable posting publicly what would essentially be a hacking guide.)
  • > #8 - Debug assertions should always describe invariants

    Well, maybe for you, I use asserts to describe my assumptions (how do I know something is invariant? I don't, I assume it is, so maybe I'm just splitting hairs). I agree your example is bad, but sometimes that is a development placeholder that says "I haven't figured out what my failure handling procedure should be here, so I'll assert this during development to show that I've assumed malloc() succeeding to work for now" (a bit lick, but better than , a "TODO" comment). You do manually inspect every assert() before releasing code, don't you....

    > #9 - Use friends very sparingly in C++

    Well, maybe that's how you use friends, but others may do it differently. Try reading Jiri Soukup's book on Taming C++ which uses friends for a very different but useful purpose, he uses them to make design patterns distinguishable in the delivered source (where they normally vanish between the design docs and the coding). Again, I agree that overdoing friends CAN be a bad sign, but I'd point out that there is more than one way to write code in languages as rich as C++.

    I'd remind all developers to question their own assumptions as well as questioning other peoples' implementations - if they don't match up the fault is not always that of the "Other guy".

  • Oh my god yes, your example #8 drives me completely up a wall. I see it all the time:

    rc = some_function(args);
    ASSERT(rc == SUCCESS);

    Which makes me want to ask the developer: "What the hell is wrong with you?" and smack them with a bat.

    > Actually, the Assert in the IntegerDivide function is
    > not so bad -- if the documentation for the function
    > says that it should only be called with a non-zero
    > divisor.

    Yes, it really is that bad (assuming it's not private / testing some purely internal assumption). A function that will panic or assert when it is given bad input is a function that has a bug in it. All functions should check their inputs for validity and return an error if it finds they are invalid. Anything else is just begging for misuse and brokenness.

    Properly used, asserts should always, 100% of the time, indicate logical fallacies or programming errors.

    I'm conflicted on ASSERT(0). I understsand the motivation but hey, if you want to make your process panic so you can debug it, why not just use panic() instead? That's what it's for.
  • I had this doubt for quite some time. Maybe it is obvious, but still. Shouldn't implementations of strcpy() etc. have appropriate assert statements to check whether the pointer is valid? I never found them in textbooks or in the glibc.


    Thanks,
    Vishal.

Page 2 of 2 (23 items) 12