Message ID | 87lfg6zdl5.wl-chenli@uniontech.com |
---|---|
State | Rejected |
Headers |
Return-Path: <libc-alpha-bounces@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 3F7E63953CE6; Fri, 16 Oct 2020 10:11:37 +0000 (GMT) X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from vipregular1.263.net (vipregular1.263.net [211.150.80.21]) by sourceware.org (Postfix) with ESMTPS id 481373857817 for <libc-alpha@sourceware.org>; Fri, 16 Oct 2020 10:11:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 481373857817 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=uniontech.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=chenli@uniontech.com Received: from localhost (unknown [192.168.167.8]) by vipregular1.263.net (Postfix) with ESMTP id 974EC2F1 for <libc-alpha@sourceware.org>; Fri, 16 Oct 2020 18:11:26 +0800 (CST) X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-ADDR-CHECKED4: 1 X-ANTISPAM-LEVEL: 2 X-SKE-CHECKED: 1 X-ABS-CHECKED: 1 Received: from manjaro.uniontech.com (unknown [58.246.122.242]) by smtp.263.net (postfix) whith ESMTP id P2454T140206485231360S1602843086045065_; Fri, 16 Oct 2020 18:11:26 +0800 (CST) X-IP-DOMAINF: 1 X-UNIQUE-TAG: <1730ec5195ee4172272d2e969afcec2a> X-RL-SENDER: chenli@uniontech.com X-SENDER: chenli@uniontech.com X-LOGIN-NAME: chenli@uniontech.com X-FST-TO: libc-alpha@sourceware.org X-SENDER-IP: 58.246.122.242 X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 X-System-Flag: 0 Date: Fri, 16 Oct 2020 18:09:58 +0800 Message-ID: <87lfg6zdl5.wl-chenli@uniontech.com> From: Chen Li <chenli@uniontech.com> To: libc-alpha@sourceware.org Subject: [PATCH] shm_open/unlink: fix errno if namelen >= NAME_MAX User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?iso-8859-4?q?Goj=F2?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-10.1 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, KAM_SHORT, LONGWORDS, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list <libc-alpha.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=subscribe> Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" <libc-alpha-bounces@sourceware.org> |
Series |
shm_open/unlink: fix errno if namelen >= NAME_MAX
|
|
Commit Message
Chen Li
Oct. 16, 2020, 10:09 a.m. UTC
According to linux's manpage and posix's doc, errno should be set to ENAMETOOLONG if the path exceeds the maximuz length: linux man page(http://man7.org/linux/man-pages/man3/shm_open.3.html) ``` ENAMETOOLONG The length of name exceeds PATH_MAX. ``` posix doc(https://pubs.opengroup.org/onlinepubs/009695399/functions/shm_open.html): ``` [ENAMETOOLONG] The length of the name argument exceeds {PATH_MAX} or a pathname component is longer than {NAME_MAX}. ``` glibc doesn't handle ENAMETOOLONG correctly previously. When the path exceeds the maximum value, errno was set to EINVAL instead, which doesn't conform the man page and posix standard. This patch removes the NAME_MAX check in SHM_GET_NAME and leaves this check to open syscall, which should handle maximunize length correctly inside various filesystem implementations. --- sysdeps/posix/shm-directory.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Comments
On 16/10/2020 07:09, Chen Li wrote: > > According to linux's manpage and posix's doc, errno should be > set to ENAMETOOLONG if the path exceeds the maximuz length: > > linux man page(http://man7.org/linux/man-pages/man3/shm_open.3.html) > > ``` > ENAMETOOLONG > The length of name exceeds PATH_MAX. > ``` > > posix doc(https://pubs.opengroup.org/onlinepubs/009695399/functions/shm_open.html): > > ``` > [ENAMETOOLONG] > The length of the name argument exceeds {PATH_MAX} or a pathname component is longer than {NAME_MAX}. > ``` > glibc doesn't handle ENAMETOOLONG correctly previously. When the path > exceeds the maximum value, errno was set to EINVAL instead, which > doesn't conform the man page and posix standard. > > This patch removes the NAME_MAX check in SHM_GET_NAME and leaves this > check to open syscall, which should handle maximunize length correctly > inside various filesystem implementations. Although it fixes the errno value for large filenames, it also allows a possible unbounded stack allocation since the resulting path will be issued with alloca. I think it would be good to refactor the code to use a PATH_MAX variable instead, something like: _Bool shm_get_name (const char *prefix, char *shm_name, size shm_path_max) { size_t shm_dirlen; const char *shm_dir = __shm_directory (&shm_dirlen); if (shm_dir == NULL) { __set_errno (ENOSYS); return false; } while (name[0] == '/') ++name; size_t namelen = strlen (name) + 1; if (namelen == 1 || strchr (name, '/') != NULL) { __set_errno (EINVAL); result false; } if (shm_dirlen + namelen > shm_path_max) { __set_errno (ENAMETOOLONG); result false; } __mempcpy (__memcpy (shm_name, shm_dir, shm_dirlen), name, namelen); return true; } It would be good to move shm_get_name to its own implementation file as well (since it is used on both shm_open and shm_unlink). So on sysdeps/posix/shm_open.c: int shm_open (const char *name, int oflag, mode_t mode) { char shm_path[PATH_MAX]; if (! shm_get_name (shm_path, sizeof shm_path)) return SEM_FAILED; [...] } > --- > sysdeps/posix/shm-directory.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sysdeps/posix/shm-directory.h b/sysdeps/posix/shm-directory.h > index c7979ebb72..5a1aab2c14 100644 > --- a/sysdeps/posix/shm-directory.h > +++ b/sysdeps/posix/shm-directory.h > @@ -53,7 +53,7 @@ extern const char *__shm_directory (size_t *len); > ++name; \ > size_t namelen = strlen (name) + 1; \ > /* Validate the filename. */ \ > - if (namelen == 1 || namelen >= NAME_MAX || strchr (name, '/') != NULL) \ > + if (namelen == 1 || strchr (name, '/') != NULL) \ > { \ > __set_errno (errno_for_invalid); \ > return retval_for_invalid; \ > >
* Adhemerval Zanella via Libc-alpha: > I think it would be good to refactor the code to use a PATH_MAX variable > instead, something like: As this is generic code, I don't think we can assume PATH_MAX is defined (Hurd does not have it AFAIK). Thanks, Florian
On 27/10/2020 07:27, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> I think it would be good to refactor the code to use a PATH_MAX variable >> instead, something like: > > As this is generic code, I don't think we can assume PATH_MAX is defined > (Hurd does not have it AFAIK). Although Hurd uses the POSIX implementation, I am not sure if it is fully functional. It does not support sem [1] [2]. I would like to avoid creating a unbounded path, at least on Linux with BZ#25383 expected fix it would highly unlikely we will even need paths larger than PATH_MAX (and current implementation does imposes the NAME_MAX limit). So I think it would be better to make Linux uses at maximum PATH_MAX, or even NAME_MAX plus "/dev/shm/". [1] https://sourceware.org/bugzilla/show_bug.cgi?id=25524 [2] https://sourceware.org/bugzilla/show_bug.cgi?id=25521
On Mon Oct 26 19:44:44 GMT 2020, Adhemerval Zanella wrote: > I think it would be good to refactor the code to use a PATH_MAX variable > instead, something like: > > _Bool > shm_get_name (const char *prefix, char *shm_name, size shm_path_max) So you prefer function over macro here? If so, shm_dir, shm_dirlen, shm_name should all be passed as args too, because unlike c macro, function is not dynamical scope. Given such long arg list(errno_for_invalid, retval_for_invalid, prefix, shm_dir, shm_dirlen, shm_name), I'm not sure weathre it confrom glibc's coding style, and is refactoring to split into more small functions a good idea? Thank, Chen Li
On 03/11/2020 11:45, Chen Li wrote: > > On Mon Oct 26 19:44:44 GMT 2020, > Adhemerval Zanella wrote: > >> I think it would be good to refactor the code to use a PATH_MAX variable >> instead, something like: >> >> _Bool >> shm_get_name (const char *prefix, char *shm_name, size shm_path_max) > > So you prefer function over macro here? If so, shm_dir, shm_dirlen, shm_name > should all be passed as args too, because unlike c macro, function is not > dynamical scope. Given such long arg list(errno_for_invalid, retval_for_invalid, > prefix, shm_dir, shm_dirlen, shm_name), I'm not sure weathre it confrom glibc's > coding style, and is refactoring to split into more small functions a good idea? Florian has pointed me out a better solution would be use use his fix [1] for BZ#25383 [2] which refactor the POSIX sem/shm code to use a static buffer instead. Then the ENAMETOOLONG could be returned by '__shm_get_name' instead. Florian, do you plan to send your patch for review? [1] https://sourceware.org/bugzilla/attachment.cgi?id=12921 [2] https://sourceware.org/bugzilla/show_bug.cgi?id=25383
* Adhemerval Zanella: > Florian has pointed me out a better solution would be use > use his fix [1] for BZ#25383 [2] which refactor the POSIX sem/shm > code to use a static buffer instead. > > Then the ENAMETOOLONG could be returned by '__shm_get_name' > instead. > > Florian, do you plan to send your patch for review? I'm working on the change you requested (eliminate the new struct type). No ETA yet. Hopefully still this month. Thanks, Florian
On 03/11/2020 13:49, Florian Weimer wrote: > * Adhemerval Zanella: > >> Florian has pointed me out a better solution would be use >> use his fix [1] for BZ#25383 [2] which refactor the POSIX sem/shm >> code to use a static buffer instead. >> >> Then the ENAMETOOLONG could be returned by '__shm_get_name' >> instead. >> >> Florian, do you plan to send your patch for review? > > I'm working on the change you requested (eliminate the new struct > type). No ETA yet. Hopefully still this month. Thanks, I think the NAME_MAX errno fix should be simpler on top of your patch.
diff --git a/sysdeps/posix/shm-directory.h b/sysdeps/posix/shm-directory.h index c7979ebb72..5a1aab2c14 100644 --- a/sysdeps/posix/shm-directory.h +++ b/sysdeps/posix/shm-directory.h @@ -53,7 +53,7 @@ extern const char *__shm_directory (size_t *len); ++name; \ size_t namelen = strlen (name) + 1; \ /* Validate the filename. */ \ - if (namelen == 1 || namelen >= NAME_MAX || strchr (name, '/') != NULL) \ + if (namelen == 1 || strchr (name, '/') != NULL) \ { \ __set_errno (errno_for_invalid); \ return retval_for_invalid; \