[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