Re: still getting errors with CRM

On Thu, May 1, 2008 at 2:08 AM, David Morris <[email protected]> wrote:
>  > Think
>  > buf[] = "hello".
>  > That's
>  > buflen=5
>  > so 'o' is at
>  > buf[4] (remember, 'C' is zero-based)
>  > and the '\' is here:
>  > buf[5] == '\\'   !!!
>
>  Being the incurably curious software engineer that I am,
>  this piqued my curiosity because the length of a 5-character
>  string is typically *six* characters to account for NULL
>  termination.  At the very least, an outside view may
>  help....

Good you mention that:
My oops. crm114 is 8-bit complete, that means it is (or should) be
able to handle any byte sequences you throw at it, including embedded
NULs. As such, 'strings' in CRM114 are not exactly 'C' strings, but
rather pointer+length combos; that's also why all those lengths get
passed around between function calls, next to buffer pointers.

I forgot to mention I was describing 'strings' in the CRM sense, so
(len=5, buf=0xaddress-->{'h','e','l','l','o'}).


>  Looking at the code in the attached file, all loops on the
>  string look like this:
>
>     for (is = 0; is <= inlen ; is++)

About those for() loops you mention: exactly; that's one part of the
story: the 'is' index is peeking 'one beyond' legal/filled buffer
space. And, again, CRM 'strings' do not need to be NUL terminated - in
theory.

>   buf[inlen] = ' '; // prove bad behaviour: this is written outside buf valid range, so should have no effect
>
>  However, the comment is wrong.  Because of the loop
>  definition, you are changing data which is being processed.

Sorry, comment is right ;-) , some assumptions are not listed and that
may have been confusing.

>  Of course, the request to "set buf[inlen] = '\\'"
>  illustrates what looks to me like a *major* boundary-condition
>  bug assuming that the string in 'buf' is *not* necissarily
>  NULL-terminated (which I am guessing is the case as othewise
>  there should be a check to verify "buf[inlen] == '\0'").

Yup, boundary condition indeed. And no, buf[inlen] is an UNDEFINED
value, as string is not NUL terminated.

I put that little code line there to show people what is going wrong
in a reproducible way, without Jason having to run his set every time
again because we need a look at it; for some reason he's got a
situation where other operations 'leave behind' a backslash-valued
byte at a spot which is ONE BEYOND legal content (inlen size bytes of
buf[]) once we get to the expand routine there.
Nothing wrong with that backslash byte, everything wrong with the code
trying to parse it while it should not.

>  statement is now processing the location buf[inlen+1].

Bingo. That's one MORE boundary issue. See my diffs from two days (3
days?) ago on this thread for a possible fix for THAT one too:

  switch (is >= inlen ? 0 : buf[is]) // [i_a] makes sure '\\' as last
char in string is 'done' properly

so, once the loop condition is fixed to 'is < inlen', a LEGAL '\\'
byte value at the end (i.e. at buf[inlen - 1]) does not cause another
boundary issue by 'faking' an extra NUL byte WITHOUT accessing memory
(for some very long strings and particular calls, you may be even
poking in unallocated space when you read buf[inlen] - which may or
may not be caught by boundary scanning tools like valgrind,
BoundsChecker, etc.)

Anyway, imagine a 'last legal byte is backslash' situation without
that little extra switch(x?0:c) tweak and now replace in your mind the
byte beyond, i.e. at buf[inlen], to be, say, a ASCII 'n'. Which will
be pulled in by the code to replace the legal backslash with a utterly
invalid LF (LineFeed) character as the 'n' in this example is out of
bounds.
(Be warned: this is ANOTHER / SECOND boundary condition in the code,
which is currently not part of Jason's issue. Nevertheless, you've got
another one there.)


>  Now, perhaps something is going on here that I am missing,
>  but since there is a check to verify that 'inlen <= maxlen',
>  this implies the code is trying to process invalid data.
>
>  I'm guessing the code should look something like this:
>
>  ================================================================================
>     for (is = 0; is <= inlen ; is++)

correction: valid range is actually

   for (is = 0; is < inlen ; is++)

which is why this is such a nice little wicked creepy crawly - and you
may have noted by now: it's brought the family too. :-)


>       else if (buf[is] == '\\' && is == inlen)

another viable alternative for the switch fix. Mind though that you'll
need to fix a few more spots before you're done.

>  As I said, I could be completely off-base and spewing
>  irrelevant garbage (wouldn't be the first time!), but
>  thought I'd provide some input as it sounds like this has
>  become a frustrating issue for many people.

No worries, you're at least 99% ON base.

>  My guess, however, is that a single backslash at the end of
>  strings is what is causing the intermittent error.  That, of
>  course, is probably being caused by something else in the
>  code (which I could believe is a 64-bit issue).

Yep. See description of probable cause (was only a hunch then) earlier
in the thread: it's not that the script puts the backslash there, it's
just some byte cruft left on the stack from previous activity and the
loops are 'nice enough' to test for one byte beyond legal range -
which wasn't caught all this time because, UNLESS that illegal EXTRA
byte is a BACKSLASH, your *length* does not get tweaked, so as long as
the CPU does not 'access violate' on out-of-bounds read accesses -
which they generally don't, you were 'safe' in the sense that the code
would seem to work as expected. Until Jason's box produced a backslash
byte at the proper spot SOME of the time.


>  The alternative to this, of course, is that buf[inlen]
>  is expected to be a null-termination character (making the

See above: nope, it isn't.



Anyway, the thing that got to me yesterday was the alleged
'64-bittiness', while it clearly wasn't. This type of bug doesn't pop
up because you change platform; it's always been there only once
changing platform you suddenly 'find out' something goes boingggg
because the bitpatterns change. I've seen it happen before.

One of the reasons 64-bit boxes can do this for you (= exhibit a
certain issue more often than it's being reported on other platforms)
is that they will produce quite different byte patterns for pointers,
thus filling stack space and everywhere else with a byte mix different
from 32-bit machines. So when you are mistreating your data space (or
pointers - remember old UNIX habits to cast between 'int' and
pointers; ouch!) 64-bit boxes may help you to the grave a little bit
quicker than you were used to. Because 'it just used to work'. Uh-huh.
But I digress.

See also some very spurious bugreports about crm114 in the debian
buglist - good for some head scratching for sure - and my guess is
some of those folks had the same basic issue Jason is having, only
they didn't know what to do next, nor did they know how to 'catch'
such flukes in any possibly consistent manner like we have now. We're
bloody darn lucky Jason got this to a sufficiently repeatable state -
otherwise we'd be pointing at clouds still.

I need a very stiff drink before I get my knickers in a twist again...

Cheers!


-- 
Met vriendelijke groeten / Best regards,

Ger Hobbelt

--------------------------------------------------
web: http://www.hobbelt.com/
 http://www.hebbut.net/
mail: [email protected]
mobile: +31-6-11 120 978
--------------------------------------------------

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone