The following pull request was submitted through Github. It can be accessed and reviewed at: https://github.com/lxc/lxc/pull/2545
This e-mail was sent by the LXC bot, direct replies will not reach the author unless they happen to be subscribed to this list. === Description (from pull-request) === The issue was introduced in PR (https://github.com/lxc/lxc/pull/1705): Previous code: ``` if (lseek(fd, size, SEEK_SET) < 0) { SYSERROR("Error seeking to set new loop file size"); close(fd); return -1; } ``` New code: ``` int fd, ret; [...] ret = lseek(fd, size, SEEK_SET); if (ret < 0) { SYSERROR("Failed to seek to set new loop file size for loop " "file \"%s\"", path); close(fd); return -1; } ``` Based on http://man7.org/linux/man-pages/man2/lseek.2.html: > Upon successful completion, lseek() returns the resulting offset location as measured in bytes from the beginning of the file. Because `size` is `uint64_t`, it's very easy to overflow `res` and as a result the code will success randomly (if overflow is >0 it will pass it it's <0 it will fail). Example from strace: ``` [...] [pid 5561] lseek(4, 53687091200, SEEK_SET) = 53687091200 [pid 5561] close(4) = 0 [pid 5561] exit_group(1) = ? [pid 5561] +++ exited with 1 +++ [...] ``` There are many ways how this issue can be fixed. `ret` can be change from `int` to `uint64_t`, another solution will be dedicated variable for `lseek` return. IMHO reverting syntax to the previous version seems to be the cleanest solution. Let me know what works for you. This PR fix issues (https://github.com/lxc/lxc/issues/1872).
From 087aab8fa9dd353c9018e977e8e79d1736996653 Mon Sep 17 00:00:00 2001 From: Lukasz Jagiello <[email protected]> Date: Fri, 17 Aug 2018 21:11:02 -0700 Subject: [PATCH] storage/loop.c: integer overflow The issue was introduced in PR (https://github.com/lxc/lxc/pull/1705): Previous code: ``` if (lseek(fd, size, SEEK_SET) < 0) { SYSERROR("Error seeking to set new loop file size"); close(fd); return -1; } ``` New code: ``` int fd, ret; [...] ret = lseek(fd, size, SEEK_SET); if (ret < 0) { SYSERROR("Failed to seek to set new loop file size for loop " "file \"%s\"", path); close(fd); return -1; } ``` Based on http://man7.org/linux/man-pages/man2/lseek.2.html: > Upon successful completion, lseek() returns the resulting offset > location as measured in bytes from the beginning of the file. Because `size` is `uint64_t`, it's very easy to overflow `res` and as a result the code will pass randomly (if overflow is >0 it will pass it it's <0 it will fail). Example from strace: ``` [...] [pid 5561] lseek(4, 53687091200, SEEK_SET) = 53687091200 [pid 5561] close(4) = 0 [pid 5561] exit_group(1) = ? [pid 5561] +++ exited with 1 +++ [...] ``` There are many ways how this issue can be fixed. `ret` can be change from `int` to `uint64_t`, another solution will be dedicated variable for lseek return. IMHO reverting syntax to the previous version seems to be the cleanest solution. Let me know what works for you. This PR fix issues (https://github.com/lxc/lxc/issues/1872). --- src/lxc/storage/loop.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/lxc/storage/loop.c b/src/lxc/storage/loop.c index c4d393452..04101d9a8 100644 --- a/src/lxc/storage/loop.c +++ b/src/lxc/storage/loop.c @@ -307,8 +307,7 @@ static int do_loop_create(const char *path, uint64_t size, const char *fstype) return -1; } - ret = lseek(fd, size, SEEK_SET); - if (ret < 0) { + if (lseek(fd, size, SEEK_SET) < 0) { SYSERROR("Failed to seek to set new loop file size for loop " "file \"%s\"", path); close(fd);
_______________________________________________ lxc-devel mailing list [email protected] http://lists.linuxcontainers.org/listinfo/lxc-devel
