Hebbut.Net: Public Offerings: CRM114 for Win32/UNIX

CRM114 says: "Cannot open/read/write file XXX" (but the file exists! @#$%^* ...) - FIXED

No elaborate part-by-part explanations like for the HS-NN-patch, this time around. This took several hours of work, backporting the fix to 'vanilla CRM114' (the GerH builds had this fix already for a while) and ensuring it's actually not screwing up another bit of code:make megatest should not segfault any faster than usual on 64-bit systems, plus Jason's test setup should PASS without any errors.

The goal: no more of the above error message, thank you very much.

Thank you

Thanks you to Jason Lewis for:

Without your effort, this would have remained a bug, subject to rich salon conjecture only, while driving away users from CRM114 due to unsolvable puzzles/surprises. Thanks, Jason!

The fix: the cause of

The cause of the trouble is due to boundary/overflow issues in one of the most important routines in crm114: crm_zexpand(...). Given a byte sequence to expand (unescape and :?: (where ?={*,+,#,@}) operator processing) this code would access bytes beyond the allowable input range.

Under specific circumstances, the code would even write beyond the maximum input/output storage space (stack- or heap-allocated array passed as a parameter): this not only inside the routine itself, but the calling code would sometimes assume the returned length would be small enough to allow writing an extra NUL sentinel byte into the destination buffer - without checking if the maximum range had been reached already. Note however, that that particular extra goodness is not the cause for a 64-bit UNIX system running CRM114 to report 'cannot open/read/write file XXX' every once in a while - seemingly completely at random and under extremely hard to reproduce circumstances.

The factual cause is due to the first bit of cruftiness mentioned above: as the code would scan past the allowable input range, specific values on the stack, located beyond the legal data range (limited by the length argument), would trigger crm_zexpand() to include those bytes into the expansion process.

For example, when the last legal byte is an ASCII '\' escape character, the unescape process might pull an extra illegal byte from beyond the input - some of the time. (Think beyond unescaping \r and \n: how about the escape sequence \/, hm?) Yes, I know, Paolo mentioned recently on the mailing list the latter should have been gone since, what?, 2004?, already, but not entirely so. Even the latest 'vanilla CRM114' WGET doesn't have it right all the time. And half a fix is no fix.

Why do I not provide detailed samples to show the incorrect behaviour?

Because,

Personally I think 'getting quite fed up' is closest to the truth - see also the conclusion at the end of this story.

Ladies and Gentlemen, here's the fix

I started today redoing the fix straight in 'vanilla CRM114' (i.e. the code directly available from sourceforge.net) but that did not work out like I hoped, thanks but no thanks to the GNU patch tool not accepting my diffs (clearly it does not like diff -u -EbwBd --strip-trailing output, and without all those whitespace-bashing options the diff is larger than the actual source because I regularly pull my sources through the uncrustify code formatter (uncrustify is way better than GNU indent in my unhumble opinion) to ensure it remains readable over time, despite quick fixes and all that jazz.

The diffs are here for your perusal:

As I don't think the diffs are much use beyond the 'let me grab this to fix my copy' activity, here are the actual source files themselves:

So far the fix. Tested on several 64-bit Linux boxes; passes Bill Yerazunis' megatest on all of them; passes the more strict make check tests from GerH releases too.

Enjoy.

Ger Hobbelt

Conclusion... and some advice

What to do when bugs happen

When, as a developer, you run into a bug, you can do several things. Your choice range includes

How did I find this bug and why is the number of (minor) code changes so 'large'?

I found the bug because I took the time to review the code. That's an example of 'fix it - and beyond'. Try it on for size. Yes, I am angry now.

You want to know what's wrong with the 'vanilla' version of the crm_expandvar.c source file?

One advice only.

Perform a code review.

Mind you: a proper code review does not only concern itself with the fact if code works for the regular use case(s).

A good code review is performed while keeping Murphy's Law in mind all the time, i.e. ask yourself: is there a chance I can make this bit screw up? And 'only once in a million' is once too often. As Mr. Pratchett so eloquently says in his books: if that last number is getting large enough, it's become a certainty.

And this code has lots of certainty.

When you review the code like I did, check the vanilla source and see how:

Do not stop at this list of hints as if it were your shopping list. Think about destructive ways to take to this code and discover.

When you got this far, take a look beyond. Have a look at crm_restrictvar() and any code using crm_zexpandvar(), either directly or indirectly (through the wrappers calls crm_nexpandvar() and crm_qexpandvar(). grep for their use throughout crm114 and scan those routines calling them. Track the variables used there. Think destruction. See the goodness.

Now answer me this riddle, please?

Should I/we wait for the issues you found while performing the review to appear thanks to user bug reports arriving?

And finally: verify me and my big mouth didn't screw up too!

Review the GerH copy too. I'm telling you any boundary/overflow issues in there have been fixed. Vertrauen ist gut, Kontrolle ist besser. [1][2]

Found something in here that's disputable or questionable? Email me or even better: send your comments to the crm114 mailing list (developer or general; depending on your judgment of the severity).

And if you think the code is a bit paranoid at places? I'd consider that a compliment: if I want to play lose, I'll go and visit a public house.

Exit stage left

Time enough spent on this. Now y'all are on.

Professional assistance is available at my regular rate. You'll get less attitude, yet more quality service.