| Message ID | 52EF68CA.9060604@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Feb 03, 2014 at 06:00:42PM +0800, Chen Gang wrote: > We can not assume "'path' + 'ctx->fs_root'" must be less than MAX_PATH, > so need use snprintf() instead of sprintf(). > > And also recommend to use ARRAY_SIZE instead of hard code macro for an > array size in snprintf(). In the event that there is overflow this will cause the data to be truncated, potentially causing QEMU to access the wrong file on the host. Both snprintf and sprintf are really bad because of their use of fixed buffers. Better to change it to g_strdup_printf which dynamically allocates buffers. Daniel
On 02/03/2014 06:34 PM, Daniel P. Berrange wrote: > On Mon, Feb 03, 2014 at 06:00:42PM +0800, Chen Gang wrote: >> We can not assume "'path' + 'ctx->fs_root'" must be less than MAX_PATH, >> so need use snprintf() instead of sprintf(). >> >> And also recommend to use ARRAY_SIZE instead of hard code macro for an >> array size in snprintf(). > > In the event that there is overflow this will cause the data to be > truncated, potentially causing QEMU to access the wrong file on the > host. Both snprintf and sprintf are really bad because of their > use of fixed buffers. Better to change it to g_strdup_printf which > dynamically allocates buffers. > That sounds reasonable to me, I will send patch v2 for it. Thanks.
On 02/03/2014 06:39 PM, Chen Gang wrote: > On 02/03/2014 06:34 PM, Daniel P. Berrange wrote: >> On Mon, Feb 03, 2014 at 06:00:42PM +0800, Chen Gang wrote: >>> We can not assume "'path' + 'ctx->fs_root'" must be less than MAX_PATH, >>> so need use snprintf() instead of sprintf(). >>> >>> And also recommend to use ARRAY_SIZE instead of hard code macro for an >>> array size in snprintf(). >> >> In the event that there is overflow this will cause the data to be >> truncated, potentially causing QEMU to access the wrong file on the >> host. Both snprintf and sprintf are really bad because of their >> use of fixed buffers. Better to change it to g_strdup_printf which >> dynamically allocates buffers. >> After check the details, I guess we can not change to g_strdup_printf or others (e.g. v9fs_string_*). v9fs need use "mkdir, remove ..." which have MAX_PATH limitation. So if the combined path is longer than MAX_PATH, before it passes to "mkdir, remove ...", it has to be truncated just like what rpath() has done. So for me, we have to still use snprintf() instead of sprintf(), but really need provide the related comments under each snprintf(). > > That sounds reasonable to me, I will send patch v2 for it. > > > Thanks. > Thanks.
On Tue, Feb 04, 2014 at 07:02:18PM +0800, Chen Gang wrote: > On 02/03/2014 06:39 PM, Chen Gang wrote: > > On 02/03/2014 06:34 PM, Daniel P. Berrange wrote: > >> On Mon, Feb 03, 2014 at 06:00:42PM +0800, Chen Gang wrote: > >>> We can not assume "'path' + 'ctx->fs_root'" must be less than MAX_PATH, > >>> so need use snprintf() instead of sprintf(). > >>> > >>> And also recommend to use ARRAY_SIZE instead of hard code macro for an > >>> array size in snprintf(). > >> > >> In the event that there is overflow this will cause the data to be > >> truncated, potentially causing QEMU to access the wrong file on the > >> host. Both snprintf and sprintf are really bad because of their > >> use of fixed buffers. Better to change it to g_strdup_printf which > >> dynamically allocates buffers. > >> > > After check the details, I guess we can not change to g_strdup_printf or > others (e.g. v9fs_string_*). > > v9fs need use "mkdir, remove ..." which have MAX_PATH limitation. So if > the combined path is longer than MAX_PATH, before it passes to "mkdir, > remove ...", it has to be truncated just like what rpath() has done. I don't believe you are correct there. Those functions should return "errno == ENAMETOOLONG - pathname was too long". The MAX_PATH constant is not even required to exist in POSIX, so I would not expect the spec to mandate anything about MAX_PATH in relation to those functions. Copying Eric who is involved the POSIX spec group to confirm. Even if they are limited, it is still better practice to use dynamic allocation for this, over fixed length buffers IMHO. Daniel
On 02/04/2014 07:06 PM, Daniel P. Berrange wrote: > On Tue, Feb 04, 2014 at 07:02:18PM +0800, Chen Gang wrote: >> On 02/03/2014 06:39 PM, Chen Gang wrote: >>> On 02/03/2014 06:34 PM, Daniel P. Berrange wrote: >>>> On Mon, Feb 03, 2014 at 06:00:42PM +0800, Chen Gang wrote: >>>>> We can not assume "'path' + 'ctx->fs_root'" must be less than MAX_PATH, >>>>> so need use snprintf() instead of sprintf(). >>>>> >>>>> And also recommend to use ARRAY_SIZE instead of hard code macro for an >>>>> array size in snprintf(). >>>> >>>> In the event that there is overflow this will cause the data to be >>>> truncated, potentially causing QEMU to access the wrong file on the >>>> host. Both snprintf and sprintf are really bad because of their >>>> use of fixed buffers. Better to change it to g_strdup_printf which >>>> dynamically allocates buffers. >>>> >> >> After check the details, I guess we can not change to g_strdup_printf or >> others (e.g. v9fs_string_*). >> >> v9fs need use "mkdir, remove ..." which have MAX_PATH limitation. So if >> the combined path is longer than MAX_PATH, before it passes to "mkdir, >> remove ...", it has to be truncated just like what rpath() has done. > > I don't believe you are correct there. Those functions should > return "errno == ENAMETOOLONG - pathname was too long". The > MAX_PATH constant is not even required to exist in POSIX, so > I would not expect the spec to mandate anything about MAX_PATH > in relation to those functions. > So the original author of v9fs will use truncation instead of return failure to upper users. I guess, we need discuss what original author have done is valuable or not (at least, it's necessary to mention about it in related documents). And at least, we are agree with each other: "using sprintf() is a bug". > Copying Eric who is involved the POSIX spec group to confirm. > > Even if they are limited, it is still better practice to use > dynamic allocation for this, over fixed length buffers IMHO. > > Daniel > Welcome any members' suggestions, discussion or completions. :-) Thanks.
Chen Gang <gang.chen.5i5j@gmail.com> writes: > On 02/03/2014 06:39 PM, Chen Gang wrote: >> On 02/03/2014 06:34 PM, Daniel P. Berrange wrote: >>> On Mon, Feb 03, 2014 at 06:00:42PM +0800, Chen Gang wrote: >>>> We can not assume "'path' + 'ctx->fs_root'" must be less than MAX_PATH, >>>> so need use snprintf() instead of sprintf(). >>>> >>>> And also recommend to use ARRAY_SIZE instead of hard code macro for an >>>> array size in snprintf(). >>> >>> In the event that there is overflow this will cause the data to be >>> truncated, potentially causing QEMU to access the wrong file on the >>> host. Both snprintf and sprintf are really bad because of their >>> use of fixed buffers. Better to change it to g_strdup_printf which >>> dynamically allocates buffers. >>> > > After check the details, I guess we can not change to g_strdup_printf or > others (e.g. v9fs_string_*). > > v9fs need use "mkdir, remove ..." which have MAX_PATH limitation. So if > the combined path is longer than MAX_PATH, before it passes to "mkdir, > remove ...", it has to be truncated just like what rpath() has done. What good could truncating possibly do? If the pathname is too long for mkdir(), truncating won't make it work, it will make it do the wrong thing. Second guessing when a pathname is too long for a system call is not a good idea. If it's too long, the system call will tell you. As Dan noted, PATH_MAX is *not* a hard limit. {PATH_MAX} Maximum number of bytes the implementation will store as a pathname in a user-supplied buffer of unspecified size, including the terminating null character. Minimum number the implementation will accept as the maximum number of bytes in a pathname. [...]
On 02/04/2014 04:06 AM, Daniel P. Berrange wrote: >> >> v9fs need use "mkdir, remove ..." which have MAX_PATH limitation. So if >> the combined path is longer than MAX_PATH, before it passes to "mkdir, >> remove ...", it has to be truncated just like what rpath() has done. > > I don't believe you are correct there. Those functions should > return "errno == ENAMETOOLONG - pathname was too long". The > MAX_PATH constant is not even required to exist in POSIX, so > I would not expect the spec to mandate anything about MAX_PATH > in relation to those functions. > > Copying Eric who is involved the POSIX spec group to confirm. > Correct - POSIX intentionally allows GNU Hurd behavior which is no MAX_PATH. You can use openat() and friends to reduce the path length of an operation you are trying, and there, your limit becomes NAME_MAX (255 on many filesystems, but also a value that POSIX allows to be undefined). POSIX merely requires that the system be consistent - it cannot toggle between ENAMETOOLONG errors through one interface and acting on the name through another. > Even if they are limited, it is still better practice to use > dynamic allocation for this, over fixed length buffers IMHO. Agreed on two counts - dynamic allocation is essential on platforms where MAX_PATH is undefined and thus where path names can be constructed that occupy longer than a system page (because you do NOT want stack allocations longer than a page); and you WANT to try the system call because an ENAMETOOLONG from the system is definitive whereas giving up early because you only guess that it will be too long or because you used snprintf and truncated the string generally causes confusion.
On 02/04/2014 05:25 AM, Markus Armbruster wrote: > Second guessing when a pathname is too long for a system call is not a > good idea. If it's too long, the system call will tell you. As Dan > noted, PATH_MAX is *not* a hard limit. > > {PATH_MAX} > Maximum number of bytes the implementation will store as a > pathname in a user-supplied buffer of unspecified size, > including the terminating null character. Minimum number the > implementation will accept as the maximum number of bytes in a > pathname. Linux allows unbelievably long absolute names. Jim Meyering proved with coreutils that you can create an absolute name well over a megabyte in length. The trick is that you have to access it via relative names where each relative name is PATH_MAX or less (that is, the Linux kernel refuses to operate on more than a page at a time when doing file name resolution), by using openat() and friends. mkdirat() can create a directory with an absolute name longer than PATH_MAX, even if mkdir() can't.
On 02/04/2014 09:12 PM, Eric Blake wrote: > On 02/04/2014 05:25 AM, Markus Armbruster wrote: > >> Second guessing when a pathname is too long for a system call is not a >> good idea. If it's too long, the system call will tell you. As Dan >> noted, PATH_MAX is *not* a hard limit. >> >> {PATH_MAX} >> Maximum number of bytes the implementation will store as a >> pathname in a user-supplied buffer of unspecified size, >> including the terminating null character. Minimum number the >> implementation will accept as the maximum number of bytes in a >> pathname. > > Linux allows unbelievably long absolute names. Jim Meyering proved with > coreutils that you can create an absolute name well over a megabyte in > length. The trick is that you have to access it via relative names > where each relative name is PATH_MAX or less (that is, the Linux kernel > refuses to operate on more than a page at a time when doing file name > resolution), by using openat() and friends. mkdirat() can create a > directory with an absolute name longer than PATH_MAX, even if mkdir() can't. > OK, thank all of you, what you said sound reasonable to me. I don't know why the original author/maintainer did not support 'unlimited' path, so better to get original authors' opinion. And can we split current discussion into 2 pieces: - fix sprintf() bug, can use snprintf() to fix it just like other places have done -- apply this patch (comments need be improved). - improve 9pfs features -- support 'unlimited' path internally. before do it, better to get original authors' response firstly. I guess, we need change quite a few areas and have a full test. Thanks.
Chen Gang <gang.chen.5i5j@gmail.com> writes: > On 02/04/2014 07:06 PM, Daniel P. Berrange wrote: >> On Tue, Feb 04, 2014 at 07:02:18PM +0800, Chen Gang wrote: >>> On 02/03/2014 06:39 PM, Chen Gang wrote: >>>> On 02/03/2014 06:34 PM, Daniel P. Berrange wrote: >>>>> On Mon, Feb 03, 2014 at 06:00:42PM +0800, Chen Gang wrote: >>>>>> We can not assume "'path' + 'ctx->fs_root'" must be less than MAX_PATH, >>>>>> so need use snprintf() instead of sprintf(). >>>>>> >>>>>> And also recommend to use ARRAY_SIZE instead of hard code macro for an >>>>>> array size in snprintf(). >>>>> >>>>> In the event that there is overflow this will cause the data to be >>>>> truncated, potentially causing QEMU to access the wrong file on the >>>>> host. Both snprintf and sprintf are really bad because of their >>>>> use of fixed buffers. Better to change it to g_strdup_printf which >>>>> dynamically allocates buffers. >>>>> >>> >>> After check the details, I guess we can not change to g_strdup_printf or >>> others (e.g. v9fs_string_*). >>> >>> v9fs need use "mkdir, remove ..." which have MAX_PATH limitation. So if >>> the combined path is longer than MAX_PATH, before it passes to "mkdir, >>> remove ...", it has to be truncated just like what rpath() has done. >> >> I don't believe you are correct there. Those functions should >> return "errno == ENAMETOOLONG - pathname was too long". The >> MAX_PATH constant is not even required to exist in POSIX, so >> I would not expect the spec to mandate anything about MAX_PATH >> in relation to those functions. >> > > So the original author of v9fs will use truncation instead of return > failure to upper users. That is a bug. The snprintf usage with PATH_MAX is to prevent buffer overflow and not to truncate. I guess we should fix path handling and propagate error correctly. -aneesh
On 02/05/2014 12:18 AM, Aneesh Kumar K.V wrote: > Chen Gang <gang.chen.5i5j@gmail.com> writes: > >> On 02/04/2014 07:06 PM, Daniel P. Berrange wrote: >>> On Tue, Feb 04, 2014 at 07:02:18PM +0800, Chen Gang wrote: >>>> On 02/03/2014 06:39 PM, Chen Gang wrote: >>>>> On 02/03/2014 06:34 PM, Daniel P. Berrange wrote: >>>>>> On Mon, Feb 03, 2014 at 06:00:42PM +0800, Chen Gang wrote: >>>>>>> We can not assume "'path' + 'ctx->fs_root'" must be less than MAX_PATH, >>>>>>> so need use snprintf() instead of sprintf(). >>>>>>> >>>>>>> And also recommend to use ARRAY_SIZE instead of hard code macro for an >>>>>>> array size in snprintf(). >>>>>> >>>>>> In the event that there is overflow this will cause the data to be >>>>>> truncated, potentially causing QEMU to access the wrong file on the >>>>>> host. Both snprintf and sprintf are really bad because of their >>>>>> use of fixed buffers. Better to change it to g_strdup_printf which >>>>>> dynamically allocates buffers. >>>>>> >>>> >>>> After check the details, I guess we can not change to g_strdup_printf or >>>> others (e.g. v9fs_string_*). >>>> >>>> v9fs need use "mkdir, remove ..." which have MAX_PATH limitation. So if >>>> the combined path is longer than MAX_PATH, before it passes to "mkdir, >>>> remove ...", it has to be truncated just like what rpath() has done. >>> >>> I don't believe you are correct there. Those functions should >>> return "errno == ENAMETOOLONG - pathname was too long". The >>> MAX_PATH constant is not even required to exist in POSIX, so >>> I would not expect the spec to mandate anything about MAX_PATH >>> in relation to those functions. >>> >> >> So the original author of v9fs will use truncation instead of return >> failure to upper users. > > > That is a bug. The snprintf usage with PATH_MAX is to prevent buffer > overflow and not to truncate. I guess we should fix path handling > and propagate error correctly. > > -aneesh > OK, thank you for your opinion and confirmation. I will/should send patch v2 for it (use 'unlimited' path and propagate error correctly). And excuse me, I have no enough time to focus on it, so I plan to send patch v2 for reviewing within this month (2014-02-28). If we can not bear this time point, please help send patch for it, thanks. And also excuse me, I am a newbie for 9pfs, also a newbie for qemu (I found it by reading source code), the patch v2 needs a test for 9pfs, so welcome any suggestions/informations about 9pfs test. Thanks
diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c index fc93e9e..44a0380 100644 --- a/hw/9pfs/virtio-9p-local.c +++ b/hw/9pfs/virtio-9p-local.c @@ -159,7 +159,7 @@ static int local_create_mapped_attr_dir(FsContext *ctx, const char *path) char attr_dir[PATH_MAX]; char *tmp_path = g_strdup(path); - snprintf(attr_dir, PATH_MAX, "%s/%s/%s", + snprintf(attr_dir, ARRAY_SIZE(attr_dir), "%s/%s/%s", ctx->fs_root, dirname(tmp_path), VIRTFS_META_DIR); err = mkdir(attr_dir, 0700); @@ -898,7 +898,8 @@ static int local_remove(FsContext *ctx, const char *path) * directory
We can not assume "'path' + 'ctx->fs_root'" must be less than MAX_PATH, so need use snprintf() instead of sprintf(). And also recommend to use ARRAY_SIZE instead of hard code macro for an array size in snprintf(). Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> --- hw/9pfs/virtio-9p-local.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) */ if (S_ISDIR(stbuf.st_mode)) { - sprintf(buffer, "%s/%s/%s", ctx->fs_root, path, VIRTFS_META_DIR); + snprintf(buffer, ARRAY_SIZE(buffer), "%s/%s/%s", + ctx->fs_root, path, VIRTFS_META_DIR); err = remove(buffer); if (err < 0 && errno != ENOENT) { /* @@ -1033,8 +1034,8 @@ static int local_unlinkat(FsContext *ctx, V9fsPath *dir, * If directory remove .virtfs_metadata contained in the * directory */ - sprintf(buffer, "%s/%s/%s", ctx->fs_root, - fullname.data, VIRTFS_META_DIR); + snprintf(buffer, ARRAY_SIZE(buffer), "%s/%s/%s", ctx->fs_root, + fullname.data, VIRTFS_META_DIR); ret = remove(buffer); if (ret < 0 && errno != ENOENT) { /*