lua-users home
lua-l archive

[Date Prev][Date Next][Thread Prev][Thread Next] [Date Index] [Thread Index]


It was thus said that the Great Sir Pogsalot once stated:
> 
> Sean: So... I should get the other person to change their code to avoid the
> rawlen() to allow for evil users to segfault from within their library?
>  Personally I don't understand how you would not typecheck with a
> luaL_checkudata() and not do the responsible rawlen() size check...  That's
> not defensive programming that's just reasonable. :\

  I think an illustration or two is in order here.  A bit of code I had at
one point went like:

	src    = *psrc;
	peos   = memchr(src,eos,SIZET_MAX);   /* doesn't work on DEC Alpha */
	assert(peos   != NULL);
	pdelim = memchr(src,delim,peos-src);
	assert(pdelim != NULL);

  SIZET_MAX is defined as (size_t)-1---the intent was that I *know* that the
character (passed in as eos) is there, because it's added if it's missing; I
just don't know where it appears, and I don't want to call strlen() on the
string as I'd rather not waste the time doing so.

  The issue was that the operating system on the DEC Alpha (Linux, but this
was ... oh ... twelve years ago?  Thirteen?) was trying to be too clever
about the size parameter and returning NULL when it saw what it felt was a
bogus value.  To me, that's not bogus, I'm telling it to scan the entire
memory space.  Don't try to protect me from myself here memchr()---that's
not your job.  If I get a SIGSEGV, it's my own fault.

  Now, you may think that passing 4294967295 (for a 32-bit system) or
18446744073709551615 (for a 64-bit system) is silly, as there's no possible
way I'll ever have 4G or 18 zettabytes (I think that's correct) of RAM in a
system.  Be that as it may, I do *have* a reason for using that value; I'm
not being evil or even stupid (not that the code ever ran on a DEC Alpha but
it never hurts to test it on as many systems as possible).

  Now, as it turned out, that code ran fine for over 12 years until it
dumped core, just out of the blue.  This issue was that pdelim *WAS* NULL! 
That the other character (passed in a delim) didn't exist.  Not all the time
(it was very sproadic) and it wasn't a complete show stopper.  But the thing
is:

	pdelim = memchr(src,delim,peos-src);
	if (pdelim == NULL) 
	 ... um ... ???

  Yeah, it didn't make sense.  That condition *MUST* be true.  And as one of
my college instructors said, "If you don't know how to handle an error
condition, don't check for it," [1] I didn't bother to check that it could
be false (but I did put in an assert(), but even had I not, it still would
have seg faulted on the next line anyway).  And for 12 years, it *WAS* true.  

  Until it wasn't.  

  Okay, the fix was rather easy once I found the exact triggering mechanism
(helped by some additional code [2]).  A blanket 

	if (pdelim == NULL)
	  return NULL;

would have simply hidden the error.  And that wouldn't have been the right
thing to do anyway (that in and of itself would have been a bug).  And at
the time, I wouldn't have know what the right thing to do was (since I
wasn't expecting it to fail in that way). [3]

  -spc (Okay, back to things Lua now ... )

[1]	At the time, I felt he was being flippant.  Turns out no, that's
	pretty sound advice.  If you don't know how to handle an error, what
	the hell are you going to do?

[2]	https://github.com/spc476/CGILib/blob/master/src/crashreport.c

	It helped in the presence of no core files.

[3]	In fact, I wouldn't have even noticed the assert() was failing until
	I added the addtional code [2] to track down another issue.  Which
	shows just how minor the bug was.

	But it was still a bug.