[PATCH] lzexpand: Fix uninitialized read in read_header

Evan Tang etang110 at gmail.com
Mon Jan 27 02:06:27 CST 2020


Just realized a better alternative method of using `!=` instead of `<`, will resubmit with that instead

> 2020/01/26 22:03、Evan Tang <etang110 at gmail.com>のメール:
> 
> read_header calls _lread which can either return the number of characters read
> or HFILE_ERROR (-1), casted to a UINT.  This was tested with a `< taget_length`
> check, which doesn't catch HFILE_ERROR (since this is a UINT), allowing that case
> to slip through
> 
> This could cause issues if LZInit is called in quick succession, first on a file
> with a valid header (to put a valid LZ header in the buffer), then on an unreadable
> file descriptor (a write-only one, for example), depending on if the compiler
> decided to reuse the memory slot in the rest of LZInit
> 
> Signed-off-by: Evan Tang <etang110 at gmail.com>
> ---
> An alternative method (which might be more obvious to someone reading the code) would be
> to store the result of _lread in a variable and then explicitly check it both
> for `< target_length` and `== HFILE_ERROR`, do you think we should do that instead?
> 
> Also, is this something we want regression tests for?  You could try
> the sequence described above, which successfully fails on clang but not gcc
> (due to how they compile the rest of LZInit).  I guess you could also
> make the first file have an invalid compressiontype which would run much less
> LZInit code and be more likely to fail, but in the end you'd still be relying
> on undefined behavior doing something specific
> ---
> dlls/kernel32/lzexpand.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/dlls/kernel32/lzexpand.c b/dlls/kernel32/lzexpand.c
> index 4b2ec2ed45..471dba6c75 100644
> --- a/dlls/kernel32/lzexpand.c
> +++ b/dlls/kernel32/lzexpand.c
> @@ -139,7 +139,7 @@ static INT read_header(HFILE fd,struct lzfileheader *head)
> 	/* We can't directly read the lzfileheader struct due to
> 	 * structure element alignment
> 	 */
> -	if (_lread(fd,buf,LZ_HEADER_LEN)<LZ_HEADER_LEN)
> +	if ((INT)_lread(fd,buf,LZ_HEADER_LEN)<LZ_HEADER_LEN)
> 		return 0;
> 	memcpy(head->magic,buf,LZ_MAGIC_LEN);
> 	memcpy(&(head->compressiontype),buf+LZ_MAGIC_LEN,1);
> -- 
> 2.15.2 (Apple Git-101.1)
> 




More information about the wine-devel mailing list