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

Upgrade lengths of RichString to size_t & many related changes. #1592

Draft
wants to merge 22 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
233c953
Fix potential out of bound access in PCPProcessTable_updateCmdline()
Explorer09 Jan 15, 2025
160c066
Minor code style adjustment in matchCmdlinePrefixWithExeSuffix()
Explorer09 Jan 16, 2025
f83ab04
Improve process cmdline basename matching with procExe path
Explorer09 Jan 16, 2025
56654dd
Improve "comm" string highlighting in Process_makeCommandStr()
Explorer09 Jan 16, 2025
9a395cb
Process: Use size_t for "cmdline" and "procExe" offsets
Explorer09 Jan 16, 2025
9eb2343
Simplify an expression in Process_updateExe()
Explorer09 Jan 16, 2025
473c847
Simplify findCommInCmdline() logic in Process.c
Explorer09 Jan 16, 2025
7ff906c
RichString_setLen() minor logic fix
Explorer09 Jan 18, 2025
bd27b7e
Prevent length wraparound in RichString_rewind()
Explorer09 Jan 18, 2025
1715ad0
Replace unnecessary RichString_appendnAscii() calls with appendAscii()
Explorer09 Jan 18, 2025
4ef38cb
Use RichString_appendnAscii() for CPU frequency text
Explorer09 Jan 18, 2025
fd54355
Add an assertion to Row_printLeftAlignedField()
Explorer09 Jan 18, 2025
39c14cd
Add bound checks when changing Panel.cursorX value
Explorer09 Jan 18, 2025
521e563
Remove redundant length clamping in Panel_draw()
Explorer09 Jan 18, 2025
78184a1
Panel.scrollH value clamp logic improvement
Explorer09 Jan 18, 2025
e101339
Use size_t for Panel.scrollH and Panel.selectedLen
Explorer09 Jan 18, 2025
9af21a7
Upgrade all length parameters of RichString class to size_t
Explorer09 Jan 18, 2025
4b32769
Add size limit checks to RichString methods
Explorer09 Jan 20, 2025
5d61426
Improve BarMeterMode_draw() limit check after data type changes
Explorer09 Jan 18, 2025
22deed3
Improve LEDMeterMode_draw() limit check after data type changes
Explorer09 Jan 18, 2025
1b8f1ab
Fix mbstowcs_nonfatal() that might convert string shorter than desired
Explorer09 Jan 18, 2025
005c9b6
Introduce RichString_append{,n}FormatAscii functions
Explorer09 Jan 20, 2025
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
152 changes: 85 additions & 67 deletions Process.c
Original file line number Diff line number Diff line change
Expand Up @@ -64,29 +64,26 @@ void Process_fillStarttimeBuffer(Process* this) {
*/
#define TASK_COMM_LEN 16

static bool findCommInCmdline(const char* comm, const char* cmdline, int cmdlineBasenameStart, int* pCommStart, int* pCommEnd) {
static bool findCommInCmdline(const char* comm, const char* cmdline, size_t cmdlineBasenameStart, size_t* pCommStart, size_t* pCommLen) {
/* Try to find procComm in tokenized cmdline - this might in rare cases
* mis-identify a string or fail, if comm or cmdline had been unsuitably
* modified by the process */
const char* tokenBase;
size_t tokenLen;
const size_t commLen = strlen(comm);

if (cmdlineBasenameStart < 0)
return false;

for (const char* token = cmdline + cmdlineBasenameStart; *token;) {
for (tokenBase = token; *token && *token != '\n'; ++token) {
if (*token == '/') {
tokenBase = token + 1;
}
}
tokenLen = token - tokenBase;
tokenLen = (size_t)(token - tokenBase);

if ((tokenLen == commLen || (tokenLen > commLen && commLen == (TASK_COMM_LEN - 1))) &&
strncmp(tokenBase, comm, commLen) == 0) {
*pCommStart = tokenBase - cmdline;
*pCommEnd = token - cmdline;
*pCommStart = (size_t)(tokenBase - cmdline);
*pCommLen = tokenLen;
return true;
}

Expand All @@ -99,15 +96,14 @@ static bool findCommInCmdline(const char* comm, const char* cmdline, int cmdline
return false;
}

static int matchCmdlinePrefixWithExeSuffix(const char* cmdline, int cmdlineBaseOffset, const char* exe, int exeBaseOffset, int exeBaseLen) {
int matchLen; /* matching length to be returned */
char delim; /* delimiter following basename */
static size_t matchCmdlinePrefixWithExeSuffix(const char* cmdline, size_t cmdlineBaseOffset, const char* exe, size_t exeBaseOffset, size_t exeBaseLen) {
size_t matchLen; /* matching length to be returned */

/* cmdline prefix is an absolute path: it must match whole exe. */
if (cmdline[0] == '/') {
matchLen = exeBaseLen + exeBaseOffset;
if (strncmp(cmdline, exe, matchLen) == 0) {
delim = cmdline[matchLen];
char delim = cmdline[matchLen];
if (delim == 0 || delim == '\n' || delim == ' ') {
return matchLen;
}
Expand All @@ -127,29 +123,33 @@ static int matchCmdlinePrefixWithExeSuffix(const char* cmdline, int cmdlineBaseO
*
* So if needed, we adjust cmdlineBaseOffset to the previous (if any)
* component of the cmdline relative path, and retry the procedure. */
bool delimFound; /* if valid basename delimiter found */
bool delimFound = true; /* if valid basename delimiter found */
do {
/* match basename */
matchLen = exeBaseLen + cmdlineBaseOffset;
if (cmdlineBaseOffset < exeBaseOffset &&
strncmp(cmdline + cmdlineBaseOffset, exe + exeBaseOffset, exeBaseLen) == 0) {
delim = cmdline[matchLen];
char delim = cmdline[matchLen];
if (delim == 0 || delim == '\n' || delim == ' ') {
int i, j;
size_t i, j;
/* reverse match the cmdline prefix and exe suffix */
for (i = cmdlineBaseOffset - 1, j = exeBaseOffset - 1;
i >= 0 && j >= 0 && cmdline[i] == exe[j]; --i, --j)
i != (size_t)-1 && j != (size_t)-1 && cmdline[i] == exe[j]; --i, --j)
;

/* full match, with exe suffix being a valid relative path */
if (i < 0 && j >= 0 && exe[j] == '/')
if (i == (size_t)-1 && j != (size_t)-1 && exe[j] == '/')
return matchLen;
}
}

/* Try to find the previous potential cmdlineBaseOffset - it would be
* preceded by '/' or nothing, and delimited by ' ' or '\n' */
for (delimFound = false, cmdlineBaseOffset -= 2; cmdlineBaseOffset > 0; --cmdlineBaseOffset) {
delimFound = false;
if (cmdlineBaseOffset <= 2) {
return 0;
}
for (cmdlineBaseOffset -= 2; cmdlineBaseOffset > 0; --cmdlineBaseOffset) {
if (delimFound) {
if (cmdline[cmdlineBaseOffset - 1] == '/') {
break;
Expand Down Expand Up @@ -309,17 +309,36 @@ void Process_makeCommandStr(Process* this, const Settings* settings) {
char* strStart = mc->str;
char* str = strStart;

int cmdlineBasenameStart = this->cmdlineBasenameStart;
int cmdlineBasenameEnd = this->cmdlineBasenameEnd;
size_t cmdlineBasenameStart = this->cmdlineBasenameStart;
size_t cmdlineBasenameEnd = this->cmdlineBasenameEnd;

if (!cmdline) {
cmdlineBasenameStart = 0;
cmdlineBasenameEnd = 0;
cmdline = "(zombie)";
}

assert(cmdlineBasenameStart >= 0);
assert(cmdlineBasenameStart <= (int)strlen(cmdline));
assert(cmdlineBasenameStart <= strlen(cmdline));

size_t exeLen = 0;
size_t exeBasenameOffset = 0;
size_t exeBasenameLen = 0;
size_t matchLen = 0;
if (procExe) {
exeLen = strlen(procExe);
exeBasenameOffset = this->procExeBasenameOffset;
exeBasenameLen = exeLen - exeBasenameOffset;

assert(exeBasenameOffset <= strlen(procExe));

if (this->cmdline) {
matchLen = matchCmdlinePrefixWithExeSuffix(this->cmdline, cmdlineBasenameStart, procExe, exeBasenameOffset, exeBasenameLen);
}
if (matchLen) {
cmdlineBasenameStart = matchLen - exeBasenameLen;
cmdlineBasenameEnd = matchLen;
}
}

if (!showMergedCommand || !procExe || !procComm) { /* fall back to cmdline */
if ((showMergedCommand || (Process_isUserlandThread(this) && showThreadNames)) && procComm && strlen(procComm)) { /* set column to or prefix it with comm */
Expand Down Expand Up @@ -350,24 +369,48 @@ void Process_makeCommandStr(Process* this, const Settings* settings) {
return;
}

int exeLen = strlen(this->procExe);
int exeBasenameOffset = this->procExeBasenameOffset;
int exeBasenameLen = exeLen - exeBasenameOffset;

assert(exeBasenameOffset >= 0);
assert(exeBasenameOffset <= (int)strlen(procExe));
size_t commLen = 0;

bool haveCommInExe = false;
if (procExe && procComm && (!Process_isUserlandThread(this) || showThreadNames)) {
haveCommInExe = strncmp(procExe + exeBasenameOffset, procComm, TASK_COMM_LEN - 1) == 0;
}
if (haveCommInExe) {
commLen = exeBasenameLen;
}

bool haveCommInCmdline = false;
size_t commStart = 0;

if (!haveCommInExe && this->cmdline && procComm && searchCommInCmdline && (!Process_isUserlandThread(this) || showThreadNames)) {
haveCommInCmdline = findCommInCmdline(procComm, cmdline, cmdlineBasenameStart, &commStart, &commLen);
}

if (!stripExeFromCmdline) {
matchLen = 0;
}
if (matchLen) {
/* strip the matched exe prefix */
cmdline += matchLen;

if (haveCommInCmdline) {
if (commStart == cmdlineBasenameStart) {
haveCommInExe = true;
haveCommInCmdline = false;
commStart = 0;
} else {
assert(commStart >= matchLen);
commStart -= matchLen;
}
}
}

/* Start with copying exe */
if (showProgramPath) {
if (shadowDistPathPrefix)
CHECK_AND_MARK_DIST_PATH_PREFIXES(procExe);
if (haveCommInExe)
WRITE_HIGHLIGHT(exeBasenameOffset, exeBasenameLen, commAttr, CMDLINE_HIGHLIGHT_FLAG_COMM);
WRITE_HIGHLIGHT(exeBasenameOffset, commLen, commAttr, CMDLINE_HIGHLIGHT_FLAG_COMM);
WRITE_HIGHLIGHT(exeBasenameOffset, exeBasenameLen, baseAttr, CMDLINE_HIGHLIGHT_FLAG_BASENAME);
if (this->procExeDeleted)
WRITE_HIGHLIGHT(exeBasenameOffset, exeBasenameLen, delExeAttr, CMDLINE_HIGHLIGHT_FLAG_DELETED);
Expand All @@ -376,7 +419,7 @@ void Process_makeCommandStr(Process* this, const Settings* settings) {
str = stpcpy(str, procExe);
} else {
if (haveCommInExe)
WRITE_HIGHLIGHT(0, exeBasenameLen, commAttr, CMDLINE_HIGHLIGHT_FLAG_COMM);
WRITE_HIGHLIGHT(0, commLen, commAttr, CMDLINE_HIGHLIGHT_FLAG_COMM);
WRITE_HIGHLIGHT(0, exeBasenameLen, baseAttr, CMDLINE_HIGHLIGHT_FLAG_BASENAME);
if (this->procExeDeleted)
WRITE_HIGHLIGHT(0, exeBasenameLen, delExeAttr, CMDLINE_HIGHLIGHT_FLAG_DELETED);
Expand All @@ -385,18 +428,6 @@ void Process_makeCommandStr(Process* this, const Settings* settings) {
str = stpcpy(str, procExe + exeBasenameOffset);
}

bool haveCommInCmdline = false;
int commStart = 0;
int commEnd = 0;

/* Try to match procComm with procExe's basename: This is reliable (predictable) */
if (searchCommInCmdline) {
/* commStart/commEnd will be adjusted later along with cmdline */
haveCommInCmdline = (!Process_isUserlandThread(this) || showThreadNames) && findCommInCmdline(procComm, cmdline, cmdlineBasenameStart, &commStart, &commEnd);
}

int matchLen = matchCmdlinePrefixWithExeSuffix(cmdline, cmdlineBasenameStart, procExe, exeBasenameOffset, exeBasenameLen);

bool haveCommField = false;

if (!haveCommInExe && !haveCommInCmdline && procComm && (!Process_isUserlandThread(this) || showThreadNames)) {
Expand All @@ -406,18 +437,6 @@ void Process_makeCommandStr(Process* this, const Settings* settings) {
haveCommField = true;
}

if (matchLen) {
if (stripExeFromCmdline) {
/* strip the matched exe prefix */
cmdline += matchLen;

commStart -= matchLen;
commEnd -= matchLen;
} else {
matchLen = 0;
}
}

if (!matchLen || (haveCommField && *cmdline)) {
/* cmdline will be a separate field */
WRITE_SEPARATOR;
Expand All @@ -427,7 +446,7 @@ void Process_makeCommandStr(Process* this, const Settings* settings) {
CHECK_AND_MARK_DIST_PATH_PREFIXES(cmdline);

if (!haveCommInExe && haveCommInCmdline && !haveCommField && (!Process_isUserlandThread(this) || showThreadNames))
WRITE_HIGHLIGHT(commStart, commEnd - commStart, commAttr, CMDLINE_HIGHLIGHT_FLAG_COMM);
WRITE_HIGHLIGHT(commStart, commLen, commAttr, CMDLINE_HIGHLIGHT_FLAG_COMM);

/* Display cmdline if it hasn't been consumed by procExe */
if (*cmdline)
Expand All @@ -445,20 +464,20 @@ void Process_writeCommand(const Process* this, int attr, int baseAttr, RichStrin
const ProcessMergedCommand* mc = &this->mergedCommand;
const char* mergedCommand = mc->str;

int strStart = RichString_size(str);
size_t strStart = RichString_size(str);

const Settings* settings = this->super.host->settings;
const bool highlightBaseName = settings->highlightBaseName;
const bool highlightSeparator = true;
const bool highlightDeleted = settings->highlightDeletedExe;

if (!mergedCommand) {
int len = 0;
size_t len = 0;
const char* cmdline = this->cmdline;

if (highlightBaseName || !settings->showProgramPath) {
int basename = 0;
for (int i = 0; i < this->cmdlineBasenameEnd; i++) {
size_t basename = 0;
for (size_t i = 0; i < this->cmdlineBasenameEnd; i++) {
if (cmdline[i] == '/') {
basename = i + 1;
} else if (cmdline[i] == ':') {
Expand Down Expand Up @@ -850,7 +869,7 @@ bool Process_rowMatchesFilter(const Row* super, const Table* table) {
void Process_init(Process* this, const Machine* host) {
Row_init(&this->super, host);

this->cmdlineBasenameEnd = -1;
this->cmdlineBasenameEnd = 0;
this->st_uid = (uid_t)-1;
}

Expand Down Expand Up @@ -1002,12 +1021,12 @@ void Process_updateComm(Process* this, const char* comm) {
this->mergedCommand.lastUpdate = 0;
}

static int skipPotentialPath(const char* cmdline, int end) {
static size_t skipPotentialPath(const char* cmdline, size_t end) {
if (cmdline[0] != '/')
return 0;

int slash = 0;
for (int i = 1; i < end; i++) {
size_t slash = 0;
for (size_t i = 1; i < end; i++) {
if (cmdline[i] == '/' && cmdline[i + 1] != '\0') {
slash = i + 1;
continue;
Expand All @@ -1023,11 +1042,10 @@ static int skipPotentialPath(const char* cmdline, int end) {
return slash;
}

void Process_updateCmdline(Process* this, const char* cmdline, int basenameStart, int basenameEnd) {
assert(basenameStart >= 0);
assert((cmdline && basenameStart < (int)strlen(cmdline)) || (!cmdline && basenameStart == 0));
void Process_updateCmdline(Process* this, const char* cmdline, size_t basenameStart, size_t basenameEnd) {
assert((cmdline && basenameStart < strlen(cmdline)) || (!cmdline && basenameStart == 0));
assert((basenameEnd > basenameStart) || (basenameEnd == 0 && basenameStart == 0));
assert((cmdline && basenameEnd <= (int)strlen(cmdline)) || (!cmdline && basenameEnd == 0));
assert((cmdline && basenameEnd <= strlen(cmdline)) || (!cmdline && basenameEnd == 0));

if (!this->cmdline && !cmdline)
return;
Expand Down Expand Up @@ -1060,7 +1078,7 @@ void Process_updateExe(Process* this, const char* exe) {
if (exe) {
this->procExe = xStrdup(exe);
const char* lastSlash = strrchr(exe, '/');
this->procExeBasenameOffset = (lastSlash && *(lastSlash + 1) != '\0' && lastSlash != exe) ? (lastSlash - exe + 1) : 0;
this->procExeBasenameOffset = (lastSlash > exe && *(lastSlash + 1) != '\0') ? (size_t)(lastSlash - exe + 1) : 0;
} else {
this->procExe = NULL;
this->procExeBasenameOffset = 0;
Expand Down
8 changes: 4 additions & 4 deletions Process.h
Original file line number Diff line number Diff line change
Expand Up @@ -129,10 +129,10 @@ typedef struct Process_ {
char* cmdline;

/* End Offset in cmdline of the process basename */
int cmdlineBasenameEnd;
size_t cmdlineBasenameEnd;

/* Start Offset in cmdline of the process basename */
int cmdlineBasenameStart;
size_t cmdlineBasenameStart;

/* The process' "command" name */
char* procComm;
Expand All @@ -144,7 +144,7 @@ typedef struct Process_ {
char* procCwd;

/* Offset in procExe of the process basename */
int procExeBasenameOffset;
size_t procExeBasenameOffset;

/* Tells if the executable has been replaced in the filesystem since start */
bool procExeDeleted;
Expand Down Expand Up @@ -326,7 +326,7 @@ int Process_compareByKey_Base(const Process* p1, const Process* p2, ProcessField
const char* Process_getCommand(const Process* this);

void Process_updateComm(Process* this, const char* comm);
void Process_updateCmdline(Process* this, const char* cmdline, int basenameStart, int basenameEnd);
void Process_updateCmdline(Process* this, const char* cmdline, size_t basenameStart, size_t basenameEnd);
void Process_updateExe(Process* this, const char* exe);

/* This function constructs the string that is displayed by
Expand Down
4 changes: 2 additions & 2 deletions RichString.c
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ static void RichString_extendLen(RichString* this, int len) {
}

static void RichString_setLen(RichString* this, int len) {
if (len < RICHSTRING_MAXLEN && this->chlen < RICHSTRING_MAXLEN) {
if (len <= RICHSTRING_MAXLEN && this->chlen <= RICHSTRING_MAXLEN) {
RichString_setChar(this, len, 0);
this->chlen = len;
} else {
Expand All @@ -59,7 +59,7 @@ static void RichString_setLen(RichString* this, int len) {
}

void RichString_rewind(RichString* this, int count) {
RichString_setLen(this, this->chlen - count);
RichString_setLen(this, this->chlen > count ? this->chlen - count : 0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to self: Once the types of RichString.chlen and count are changed to unsigned, we can change this expression to a saturatingSub() call to make the expression more concise to read. However, I found missed optimizations with this: In GCC (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=118679) and in Clang
(llvm/llvm-project#124714)

}

#ifdef HAVE_LIBNCURSESW
Expand Down
6 changes: 3 additions & 3 deletions darwin/DarwinProcess.c
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ static void DarwinProcess_updateCmdLine(const struct kinfo_proc* k, Process* pro
/* Save where the argv[0] string starts. */
sp = cp;

int end = 0;
size_t end = 0;
for ( np = NULL; c < nargs && cp < &procargs[size]; cp++ ) {
if ( *cp == '\0' ) {
c++;
Expand All @@ -243,7 +243,7 @@ static void DarwinProcess_updateCmdLine(const struct kinfo_proc* k, Process* pro
/* Note location of current '\0'. */
np = cp;
if (end == 0) {
end = cp - sp;
end = (size_t)(cp - sp);
}
}
}
Expand All @@ -257,7 +257,7 @@ static void DarwinProcess_updateCmdLine(const struct kinfo_proc* k, Process* pro
goto ERROR_B;
}
if (end == 0) {
end = np - sp;
end = (size_t)(np - sp);
}

Process_updateCmdline(proc, sp, 0, end);
Expand Down
Loading