-log causes blat.exe to crash on XP and 2003 Server

Logged In: NO

Same problem exist on Windows 2000 Server with Version 2.6.2

Logged In: YES
user_id=800692
Originator: NO

Ah, the answer is in your last line, the failure is caused by MSVCRT.DLL. Here is the file I use with my WinXP SP2: http://home.att.net/~chip.programmer/blat/msvcrt.dll

OPTIONS.CPP 1712

memcpy(bodyFilename, argv[this_arg+1], sizeof(bodyFilename)-1 );

OPTIONS.CPP 2464
memcpy( logFile, argv[this_arg+1], sizeof(logFile) - 1 );

these lines are cause of crash.
the source size is not taken into account.
so sometimes we have crash when using -log and <filename>.
they should be changed to
memset(bodyFilename, 0, sizeof(bodyFilename));
memcpy(bodyFilename, argv[this_arg+1], min(sizeof(bodyFilename)-1, strlen(bodyFilename)) );

and

memset(logFile, 0, sizeof(logFile));
memcpy( logFile, argv[this_arg+1], min(sizeof(logFile) - 1, strlen(argv[this_arg+1])));
respectively.

memcpy(bodyFilename, argv[this_arg+1], min(sizeof(bodyFilename)-1, strlen(argv[this_arg+1])) );
of course

Let's look at the two routines to see what they are doing, and their associated character arrays:

extern char bodyFilename[_MAX_PATH];
static char logFile[_MAX_PATH];

static int checkBodyFile ( int argc, char ** argv, int this_arg, int startargv )
{
argc = argc; // For eliminating compiler warnings.
startargv = startargv;

memcpy(bodyFilename, argv[this_arg+1], sizeof(bodyFilename)-1 );
bodyFilename[sizeof(bodyFilename)-1] = 0;

return(1);
}

static int checkLogMessages ( int argc, char ** argv, int this_arg, int startargv )
{
argc = argc; // For eliminating compiler warnings.
startargv = startargv;

if ( logOut )
fclose( logOut );

memcpy( logFile, argv[this_arg+1], sizeof(logFile) - 1 );
logFile[sizeof(logFile) - 1] = 0;

if ( clearLogFirst )
logOut = fopen(logFile, "w");
else
logOut = fopen(logFile, "a");

// if all goes well the file is closed normally
// if we return before closing the library SHOULD
// close it for us..

return(1);
}

As you can see, these two character arrays have been defined to be of _MAX_PATH length.

If we look at the actual assembly code that was generated, we see the follow for checkBodyFile():

0000 ?checkBodyFile@@YAHHPAPADHH@Z:
0000 8B 44 24 0C mov eax,dword ptr 0xc[esp]
0004 8B 4C 24 08 mov ecx,dword ptr 0x8[esp]
0008 68 03 01 00 00 push 0x00000103
000D FF 74 81 04 push dword ptr 0x4[ecx+eax*4]
0011 68 00 00 00 00 push ?bodyFilename@@3PADA
0016 E8 00 00 00 00 call j^_memcpy
001B 33 C0 xor eax,eax
001D 83 C4 0C add esp,0x0000000c
0020 C6 05 03 01 00 00 00 mov byte ptr ?bodyFilename@@3PADA+0x103,0x00
0027 40 inc eax
0028 C3 ret

According to the memcpy() statement, there should be _MAX_PATH-1 *bytes* copied from argv[this_arg+1] into bodyFilename[], as represented by "sizeof(bodyFilename)-1". Since _MAX_PATH is defined as 260 by Windows, and 260 decimal is the same as 0x00000104 in hexadecimal, _MAX_PATH-1 is then represented as 0x00000103 that can be seen in the assembly code above. This means that your comments about the size of the target (bodyFilename) is inaccurate. The truth is that the size of the receiving array *is* accounted for.

When we view the assembly code generated for checkLogMessages(), we see similar code:

0000 ?checkLogMessages@@YAHHPAPADHH@Z:
0000 A1 00 00 00 00 mov eax,dword ptr ?logOut@@3PAU_iobuf@@A
0005 85 C0 test eax,eax
0007 74 08 je X$208
0009 50 push eax
000A FF 15 00 00 00 00 call dword ptr __imp__fclose
0010 59 pop ecx
0011 X$208:
0011 8B 44 24 0C mov eax,dword ptr 0xc[esp]
0015 8B 4C 24 08 mov ecx,dword ptr 0x8[esp]
0019 56 push esi
001A 68 03 01 00 00 push 0x00000103
001F FF 74 81 04 push dword ptr 0x4[ecx+eax*4]
0023 BE 00 00 00 00 mov esi,_logFile
0028 56 push esi
0029 E8 00 00 00 00 call j^_memcpy
002E 83 C4 0C add esp,0x0000000c
0031 80 3D 00 00 00 00 00 cmp byte ptr ?clearLogFirst@@3DA,0x00
0038 C6 05 03 01 00 00 00 mov byte ptr _logFile+0x103,0x00
003F 74 07 je X$209
0041 68 00 00 00 00 push ??_C@_01NOFIACDB@w?$AA@
0046 EB 05 jmp X$210
0048 X$209:
0048 68 00 00 00 00 push ??_C@_01MCMALHOG@a?$AA@
004D X$210:
004D 56 push esi
004E FF 15 00 00 00 00 call dword ptr __imp__fopen
0054 59 pop ecx
0055 A3 00 00 00 00 mov dword ptr ?logOut@@3PAU_iobuf@@A,eax
005A 59 pop ecx
005B 33 C0 xor eax,eax
005D 40 inc eax
005E 5E pop esi
005F C3 ret

In both of these routines, what I do not have is any code to validate argv[this_arg+1] to be non-zero or that the first byte of that argument is non-zero. I will create version 2.7.1 in a moment with this change, and post it to my download site at http://home.att.net/~chip.programmer/blat/v2.71/blat.exe

Chip

No.
My comment is about source size, not target.

"According to the memcpy() statement, there should be _MAX_PATH-1 *bytes*
copied from argv[this_arg+1] into bodyFilename[], as represented by
"sizeof(bodyFilename)-1"."
right. but argv[this_arg+1] is not _MAX_PATH-1 in length. so we read some bytes after end of argv[this_arg+1]. and when argv[this_arg+1] is close to end of page then we get error "Instruction "read" could not be executed".
may be you should use something like
strncpy(bodyFilename, argv[this_arg+1], sizeof(bodyFilename)-1 );
.

Dima

The original message made it seem like you were referring to the target array, not the source array. I will change the memcpy() to strncpy() based on the argument that your source array might be closer to the end of a Windows page than the size of my target array. Thank you for the added clarification in your first line. :)

Chip