| Message ID | 1359044517-18243-1-git-send-email-psusi@ubuntu.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, Jan 24, 2013 at 11:21:56AM -0500, Phillip Susi wrote: > ext2fs_llseek() was using lseek instead of lseek64. The > only time it would use lseek64 is if passed an offset that > overflowed 32 bits. This works for SEEK_SET, but not > SEEK_CUR, which can apply a small offset to move the file > pointer past the 32 bit limit. > > The code has been changed to instead try lseek64 first, and > fall back to lseek if that fails. It also was doing a > runtime check of the size of off_t. This has been moved to > compile time. > > Signed-off-by: Phillip Susi <psusi@ubuntu.com> How did you find this? I've done a quick search for SEEK_CUR, and it looks like only place where this could cause a problem is with e2image. And a quick test of a i386 version of e2image with a large file system is that it does indeed blow up with an "Inappropriate ioctl for device" error. Is there any other potential problems that are caused by this bug? I like to explain the impacts of bug fixes in libext2fs for folks who are doing bug fix / code archeology. Thanks, - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 1/24/2013 2:51 PM, Theodore Ts'o wrote: > How did you find this? I've done a quick search for SEEK_CUR, and > it looks like only place where this could cause a problem is with > e2image. And a quick test of a i386 version of e2image with a > large file system is that it does indeed blow up with an > "Inappropriate ioctl for device" error. That's where I found it, but the error should be "seek: Value too large for defined data type" > Is there any other potential problems that are caused by this bug? > I like to explain the impacts of bug fixes in libext2fs for folks > who are doing bug fix / code archeology. If e2image is the only internal user of the call with SEEK_CUR, then I guess it only affects any external users of the library who were doing this ( I am not aware of any ). -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.17 (MingW32) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBAgAGBQJRAZgNAAoJEJrBOlT6nu752g0IAN4czK0LevEa9Mx2htaZ3oms TtcpKe7tuXgmvLB0XWv8Q2f6zCDMH7qmWqWZORdx98jg2XeynaEbZvVIQFEsfOVa 05r7sk64TPGXrdkwuqFTe4M7MYmrQmLnfGzd2NMddlWcXOc+R4pFUrrvYbU7QQv/ SGfb9Uo+EFZhSuJLPY1QW4EO09ghD4k6XHnHtioAsgjyxSa4p2NcZiBxmFT7ZJdw DbET65wHkKT2s+jLQQAOPx/IYAftrZuV7t+YoX7RTS4tdpENlgoVlZ3drCvHB2aG LwDDns4C1e0cAMncGKh3HY4/CInMZ1jFWHYW+VvuzZNJ7Uxur2mG6NUyWp6cNqU= =WeEv -----END PGP SIGNATURE----- -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 24, 2013 at 03:22:37PM -0500, Phillip Susi wrote: > > On 1/24/2013 2:51 PM, Theodore Ts'o wrote: > > How did you find this? I've done a quick search for SEEK_CUR, and > > it looks like only place where this could cause a problem is with > > e2image. And a quick test of a i386 version of e2image with a > > large file system is that it does indeed blow up with an > > "Inappropriate ioctl for device" error. > > That's where I found it, but the error should be "seek: Value too > large for defined data type" Well, I did my testing using an i386 debian/testing chroot running under a x86-64 3.8.0-rc3 kernel. I'm guessing it was the use of a 32-bit userspace / 64-bit kernel that probably explains the difference. > > Is there any other potential problems that are caused by this bug? > > I like to explain the impacts of bug fixes in libext2fs for folks > > who are doing bug fix / code archeology. > > If e2image is the only internal user of the call with SEEK_CUR, then I > guess it only affects any external users of the library who were doing > this ( I am not aware of any ). Well, there are some binaries that aren't usually built by most distributions (make-sparse and copy-sparse), but in terms of primary e2fsprogs programs (mke2fs, e2fskc, tune2fs, chattr, lsattr, etc.) nope, none of them use SEEK_CUR. The lib/ext2fs/fileio.c file does use SEEK_CUR, which means it might impact 3rd party packages such as e2tools and ext2fuse (although that's generally only used on Mac and Windows systems). Cheers, - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 25, 2013 at 10:25:14AM +0800, Zheng Liu wrote: > > This patch makes me consider my patches that dump a sparse file in > debugfs. In my patch [1] llseek64(2) is called to seek to the next > data in target file. So I believe ext2fs_llseek() is a better choice. > I am happy to send a newer patch to fix it. What do you think? > > 1. http://www.spinics.net/lists/linux-ext4/msg36134.html You're right, we should use ext2fs_llseek() for better portability. Thanks, - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 24, 2013 at 03:32:30PM -0500, Theodore Ts'o wrote: > On Thu, Jan 24, 2013 at 03:22:37PM -0500, Phillip Susi wrote: > > > > On 1/24/2013 2:51 PM, Theodore Ts'o wrote: > > > How did you find this? I've done a quick search for SEEK_CUR, and > > > it looks like only place where this could cause a problem is with > > > e2image. And a quick test of a i386 version of e2image with a > > > large file system is that it does indeed blow up with an > > > "Inappropriate ioctl for device" error. > > > > That's where I found it, but the error should be "seek: Value too > > large for defined data type" > > Well, I did my testing using an i386 debian/testing chroot running > under a x86-64 3.8.0-rc3 kernel. I'm guessing it was the use of a > 32-bit userspace / 64-bit kernel that probably explains the > difference. > > > > Is there any other potential problems that are caused by this bug? > > > I like to explain the impacts of bug fixes in libext2fs for folks > > > who are doing bug fix / code archeology. > > > > If e2image is the only internal user of the call with SEEK_CUR, then I > > guess it only affects any external users of the library who were doing > > this ( I am not aware of any ). > > Well, there are some binaries that aren't usually built by most > distributions (make-sparse and copy-sparse), but in terms of primary > e2fsprogs programs (mke2fs, e2fskc, tune2fs, chattr, lsattr, etc.) > nope, none of them use SEEK_CUR. > > The lib/ext2fs/fileio.c file does use SEEK_CUR, which means it might > impact 3rd party packages such as e2tools and ext2fuse (although > that's generally only used on Mac and Windows systems). Hi Ted, This patch makes me consider my patches that dump a sparse file in debugfs. In my patch [1] llseek64(2) is called to seek to the next data in target file. So I believe ext2fs_llseek() is a better choice. I am happy to send a newer patch to fix it. What do you think? 1. http://www.spinics.net/lists/linux-ext4/msg36134.html Thanks, - Zheng -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 24, 2013 at 09:16:05PM -0500, Theodore Ts'o wrote: > On Fri, Jan 25, 2013 at 10:25:14AM +0800, Zheng Liu wrote: > > > > This patch makes me consider my patches that dump a sparse file in > > debugfs. In my patch [1] llseek64(2) is called to seek to the next > > data in target file. So I believe ext2fs_llseek() is a better choice. > > I am happy to send a newer patch to fix it. What do you think? > > > > 1. http://www.spinics.net/lists/linux-ext4/msg36134.html > > You're right, we should use ext2fs_llseek() for better portability. > Thanks, Yep, the next version will be sent out soon. Regards, - Zheng -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 24, 2013 at 11:21:56AM -0500, Phillip Susi wrote: > ext2fs_llseek() was using lseek instead of lseek64. The > only time it would use lseek64 is if passed an offset that > overflowed 32 bits. This works for SEEK_SET, but not > SEEK_CUR, which can apply a small offset to move the file > pointer past the 32 bit limit. > > The code has been changed to instead try lseek64 first, and > fall back to lseek if that fails. It also was doing a > runtime check of the size of off_t. This has been moved to > compile time. > > Signed-off-by: Phillip Susi <psusi@ubuntu.com> Applied, although I had to make a small change so that in the case where llseek doesn't exist, we need to retry with lseek immediately. Also, in the fallback case, we still need to make the runtime check to assure that the requested offset is less than off_t. On the 1.43 branch it probably makes sense to get rid of the whole mess with my_llseek and just use llseek64 directly. There's a bunch of complexity here which because over a decade ago, glibc had a buggy header files which caused llseek64 to malfunction (which is why I didn't enable use of llseek64 and > 2GB file system support in the first place), and the Debian developer at the time tried to be too clever and shipped a patched version of e2fsprogs which tried to use llseek64 without consulting me first, and then users started losing data and having the file systems corrupted as a result. So as a result, because I couldn't trust glibc to give me a non-buggy version of llseek64, and Debian users were losing data in real time, I had e2fsprogs call the system call directly. It's been a long time since systems with this particular glibc Fail have been around, so eventually I should rip out a lot of this mess. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/configure.in b/configure.in index ac3cd92..181faeb 100644 --- a/configure.in +++ b/configure.in @@ -970,14 +970,17 @@ AC_CHECK_SIZEOF(short) AC_CHECK_SIZEOF(int) AC_CHECK_SIZEOF(long) AC_CHECK_SIZEOF(long long) +AC_CHECK_SIZEOF(off_t) SIZEOF_SHORT=$ac_cv_sizeof_short SIZEOF_INT=$ac_cv_sizeof_int SIZEOF_LONG=$ac_cv_sizeof_long SIZEOF_LONG_LONG=$ac_cv_sizeof_long_long +SIZEOF_OFF_T=$ac_cv_sizeof_off_t AC_SUBST(SIZEOF_SHORT) AC_SUBST(SIZEOF_INT) AC_SUBST(SIZEOF_LONG) AC_SUBST(SIZEOF_LONG_LONG) +AC_SUBST(SIZEOF_OFF_T) AC_C_BIGENDIAN BUILD_CC="$BUILD_CC" CPP="$CPP" /bin/sh $ac_aux_dir/parse-types.sh ASM_TYPES_HEADER=./asm_types.h diff --git a/lib/config.h.in b/lib/config.h.in index 6f88c9c..eebb75d 100644 --- a/lib/config.h.in +++ b/lib/config.h.in @@ -552,6 +552,9 @@ /* The size of `long long', as computed by sizeof. */ #undef SIZEOF_LONG_LONG +/* The size of `off_t', as computed by sizeof. */ +#undef SIZEOF_OFF_T + /* The size of `short', as computed by sizeof. */ #undef SIZEOF_SHORT diff --git a/lib/ext2fs/llseek.c b/lib/ext2fs/llseek.c index b0576e4..e5cb784 100644 --- a/lib/ext2fs/llseek.c +++ b/lib/ext2fs/llseek.c @@ -90,18 +90,14 @@ static ext2_loff_t my_llseek (int fd, ext2_loff_t offset, int origin) ext2_loff_t ext2fs_llseek (int fd, ext2_loff_t offset, int origin) { +#if SIZEOF_OFF_T >= SIZEOF_LONG_LONG + return lseek (fd, offset, origin); +#else ext2_loff_t result; static int do_compat = 0; - if ((sizeof(off_t) >= sizeof(ext2_loff_t)) || - (offset < ((ext2_loff_t) 1 << ((sizeof(off_t)*8) -1)))) + if (do_compat) return lseek(fd, (off_t) offset, origin); - - if (do_compat) { - errno = EINVAL; - return -1; - } - result = my_llseek (fd, offset, origin); if (result == -1 && errno == ENOSYS) { /* @@ -112,6 +108,7 @@ ext2_loff_t ext2fs_llseek (int fd, ext2_loff_t offset, int origin) errno = EINVAL; } return result; +#endif } #else /* !linux */
ext2fs_llseek() was using lseek instead of lseek64. The only time it would use lseek64 is if passed an offset that overflowed 32 bits. This works for SEEK_SET, but not SEEK_CUR, which can apply a small offset to move the file pointer past the 32 bit limit. The code has been changed to instead try lseek64 first, and fall back to lseek if that fails. It also was doing a runtime check of the size of off_t. This has been moved to compile time. Signed-off-by: Phillip Susi <psusi@ubuntu.com> --- configure.in | 3 +++ lib/config.h.in | 3 +++ lib/ext2fs/llseek.c | 13 +++++-------- 3 files changed, 11 insertions(+), 8 deletions(-)