[sword-devel] crashes with ciphering code

[Martin Gruner]

Hi DM and everybody.

I changed the line to:
	strncpy(*buf, cacheBlock->getEntry(entry), size);
	(*buf)[size]='\0';

But it didn't work. Still crashes.

I believe I found the reason though. As I assumed many months ago, the reading 
process works like this:
decryption -> decompression -> display.

Proof (zstr.cpp):
	unsigned long len = size;
	buf.setSize(size);
	rawZFilter(buf, 0); // 0 = decipher

	compressor->zBuf(&len, buf.getRawData());
	char *rawBuf = compressor->Buf(0, &len);
	cacheBlock = new EntriesBlock(rawBuf, len);
	cacheBlockIndex = block;

That means we're trying to decompress faulty data! Can we at least check for a 
zip signature or something in the decompressor?
This must also be the reason for the frequent warnings
"no room in outbuffer to during decompression. see zipcomp.cpp".

If I have the "flow of events correct" this time, then we have a design 
problem. It would be best to change the flow of events and rebuild the 
modules and release a fixed and incompatible sword version.

Or am I wrong? Please advise.

Martin



Am Montag, 27. November 2006 22:51 schrieb DM Smith:
> Joachim Ansorg wrote:
> >>   zStr::getCompressedText calls
> >>  strcpy(*buf, cacheBlock->getEntry(entry));
> >
> > My fix for this would be (without digging deep into the sources) in line
> > 438 of zstr.cpp:
> > 	strncpy(*buf, cacheBlock->getEntry(entry), size);
>
> This takes me way back. I haven't done serious C coding in years! But I
> remember something about this one. I had been bitten by it several times.
>
> IIRC:
> strncpy will not null terminate the string. Generally its good to follow
> it with buf[size] = '\0';
>
> strncpy will stop copying if it sees '\0' before it reaches the end. In
> that case, strncpy stops after it copies the '\0'.
> It may be good to compare strlen(buf) == size and throw an error if it
> is not.
>
> Putting this together:
> strncpy(*buf, cacheBlock->getEntry(entry), size);
> buf[size] = '\0';
> if (strlen(buf) != size) an error has occurred.
>
> Also, since we expect that the string is copied in its entirety, it is
> probably better and faster to use memcpy. Memmove would also work but
> would be slightly slower than memcpy.
>
> > strcpy expects a \0-terminated string. If the deciphering with the wrong
> > key creates a char* without a proper \0 this would result in an address
> > out of bounds. So the fix is to make sure we just copy the number of
> > bytes which are available in the cacheBlock.
> > I did not yet think whether a \0 has to explicitely be set at the end of
> > *buf.
> >
> > Does this make sense?
> > Does somebody have the setup to test this?
> >
> > Thanks,
> > Joachim
>
> _______________________________________________
> sword-devel mailing list: sword-devel at crosswire.org
> http://www.crosswire.org/mailman/listinfo/sword-devel
> Instructions to unsubscribe/change your settings at above page