xiaoxiang781216 commented on code in PR #19900:
URL: https://github.com/apache/nuttx/pull/19900#discussion_r3891133090
##########
fs/driver/fs_findmtddriver.c:
##########
@@ -70,20 +70,20 @@ int find_mtddriver(FAR const char *pathname, FAR struct
inode **ppinode)
/* Find the inode registered with this pathname */
- SETUP_SEARCH(&desc, pathname, false);
+ ret = inode_search_setup(&desc, pathname, false);
+ if (ret < 0)
+ {
+ return ret;
+ }
- ret = inode_find(&desc);
+ ret = inode_find(&desc, &inode);
if (ret < 0)
{
ferr("ERROR: Failed to find %s\n", pathname);
ret = -ENOENT;
Review Comment:
remove
##########
fs/inode/fs_inodefind.c:
##########
@@ -58,17 +58,16 @@ int inode_find(FAR struct inode_search_s *desc)
*/
inode_rlock();
- ret = inode_search(desc);
+ ret = inode_search(desc, inode);
if (ret >= 0)
Review Comment:
need check inode isnot NULL
##########
fs/vfs/fs_symlink.c:
##########
@@ -94,9 +94,14 @@ int symlink(FAR const char *path1, FAR const char *path2)
* 'path2' does not lie on a mounted volume.
*/
- SETUP_SEARCH(&desc, path2, false);
+ ret = inode_search_setup(&desc, path2, false);
+ if (ret < 0)
+ {
+ errcode = -ret;
Review Comment:
remove errcode too
##########
fs/vfs/fs_symlink.c:
##########
@@ -106,13 +111,13 @@ int symlink(FAR const char *path1, FAR const char *path2)
#ifndef CONFIG_DISABLE_MOUNTPOINT
/* Check if the inode is a mountpoint. */
- DEBUGASSERT(desc.node != NULL);
- if (INODE_IS_MOUNTPT(desc.node))
+ DEBUGASSERT(inode != NULL);
+ if (INODE_IS_MOUNTPT(inode))
{
- if (desc.node->u.i_mops && desc.node->u.i_mops->symlink)
+ if (inode->u.i_mops && inode->u.i_mops->symlink)
{
- ret = desc.node->u.i_mops->symlink(desc.node, path1,
- desc.relpath);
+ ret = inode->u.i_mops->symlink(inode, path1,
+ desc.relpath);
Review Comment:
add one space
##########
fs/vfs/fs_rename.c:
##########
@@ -96,19 +101,18 @@ static int pseudorename(FAR const char *oldpath, FAR
struct inode *oldinode,
ret = inode_checkperm(oldparent, W_OK);
if (ret < 0)
{
- goto errout_with_lock;
+ goto errout_with_newsearch;
}
/* According to POSIX, any new inode at this path should be removed
* first, provided that it is not a directory.
*/
- ret = inode_search(&newdesc);
+ ret = inode_search(&newdesc, &newinode);
Review Comment:
need release newinode
##########
fs/vfs/fs_rename.c:
##########
@@ -191,19 +195,24 @@ static int pseudorename(FAR const char *oldpath, FAR
struct inode *oldinode,
ret = inode_reserve(newpath, 0777, &newinode);
if (ret < 0)
{
- goto errout_with_lock;
+ goto errout_with_newsearch;
}
/* Re-resolve the source under the same lock before unlinking it. */
- SETUP_SEARCH(&olddesc, oldpath, true);
- ret = inode_search(&olddesc);
- RELEASE_SEARCH(&olddesc);
- if (ret < 0 || olddesc.node != oldinode)
+ ret = inode_search_setup(&olddesc, oldpath, true);
+ if (ret < 0)
+ {
+ goto errout_with_newsearch;
+ }
+
+ ret = inode_search(&olddesc, &oldfound);
+ inode_search_release(&olddesc);
Review Comment:
release oldfound
##########
fs/inode/fs_inodesearch.c:
##########
@@ -155,7 +157,8 @@ static int _inode_compare(FAR const char *fname, FAR struct
inode *inode)
#ifdef CONFIG_FS_LINKS
static int _inode_linktarget(FAR struct inode *inode,
Review Comment:
let change inode to ** directly
##########
fs/inode/fs_inodesearch.c:
##########
@@ -677,87 +687,153 @@ static FAR const char *_inode_getcwd(void)
****************************************************************************/
/****************************************************************************
- * Name: inode_search
+ * Name: inode_search_setup
*
* Description:
- * Find the inode associated with 'path' returning the inode references
- * and references to its companion nodes.
- *
- * 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.
- *
- * inode_search will follow soft links in path leading up to the terminal
- * node. Whether or no inode_search() will deference that terminal node
- * depends on the 'nofollow' input.
- *
- * 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
+ * Initialize a search descriptor and make 'path' host-absolute: join
+ * $PWD if it is relative, prepend the chroot jail if one is installed,
+ * and canonicalize "." / "..". On success desc->path points at
+ * desc->buffer.
*
****************************************************************************/
-int inode_search(FAR struct inode_search_s *desc)
+int inode_search_setup(FAR struct inode_search_s *desc,
Review Comment:
Move before inode_search to reduce the difference
##########
fs/inode/fs_inodesearch.c:
##########
@@ -642,10 +607,55 @@ static int _inode_search(FAR struct inode_search_s *desc)
*/
desc->path = name;
- desc->node = inode;
desc->peer = left;
desc->parent = above;
desc->relpath = relpath;
+
+#ifdef CONFIG_FS_LINKS
+ if (ret >= 0)
+ {
+ DEBUGASSERT(inode != NULL);
+
+ /* Is the terminal node a softlink? Should we follow it? */
+
+ if (!desc->nofollow && INODE_IS_SOFTLINK(inode))
+ {
+ /* The terminating inode is a valid soft link: Return the inode,
+ * corresponding to link target. _inode_linktarget() will follow
+ * a link (or a series of links to links) and will return the
+ * link target of the final symbolic link in the series.
+ */
+
+ ret = _inode_linktarget(inode, desc, &inode);
+ if (ret < 0)
+ {
+ /* The most likely cause for failure is that the target of the
+ * symbolic link does not exist.
+ */
+
+ if (inodep != NULL)
+ {
+ *inodep = inode;
Review Comment:
why update in the fail path
##########
fs/vfs/fs_rename.c:
##########
@@ -191,19 +195,24 @@ static int pseudorename(FAR const char *oldpath, FAR
struct inode *oldinode,
ret = inode_reserve(newpath, 0777, &newinode);
Review Comment:
need release newinode before overwrite it
##########
fs/driver/fs_findblockdriver.c:
##########
@@ -77,20 +77,20 @@ int find_blockdriver(FAR const char *pathname, int
mountflags,
/* Find the inode registered with this pathname */
- SETUP_SEARCH(&desc, pathname, false);
+ ret = inode_search_setup(&desc, pathname, false);
+ if (ret < 0)
+ {
+ return ret;
+ }
- ret = inode_find(&desc);
+ ret = inode_find(&desc, &inode);
if (ret < 0)
{
ferr("ERROR: Failed to find %s\n", pathname);
ret = -ENOENT;
Review Comment:
remove
##########
fs/inode/fs_inodesearch.c:
##########
@@ -677,87 +687,153 @@ static FAR const char *_inode_getcwd(void)
****************************************************************************/
/****************************************************************************
- * Name: inode_search
+ * Name: inode_search_setup
*
* Description:
- * Find the inode associated with 'path' returning the inode references
- * and references to its companion nodes.
- *
- * 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.
- *
- * inode_search will follow soft links in path leading up to the terminal
- * node. Whether or no inode_search() will deference that terminal node
- * depends on the 'nofollow' input.
- *
- * 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
+ * Initialize a search descriptor and make 'path' host-absolute: join
+ * $PWD if it is relative, prepend the chroot jail if one is installed,
+ * and canonicalize "." / "..". On success desc->path points at
+ * desc->buffer.
*
****************************************************************************/
-int inode_search(FAR struct inode_search_s *desc)
+int inode_search_setup(FAR struct inode_search_s *desc,
+ FAR const char *path, bool nofollow)
{
+ FAR const char *cwd = NULL;
+ FAR const char *root = NULL;
+ FAR char *dst_min;
+ size_t rootlen = 0;
+ size_t buflen;
int ret;
- /* Perform the common _inode_search() logic. This does everything except
- * operations special operations that must be performed on the terminal
- * node if node is a symbolic link.
- */
-
- DEBUGASSERT(desc != NULL && desc->path != NULL);
+ desc->path = path;
+ desc->peer = NULL;
+ desc->parent = NULL;
+ desc->relpath = NULL;
+ desc->buffer = NULL;
+ desc->nofollow = nofollow;
- ret = _inode_search(desc);
+ if (path == NULL)
+ {
+ return -EINVAL;
+ }
-#ifdef CONFIG_FS_LINKS
- if (ret >= 0)
+ ret = _inode_checkpath(path);
+ if (ret < 0)
{
- FAR struct inode *inode;
+ return ret;
+ }
- /* Search completed successfully */
+#ifdef CONFIG_FS_CHROOT
+ root = _inode_root_path();
+ if (root != NULL)
+ {
+ rootlen = strlen(root);
+ }
+#endif
- inode = desc->node;
- DEBUGASSERT(inode != NULL);
+ /* For a relative path the absolute form is "<cwd>/<path>". That
+ * concatenation can exceed PATH_MAX even when the relative path
+ * itself is within PATH_MAX: a relative path of PATH_MAX-1 bytes
+ * is legal per pathconf(_PC_PATH_MAX), but the prefix added by the
+ * cwd pushes the uncanonicalized form past the limit. Size the
+ * buffer to hold the full absolute form so that ".." segments are
+ * collapsed against the correct suffix; truncating first could
+ * drop the trailing component and let ".." collapse the path onto
+ * a directory (yielding the wrong errno, e.g. EISDIR, instead of
+ * resolving the file). _inode_canonicalize() still rejects any
+ * result whose canonicalized length reaches PATH_MAX.
+ */
- /* Is the terminal node a softlink? Should we follow it? */
+ if (*path != '/')
+ {
+ cwd = _inode_getcwd();
+ buflen = strlen(cwd) + 1 + strlen(path) + 1;
+ }
+ else
+ {
+ buflen = strlen(path) + 1;
+ }
- if (!desc->nofollow && INODE_IS_SOFTLINK(inode))
- {
- /* The terminating inode is a valid soft link: Return the inode,
- * corresponding to link target. _inode_linktarget() will follow
- * a link (or a series of links to links) and will return the
- * link target of the final symbolic link in the series.
- */
+ buflen += rootlen;
+ if (buflen < PATH_MAX)
+ {
+ buflen = PATH_MAX;
+ }
- ret = _inode_linktarget(inode, desc);
- if (ret < 0)
- {
- /* The most likely cause for failure is that the target of the
- * symbolic link does not exist.
- */
+ desc->buffer = lib_get_tempbuffer(buflen);
+ if (desc->buffer == NULL)
+ {
+ return -ENOMEM;
+ }
- return ret;
- }
+ if (root != NULL)
+ {
+ if (cwd != NULL)
+ {
+ snprintf(desc->buffer, buflen, "%s%s/%s", root, cwd, path);
}
- else if (!desc->nofollow && INODE_IS_HARDLINK(inode))
+ else
{
- /* The terminating inode is a valid hard link */
+ snprintf(desc->buffer, buflen, "%s%s", root, path);
+ }
+ }
+ else if (cwd != NULL)
+ {
+ snprintf(desc->buffer, buflen, "%s/%s", cwd, path);
+ }
+ else
+ {
+ strlcpy(desc->buffer, path, buflen);
+ }
- inode = inode->i_private;
- DEBUGASSERT(inode != NULL);
+ desc->path = desc->buffer;
- desc->node = inode;
+ /* Canonicalize the path to remove "." and ".." segments. This ensures
+ * that mountpoint relpath never contains ".." which most filesystems
+ * (tmpfs, romfs, etc.) cannot resolve. When a jail is installed,
+ * dst_min keeps ".." from popping above tg_root.
+ */
+
+ dst_min = desc->buffer + 1;
+#ifdef CONFIG_FS_CHROOT
+ if (root != NULL)
+ {
+ dst_min = desc->buffer + rootlen;
+ if (rootlen > 0 && root[rootlen - 1] != '/')
+ {
+ dst_min++;
}
}
#endif
+ ret = _inode_canonicalize(desc->buffer, dst_min);
+ if (ret < 0)
+ {
+ inode_search_release(desc);
+ }
+
return ret;
}
+/****************************************************************************
+ * Name: inode_search_release
+ *
+ * Description:
+ * Release any buffer allocated by inode_search_setup().
+ *
+ ****************************************************************************/
+
+void inode_search_release(FAR struct inode_search_s *desc)
Review Comment:
Move before inode_search_setup
--
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]
