xiaoxiang781216 commented on code in PR #19900: URL: https://github.com/apache/nuttx/pull/19900#discussion_r3881696392
########## fs/inode/fs_inodesearch.c: ########## @@ -49,6 +50,11 @@ static int _inode_linktarget(FAR struct inode *inode, #endif static int _inode_search(FAR struct inode_search_s *desc); static FAR const char *_inode_getcwd(void); +#ifdef CONFIG_FS_CHROOT +static int inode_normalize_abs(FAR const char *in, FAR char *out, Review Comment: please reuse _inode_canonicalize from https://github.com/apache/nuttx/pull/19991/changes#diff-7bc927241d95c20f33dde38f6f6f80ef209cc94fecfef45ef248d0d7dd7d4934R238 ########## fs/inode/fs_inodesearch.c: ########## @@ -265,10 +276,16 @@ static int _inode_canonicalize(FAR char *path) if (src[0] == '.' && src[1] == '.' && (src[2] == '/' || src[2] == '\0')) { - /* Cannot go above root */ + /* Cannot go above the floor (host "/" or the jail prefix) */ - if (dst <= path + 1) + if (dst <= min_dst) { + if (min_dst > path + 1) Review Comment: why need check ########## fs/inode/fs_inodesearch.c: ########## @@ -265,10 +276,16 @@ static int _inode_canonicalize(FAR char *path) if (src[0] == '.' && src[1] == '.' && (src[2] == '/' || src[2] == '\0')) { - /* Cannot go above root */ + /* Cannot go above the floor (host "/" or the jail prefix) */ - if (dst <= path + 1) + if (dst <= min_dst) Review Comment: ``` if (dst > min_dst) { /* Remove trailing slash first */ dst--; /* Scan backward to find the previous '/' */ while (dst > path + 1 && *(dst - 1) != '/') while (dst > min_dst && *(dst - 1) != '/') { dst--; } } src += (src[2] == '/') ? 3 : 2; continue; ``` ########## fs/inode/fs_inodesearch.c: ########## @@ -676,6 +794,50 @@ static FAR const char *_inode_getcwd(void) * Public Functions ****************************************************************************/ +#ifdef CONFIG_FS_CHROOT +/**************************************************************************** + * Name: inode_chroot_hostpath + * + * Description: + * Convert 'path' to a host absolute path under the calling group's jail: + * make it absolute via $PWD if needed, prepend tg_root, canonicalize, + * and keep the result under the jail. With no jail, only make absolute + * and canonicalize. + * + ****************************************************************************/ + +int inode_chroot_hostpath(FAR const char *path, FAR char *out, Review Comment: remove, call inode_search_setup directly ########## fs/inode/fs_inodesearch.c: ########## @@ -358,34 +375,44 @@ static int _inode_checkpath(const char *path) return pathlen >= PATH_MAX ? -ENAMETOOLONG : OK; } +#ifdef CONFIG_FS_CHROOT /**************************************************************************** - * Name: _inode_search + * Name: _inode_chroot_root * * Description: - * Find the inode associated with 'path' returning the inode references - * and references to its companion nodes. This is the internal, common - * implementation of inode_search(). - * - * If a mountpoint is encountered in the search prior to encountering the - * terminal node, the search will terminate at the mountpoint inode. That - * inode and the relative path from the mountpoint, 'relpath' will be - * returned. + * Return the calling group's jail prefix, or NULL if none is installed. * - * If a soft link is encountered that is not the terminal node in the path, - * that link WILL be deferenced unconditionally. + ****************************************************************************/ + +static FAR const char *_inode_chroot_root(void) Review Comment: _inode_chroot_root->_inode_root_path ########## fs/inode/fs_inodesearch.c: ########## @@ -446,13 +491,86 @@ static int _inode_search(FAR struct inode_search_s *desc) desc->path = desc->buffer; } +#ifdef CONFIG_FS_CHROOT + else if (root != NULL) + { + size_t rootlen = strlen(root); + size_t pathlen = strlen(desc->buffer); + size_t need = rootlen + pathlen + 1; + FAR char *newbuf; + + if (need < PATH_MAX) + { + need = PATH_MAX; + } + + newbuf = lib_get_tempbuffer(need); + if (newbuf == NULL) + { + return -ENOMEM; + } + + snprintf(newbuf, need, "%s%s", root, desc->buffer); + lib_put_tempbuffer(desc->buffer); + desc->buffer = newbuf; + desc->path = newbuf; + } +#endif /* Canonicalize the path to remove "." and ".." segments. This ensures * that mountpoint relpath never contains ".." which most filesystems - * (tmpfs, romfs, etc.) cannot resolve. + * (tmpfs, romfs, etc.) cannot resolve. When a jail is installed, + * min_dst keeps ".." from popping above tg_root. */ - ret = _inode_canonicalize(desc->buffer); + min_dst = desc->buffer + 1; +#ifdef CONFIG_FS_CHROOT + if (root != NULL) + { + size_t rootlen = strlen(root); Review Comment: let's avoid strlen root twice ########## fs/inode/fs_inodesearch.c: ########## @@ -446,13 +491,86 @@ static int _inode_search(FAR struct inode_search_s *desc) desc->path = desc->buffer; } +#ifdef CONFIG_FS_CHROOT + else if (root != NULL) + { + size_t rootlen = strlen(root); + size_t pathlen = strlen(desc->buffer); + size_t need = rootlen + pathlen + 1; Review Comment: `buflen = strlen(root) + strlen(desc->buffer) + 1`` ########## fs/inode/fs_inodesearch.c: ########## @@ -229,19 +231,28 @@ static int _compute_path_depth(FAR const char *path) * * Description: * Remove "." and ".." segments from an absolute path in-place. - * The path MUST start with '/'. Returns -EINVAL if ".." attempts - * to ascend beyond the root directory, or -ENAMETOOLONG if the - * canonicalized result is >= PATH_MAX bytes. + * The path MUST start with '/'. 'min_dst' is the lowest write + * position ".." may pop to (path + 1 for the host root). Returns + * -EINVAL if ".." attempts to ascend beyond that floor when the + * floor is the host root, or -ENAMETOOLONG if the canonicalized + * result is >= PATH_MAX bytes. If 'min_dst' is past path + 1 + * (a chroot jail prefix), ".." is dropped instead of erroring so + * the result stays under the jail. * ****************************************************************************/ -static int _inode_canonicalize(FAR char *path) +static int _inode_canonicalize(FAR char *path, FAR char *min_dst) Review Comment: min_dst->dst_min ########## fs/inode/fs_inodesearch.c: ########## @@ -358,34 +375,44 @@ static int _inode_checkpath(const char *path) return pathlen >= PATH_MAX ? -ENAMETOOLONG : OK; } +#ifdef CONFIG_FS_CHROOT /**************************************************************************** - * Name: _inode_search + * Name: _inode_chroot_root * * Description: - * Find the inode associated with 'path' returning the inode references - * and references to its companion nodes. This is the internal, common - * implementation of inode_search(). - * - * If a mountpoint is encountered in the search prior to encountering the - * terminal node, the search will terminate at the mountpoint inode. That - * inode and the relative path from the mountpoint, 'relpath' will be - * returned. + * Return the calling group's jail prefix, or NULL if none is installed. * - * If a soft link is encountered that is not the terminal node in the path, - * that link WILL be deferenced unconditionally. + ****************************************************************************/ + +static FAR const char *_inode_chroot_root(void) +{ + FAR struct tcb_s *tcb = nxsched_self(); + + if (tcb != NULL && tcb->group != NULL) + { + return tcb->group->tg_root; + } + + return NULL; +} +#endif + +/**************************************************************************** + * Name: _inode_prepare_path * - * Assumptions: - * The caller holds the g_inode_sem semaphore + * Description: + * Make 'desc->path' absolute, optionally prepend the chroot jail, and + * canonicalize "." / ".." in-place. On success desc->path points at + * desc->buffer, which holds the host-absolute path. * ****************************************************************************/ -static int _inode_search(FAR struct inode_search_s *desc) +static int _inode_prepare_path(FAR struct inode_search_s *desc) Review Comment: let's change _inode_prepare_path to inode_search_setup and remove SETUP_SEARCH, and remove RELEASE_SEARCH to inode_search_release. ########## fs/inode/fs_inodesearch.c: ########## @@ -676,6 +794,50 @@ static FAR const char *_inode_getcwd(void) * Public Functions ****************************************************************************/ +#ifdef CONFIG_FS_CHROOT +/**************************************************************************** + * Name: inode_chroot_hostpath + * + * Description: + * Convert 'path' to a host absolute path under the calling group's jail: + * make it absolute via $PWD if needed, prepend tg_root, canonicalize, + * and keep the result under the jail. With no jail, only make absolute + * and canonicalize. + * + ****************************************************************************/ + +int inode_chroot_hostpath(FAR const char *path, FAR char *out, + size_t outlen) +{ + struct inode_search_s desc; + int ret; + + if (path == NULL || path[0] == '\0' || out == NULL || outlen < 2) + { + return -EINVAL; + } + + SETUP_SEARCH(&desc, path, true); + + ret = _inode_prepare_path(&desc); + if (ret >= 0) + { + if (strlen(desc.path) >= outlen) + { + ret = -ENAMETOOLONG; + } + else + { + strlcpy(out, desc.path, outlen); + ret = OK; Review Comment: remove, don't need ########## fs/inode/fs_inodesearch.c: ########## @@ -446,13 +491,86 @@ static int _inode_search(FAR struct inode_search_s *desc) desc->path = desc->buffer; } +#ifdef CONFIG_FS_CHROOT + else if (root != NULL) + { + size_t rootlen = strlen(root); + size_t pathlen = strlen(desc->buffer); + size_t need = rootlen + pathlen + 1; + FAR char *newbuf; + + if (need < PATH_MAX) + { + need = PATH_MAX; + } + + newbuf = lib_get_tempbuffer(need); + if (newbuf == NULL) + { + return -ENOMEM; + } + + snprintf(newbuf, need, "%s%s", root, desc->buffer); + lib_put_tempbuffer(desc->buffer); + desc->buffer = newbuf; + desc->path = newbuf; + } +#endif /* Canonicalize the path to remove "." and ".." segments. This ensures * that mountpoint relpath never contains ".." which most filesystems - * (tmpfs, romfs, etc.) cannot resolve. + * (tmpfs, romfs, etc.) cannot resolve. When a jail is installed, + * min_dst keeps ".." from popping above tg_root. */ - ret = _inode_canonicalize(desc->buffer); + min_dst = desc->buffer + 1; +#ifdef CONFIG_FS_CHROOT + if (root != NULL) + { + size_t rootlen = strlen(root); + + min_dst = desc->buffer + rootlen; + if (rootlen > 0 && root[rootlen - 1] != '/') + { + min_dst++; + } + } +#endif + + return _inode_canonicalize(desc->buffer, min_dst); +} + +/**************************************************************************** + * Name: _inode_search + * + * Description: + * Find the inode associated with 'path' returning the inode references + * and references to its companion nodes. This is the internal, common + * implementation of inode_search(). + * + * If a mountpoint is encountered in the search prior to encountering the + * terminal node, the search will terminate at the mountpoint inode. That + * inode and the relative path from the mountpoint, 'relpath' will be + * returned. + * + * If a soft link is encountered that is not the terminal node in the path, + * that link WILL be deferenced unconditionally. + * + * Assumptions: + * The caller holds the g_inode_sem semaphore + * + ****************************************************************************/ + +static int _inode_search(FAR struct inode_search_s *desc) +{ + FAR const char *name; Review Comment: move after line 570 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
