On Tue, 5 Feb 2002 lawson_whitney at juno.com wrote:
[...]
> This fixes the problem I reported. Maybe we should make the filename
> longer to accomodate the trailing \0, maybe because it is a fixed
> length, the trailing \0 is gratuitous, but as it was, that stepped all
> over bigBlockSizeBits, generally disrupted my app, and looks sort of
> like an off by one error. I'll attach a copy in case your mailer folds
> it.
I don't know what the code is supposed to do so I cannot comment much
on your patch. But it looks reasonable. But this file contains quite a
few other cases that are definitely not reasonable:
Context:
struct StorageImpl { ...
WCHAR filename[PROPERTY_NAME_BUFFER_LEN];
.. };
* in StorageBaseImpl_RenameElement()
StorageBaseImpl_CreateStream()
StorageImpl_CreateStorage()
renamedProperty.sizeOfNameString =
( lstrlenW(pwcsNewName)+1 ) * sizeof(WCHAR);
if (renamedProperty.sizeOfNameString > PROPERTY_NAME_BUFFER_LEN)
return STG_E_INVALIDNAME;
strcpyW(renamedProperty.name, pwcsNewName);
Should not cause crashes but shouldn't the test be:
renamedProperty.sizeOfNameString > PROPERTY_NAME_BUFFER_LEN*sizeof(WCHAR)
Although this is a contrieved way to put it.
* StorageImpl_ReadProperty()
WCHAR *propName = (index == This->rootPropertySetIndex) ?
This->filename : (WCHAR *)currentProperty+OFFSET_PS_NAME;
memset(buffer->name, 0, sizeof(buffer->name));
memcpy(
buffer->name,
propName,
PROPERTY_NAME_BUFFER_LEN );
- Do we really have a garantee that there will be
PROPERTY_NAME_BUFFER_LEN bytes available for memcpy to copy?
(grrr, just saw that OFFSET_PS_NAME==0, still you either suppose
it's 0 or not)
- Furthermore since this is bytes, there could be as much as
2*PROPERTY_NAME_BUFFER_LEN bytes to copy!
- What's the point of the memset by the way?
-> a unicode strncpy seems indicated
* StorageImpl_WriteProperty
-> pretty much the same problem
--
Francois Gouget fgouget at free.frhttp://fgouget.free.fr/
Cahn's Axiom: When all else fails, read the instructions.