Current version

v1.10.4 (stable)

Navigation

Main page
Archived news
Downloads
Documentation
   Capture
   Compiling
   Processing
   Crashes
Features
Filters
Plugin SDK
Knowledge base
Donate
Contact info
Forum
 
Other projects
   Altirra

Search

Archives

01 Dec - 31 Dec 2013
01 Oct - 31 Oct 2013
01 Aug - 31 Aug 2013
01 May - 31 May 2013
01 Mar - 31 Mar 2013
01 Feb - 29 Feb 2013
01 Dec - 31 Dec 2012
01 Nov - 30 Nov 2012
01 Oct - 31 Oct 2012
01 Sep - 30 Sep 2012
01 Aug - 31 Aug 2012
01 June - 30 June 2012
01 May - 31 May 2012
01 Apr - 30 Apr 2012
01 Dec - 31 Dec 2011
01 Nov - 30 Nov 2011
01 Oct - 31 Oct 2011
01 Sep - 30 Sep 2011
01 Aug - 31 Aug 2011
01 Jul - 31 Jul 2011
01 June - 30 June 2011
01 May - 31 May 2011
01 Apr - 30 Apr 2011
01 Mar - 31 Mar 2011
01 Feb - 29 Feb 2011
01 Jan - 31 Jan 2011
01 Dec - 31 Dec 2010
01 Nov - 30 Nov 2010
01 Oct - 31 Oct 2010
01 Sep - 30 Sep 2010
01 Aug - 31 Aug 2010
01 Jul - 31 Jul 2010
01 June - 30 June 2010
01 May - 31 May 2010
01 Apr - 30 Apr 2010
01 Mar - 31 Mar 2010
01 Feb - 29 Feb 2010
01 Jan - 31 Jan 2010
01 Dec - 31 Dec 2009
01 Nov - 30 Nov 2009
01 Oct - 31 Oct 2009
01 Sep - 30 Sep 2009
01 Aug - 31 Aug 2009
01 Jul - 31 Jul 2009
01 June - 30 June 2009
01 May - 31 May 2009
01 Apr - 30 Apr 2009
01 Mar - 31 Mar 2009
01 Feb - 29 Feb 2009
01 Jan - 31 Jan 2009
01 Dec - 31 Dec 2008
01 Nov - 30 Nov 2008
01 Oct - 31 Oct 2008
01 Sep - 30 Sep 2008
01 Aug - 31 Aug 2008
01 Jul - 31 Jul 2008
01 June - 30 June 2008
01 May - 31 May 2008
01 Apr - 30 Apr 2008
01 Mar - 31 Mar 2008
01 Feb - 29 Feb 2008
01 Jan - 31 Jan 2008
01 Dec - 31 Dec 2007
01 Nov - 30 Nov 2007
01 Oct - 31 Oct 2007
01 Sep - 30 Sep 2007
01 Aug - 31 Aug 2007
01 Jul - 31 Jul 2007
01 June - 30 June 2007
01 May - 31 May 2007
01 Apr - 30 Apr 2007
01 Mar - 31 Mar 2007
01 Feb - 29 Feb 2007
01 Jan - 31 Jan 2007
01 Dec - 31 Dec 2006
01 Nov - 30 Nov 2006
01 Oct - 31 Oct 2006
01 Sep - 30 Sep 2006
01 Aug - 31 Aug 2006
01 Jul - 31 Jul 2006
01 June - 30 June 2006
01 May - 31 May 2006
01 Apr - 30 Apr 2006
01 Mar - 31 Mar 2006
01 Feb - 29 Feb 2006
01 Jan - 31 Jan 2006
01 Dec - 31 Dec 2005
01 Nov - 30 Nov 2005
01 Oct - 31 Oct 2005
01 Sep - 30 Sep 2005
01 Aug - 31 Aug 2005
01 Jul - 31 Jul 2005
01 June - 30 June 2005
01 May - 31 May 2005
01 Apr - 30 Apr 2005
01 Mar - 31 Mar 2005
01 Feb - 29 Feb 2005
01 Jan - 31 Jan 2005
01 Dec - 31 Dec 2004
01 Nov - 30 Nov 2004
01 Oct - 31 Oct 2004
01 Sep - 30 Sep 2004
01 Aug - 31 Aug 2004

Stuff

Powered by Pivot  
XML: RSS feed 
XML: Atom feed 

§ "Fixing" working code

One of the lessons you eventually learn the hard way as a programmer is never to "fix" working code. If you have a confirmed bug, then fix it; if you have a feature you need to add, then by all means, change it.

"Fixing" code that just looks buggy, without actually knowing how it is bad, is a mistake.

Let's say you have an 80% chance of getting code right in the first place, and thus a 20% chance of getting it wrong. That means if you just look at it again, you have a 64% chance of verifying it right again, a 16% chance of writing it incorrectly but catching the bug, and a 4% chance of blowing it both times. That leaves a 16% chance of writing it correctly and thinking it's wrong. There's then a chance that you'll act on that instinct and break working code.

Here's a real example that I just discovered:

A certain vendor's C runtime library has code to display an error dialog under certain circumstances, such as an assert, with a program path. If the program path is too long, it truncates it and only displays the end of the path with an ellipsis at the start. This was done via strncpy():

if (strlen(path) > MAXLENGTH) {
    path += strlen(path) - MAXLENGTH;
    strncpy(path, "...", 3);
}

In a later version of the product, someone noticed the use of strncpy() and replaced it with strncpy_s():

if (strlen(path) > MAXLENGTH) {
    path += strlen(path) - MAXLENGTH;
    strncpy_s(path, "...", 3);
}

Now, I'm not really a fan of the secure string library -- I'd rather just use a string class instead -- but strncpy() is admittedly a bit risky to use given its tendency to create non-null terminated strings. You can use strncpy() safely, but it's easy to forget or goof the boundary check. Safer alternatives like strlcpy() are often used instead. At first glance, the change looks like a perfectly good improvement to eliminate any potential buffer overrun mistakes... except for two problems. One, the source is a string literal and the destination is a fixed buffer that is far larger than four characters, so there was never any issue with non-termination. Second, the original code actually depended on strncpy()'s non-terminating behavior, because it is putting the ellipsis at the beginning of the string. The attempt to fix a non-existent bug actually broke the code.

So, basically, don't guess at bugs. If you think there is a bug, reproduce it first so you can actually confirm it and verify that you actually fixed something instead of breaking it, and then keep that case around for regression testing.

Comments

Comments posted:


That's why test-driven programming was invented.

John - 09 02 07 - 12:11


On the other hand, it's possible to carry this too far. Relying too much on tests can cause one to produce code with nasty, hidden problems, because some situation not covered by the tests came up. If it looks wrong, make sure you look into it. Just get the whole picture before you decide it is wrong (a second opinion helps a lot).

TechMage89 - 09 02 07 - 15:27


I'm sure not a fan of the "secure string library" either, based on MS trying to sell it to us by proudly declaring the standard C string library "deprecated" ...

Glenn Maynard - 09 02 07 - 19:14


It is true that you do not want to rely too much on tests -- if you were able to foresee all possible failure cases when writing the tests, then you wouldn't have written buggy code, not to mention the potential for the tests themselves to be buggy. However, the real power of tests is in regression testing, making sure you haven't reintroduced a bug with new changes. You can get some benefit by writing tests up front, but the dividends really pay off when you're maintaining the code down the line and have built up a library of tests for bugs that you've actually hit in the past.

So, as usual, unit testing is not a panacea, but it's another weapon in the development arsenal.

Phaeron - 09 02 07 - 23:20


Yes Phaeron, that's exactly my point. :)

John - 11 02 07 - 07:13


Hmm this really sounds like someone ported a VS6 project in VS.NET, got 1000s of warnings about using strncpy() and blindly replaced them all woth strncpy_s() just to make the compiler shut up.

George Nakos - 12 02 07 - 09:43


This looks more like a case of upgrading to VC8 and attempting to get the damn compiler to shut up about the deprecations by just replacing them all without actually reading the code, rather than just setting the defines needed to make the headers not declare them deprecated. Another case of MS "fixing" a problem and causing all kinds of problems.

Coderjoe - 12 02 07 - 19:45


OT, but I think Microsoft added an uber-#define for turning off the deprecation warnings in VS2005 SP1. Haven't tried it, though.

Phaeron - 13 02 07 - 03:14


I think what happened is that someone replaced strncpy with strncpy_n because strncpy was in Microsoft's Banned Function List. Lesson, don't replace strncpy with strncpy_n blindly.

Yuhong Bao - 09 02 08 - 20:34

Comment form


Please keep comments on-topic for this entry. If you have unrelated comments about VirtualDub, the forum is a better place to post them.
Name:  
Remember personal info?

Email (Optional):
Your email address is only revealed to the blog owner and is not shown to the public.
URL (Optional):
Comment: /

An authentication dialog may appear when you click Post Comment. Simply type in "post" as the user and "now" as the password. I have had to do this to stop automated comment spam.



Small print: All html tags except <b> and <i> will be removed from your comment. You can make links by just typing the url or mail-address.