Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HPCC-30469 Avoid duplicated PathSepChar in getExternalPath() #17898

Merged
merged 1 commit into from
Oct 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion dali/base/dautils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1479,7 +1479,16 @@ bool CDfsLogicalFileName::getExternalPath(StringBuffer &dir, StringBuffer &tail,
*e = makeStringExceptionV(-1, "plane:: does not support planes with more than one device '%s'", planeName.str());
return false;
}
dir.append(plane->queryPrefix());
const char * prefix = plane->queryPrefix();
//If the prefix is a PathSepChar, it should not be appended to the dir here because
//a PathSepChar will be appended to the dir inside the expandExternalPath() if the s
//is started with the "::".
//Also a trailing pathsepchar in the prefix should be removed.
if (!isRootDirectory(prefix))
{
dir.append(prefix);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you want or need to special case root, "/" is just an empty path with a trailing "/", so it will be covered by the next line afaics (comments should also change to reflect).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removeTrailingPathSepChar() only removes the "/" if dir.length() > 1:

inline StringBuffer &removeTrailingPathSepChar(StringBuffer &path)
{
    if (path.length()>1 && isPathSepChar(path.charAt(path.length()-1)))
    {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

removeTrailingPathSepChar(dir);
}
}
}
}
Expand Down
1 change: 1 addition & 0 deletions esp/services/ws_fileio/ws_fileioservice.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ bool CWsFileIOEx::onReadFileData(IEspContext &context, IEspReadFileDataRequest &
return true;
}

//Despite the "DestRelativePath" saying it's relative for legacy reason, it also supports absolute paths.
const char* destRelativePath = req.getDestRelativePath();
if (!destRelativePath || (destRelativePath[0] == 0))
{
Expand Down
10 changes: 10 additions & 0 deletions esp/smc/SMCLib/TpCommon.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,16 @@ extern TPWRAPPER_API void validateDropZoneAccess(IEspContext& context, const cha
if (!isHostInPlane(dropZone, hostReq, true))
throw makeStringExceptionV(ECLWATCH_INVALID_INPUT, "Host %s is not valid DropZone plane %s", hostReq, targetDZNameOrHost);
}

//If the dropZonePath is an absolute path, change it to a relative path.
if (isAbsolutePath(fileNameWithRelPath))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we also add some comment to CWsFileIOEx::onReadFileData in this PR (to reduce future confusion) to note that despite the "destRelativePath" saying it's relative for legacy reason, it also supports absolute paths?

(I'm also curious why ReadFileDataRequest uses fields with "Dest" in them, when this is only used for reading ..)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add the comment.
All of the three methods in WsFileIO uses the "Dest". But, it is not 'good' for the ReadFileDataRequest.

{
const char* relativePath = getRelativePath(fileNameWithRelPath, dropZone->queryProp("@prefix"));
if (nullptr == relativePath)
throw makeStringExceptionV(ECLWATCH_INVALID_INPUT, "Invalid DropZone path %s.", fileNameWithRelPath);
fileNameWithRelPath = relativePath;
}

const char *dropZoneName = dropZone->queryProp("@name");
SecAccessFlags permission = getDZFileScopePermissions(context, dropZoneName, fileNameWithRelPath, hostReq);
if (permission < permissionReq)
Expand Down
18 changes: 18 additions & 0 deletions system/jlib/jfile.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5642,6 +5642,24 @@ const char *splitRelativePath(const char *full,const char *basedir,StringBuffer
return t;
}

const char *getRelativePath(const char *path,const char *leadingPath)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note to myself - this is backported from 9.2

{
size_t pathLen = strlen(path);
size_t leadingLen = strlen(leadingPath);
if ((pathLen==leadingLen-1)&&isPathSepChar(leadingPath[leadingLen-1]))
--leadingLen;
if (0 == strncmp(path,leadingPath,leadingLen))
{
const char *rel = path + leadingLen;
if ('\0' == *rel)
return rel;
if (isPathSepChar(*rel))
return rel+1;
return rel;
}
return nullptr;
}

const char *splitDirMultiTail(const char *multipath,StringBuffer &dir,StringBuffer &tail)
{
// the first directory is the significant one
Expand Down
1 change: 1 addition & 0 deletions system/jlib/jfile.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -605,6 +605,7 @@ extern jlib_decl StringBuffer &makeAbsolutePath(StringBuffer &relpath,bool mustE
extern jlib_decl StringBuffer &makeAbsolutePath(const char *relpath, const char *basedir, StringBuffer &out);
extern jlib_decl StringBuffer &makePathUniversal(const char *path, StringBuffer &out);
extern jlib_decl const char *splitRelativePath(const char *full,const char *basedir,StringBuffer &reldir); // removes basedir if matches, returns tail and relative dir
extern jlib_decl const char *getRelativePath(const char *path, const char *leadingPath);
extern jlib_decl const char *splitDirMultiTail(const char *multipath,StringBuffer &dir,StringBuffer &tail);
extern jlib_decl StringBuffer &mergeDirMultiTail(const char *dir,const char *tail, StringBuffer &multipath);
extern jlib_decl StringBuffer &removeRelativeMultiPath(const char *full,const char *dir,StringBuffer &reltail); // removes dir if matches, returns relative multipath
Expand Down
Loading