Created attachment 162714 [details] proposed fix When bsdiff opens binary files, it does not properly check if the file size (off_t 64 bit) is larger than theoretical maximum memory size (size_t 64 or just 32 bit). Files that large won't work with the current code, because they clearly run out of memory. But due to +1 calculation, the integer value can overflow and binary data is written into unallocated memory (0 bytes), crashing the application. The fix is simple: If file is too large, handle it as an I/O error.
It should be (SIZE_MAX - 1) not ~0
(In reply to Andrey A. Chernov from comment #1) I.e (oldsize>(SIZE_MAX - 1))
Comment on attachment 162714 [details] proposed fix This fix is not pretty. 1) It does not set errno (EFBIG) 2) Next overflow is few lines below: if(((I=malloc((oldsize+1)*sizeof(off_t)))==NULL) || ((V=malloc((oldsize+1)*sizeof(off_t)))==NULL)) err(1,NULL);
A commit references this bug: Author: ache Date: Tue Nov 3 09:50:11 UTC 2015 New revision: 290329 URL: https://svnweb.freebsd.org/changeset/base/290329 Log: Use meaningful errno for ssize_t overflow in read(). Catch size_t overflow in malloc(). PR: 204230 MFC after: 1 week Changes: head/usr.bin/bsdiff/bsdiff/bsdiff.c
A commit references this bug: Author: ache Date: Tue Nov 3 17:27:25 UTC 2015 New revision: 290336 URL: https://svnweb.freebsd.org/changeset/base/290336 Log: Check for (old|new)size + 1 overflows off_t. PR: 204230 MFC after: 1 week Changes: head/usr.bin/bsdiff/bsdiff/bsdiff.c
Thanks for your review and adjustments to fit FreeBSD's infrastructure, and spotting the multiplication overflow! Why do you have added these latest checks though? Isn't the check for SSIZE_MAX enough, because SSIZE_MAX is smaller than SIZE_T_MAX? And even if OFF_MAX is smaller than SSIZE_MAX, there would be no integer overflow. At least that's how I see it, would be nice to hear your reasoning.
(In reply to tobias from comment #6) I write this check keeping portability in mind, it means any POSIX compliant system, not FreeBSD specifically. POSIX, introducing off_t and ssize_t, does not guarantee any relations between (s)size_t and off_t. In theoretical worst case (s)size_t can be even bigger, in practical case they all (including ssize_t and size_t) can be equally big. No standard guarantees that ssize_t should be smaller than size_t too. But don't worry, modern C compilers auto-optimize all constant comparison conditions to single smallest one.
BTW, bsdiff code can greatly reduce memory (malloc) requirements on some architectures by using ssize_t offsets internally instead of off_t offsets, but I am too lazy to rewrite it in such large scale.
A commit references this bug: Author: ache Date: Sun Nov 8 14:22:57 UTC 2015 New revision: 290546 URL: https://svnweb.freebsd.org/changeset/base/290546 Log: MFC: r290329,r290336 PR: 204230 r290329: Use meaningful errno for ssize_t overflow in read(). Catch size_t overflow in malloc(). r290336: Check for (old|new)size + 1 overflows off_t. Changes: _U stable/10/ stable/10/usr.bin/bsdiff/bsdiff/bsdiff.c