Skip to content

HPCC-35234 Ensure XRef handles trailing extensions correctly #21051

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.

Sign up for GitHub

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

Open
jackdelv wants to merge 1 commit into
base: candidate-10.0.x
Choose a base branch
from
+15 −3
Open

HPCC-35234 Ensure XRef handles trailing extensions correctly #21051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 12 additions & 1 deletion dali/base/dadfs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,8 @@ RemoteFilename &constructPartFilename(IGroup *grp,unsigned partNo,unsigned copy,
{
partNo--;
StringBuffer partName;
if (!lname||!*lname)
const char *tailExt = nullptr;
if (isEmptyString(lname))
{
if (!pmask)
{
Expand All @@ -314,13 +315,23 @@ RemoteFilename &constructPartFilename(IGroup *grp,unsigned partNo,unsigned copy,
}
lname = expandMask(partName, pmask, partNo, max);
}
else if (!isEmptyString(pmask))
{
// pmask may contain trailing extensions that aren't in lname
const char *ext = strchr(pmask, '.');
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

strchr(pmask, '.') searches the entire string, so if pmask includes a directory path containing dots (e.g. /var/lib/hpcc/v1.2/data/file._$P$), ext will point into the directory portion, and tailExt may become incorrect (or misleadingly non-null). This can result in appending the wrong substring to fullname, producing malformed filenames. Consider finding dots only in the basename portion of pmask (after the last path separator), or use an existing path/filename utility in the codebase to locate extensions reliably.

Suggested change
const char *ext = strchr(pmask, '.');
// Only consider dots in the basename portion (after the last path separator)
const char *filePart = pmask;
const char *lastSlash = strrchr(pmask, '/');
const char *lastBackslash = strrchr(pmask, '\\');
if (lastSlash || lastBackslash)
{
const char *lastSep = lastSlash;
if (!lastSep || (lastBackslash && lastBackslash > lastSep))
lastSep = lastBackslash;
filePart = lastSep + 1;
}
const char *ext = strchr(filePart, '.');

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@jackdelv jackdelv Mar 3, 2026

Choose a reason for hiding this comment

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

pmask comes from cFileDesc::getNameMask and will only ever contain the file name.

assertex(ext); // pmask should have at least one extension for the part mask
tailExt = strchr(ext+1, '.');
Comment on lines +322 to +323
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

The new assertex(ext) introduces a hard failure for any caller that passes a non-empty pmask without a . character (even if such masks were previously tolerated). Since this is a filename-construction helper, it’s safer to treat “no extension found” as “no trailing extension to append” (i.e., skip appending) rather than asserting and potentially aborting in production.

Suggested change
assertex(ext); // pmask should have at least one extension for the part mask
tailExt = strchr(ext+1, '.');
if (ext)
tailExt = strchr(ext+1, '.');

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@jackdelv jackdelv Mar 3, 2026

Choose a reason for hiding this comment

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

It may not be the best to error on this, but it should always be the case that the pmask have at least one extension. Otherwise it would not have been able to deduce the part mask and this file would not make it here. It would be treated as an external file.

}

// NB: calcStripeNumber expects a 0-based part number. 'partNo' is already decremented earlier to make it 0-based.
unsigned stripeNum = calcStripeNumber(partNo, lfnHash, numDevices);

StringBuffer fullname;
makePhysicalPartName(lname, partNo+1, max, fullname, 0, DFD_OSdefault, prefix, dirPerPart, stripeNum);

if (!isEmptyString(tailExt))
fullname.append(tailExt);

// revisit: constructPartFilename should be refactored not to deal with replicate directories, by pre-determining the alternate prefix if copy>0
// If copy>0 it could do calcPartLocation, find the replicate plane, get it's prefix, and pass to makePhysicalPartName
unsigned n = 0;
Expand Down
5 changes: 3 additions & 2 deletions dali/sasha/saxref.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1919,8 +1919,9 @@ class CNewXRefManager: public CNewXRefManagerBase
if (drv==drvs)
return; // no orphans
StringBuffer mask;
StringBuffer scopeBuf(currentScope);
scopeBuf.append("::");
StringBuffer scopeBuf;
if (!isEmptyString(currentScope))
scopeBuf.append(currentScope).append("::");
f->getNameMask(mask);
assertex(f->getName(scopeBuf)); // Should always return true for HPCC files
// orphans are only orphans if there doesn't exist a valid file
Expand Down
Loading