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 all 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
26 changes: 13 additions & 13 deletions CPUMeter.c
Original file line number Diff line number Diff line change
Expand Up @@ -137,44 +137,44 @@ static void CPUMeter_display(const Object* cast, RichString* out) {

len = xSnprintf(buffer, sizeof(buffer), "%5.1f%% ", this->values[CPU_METER_NORMAL]);
Copy link
Member

Choose a reason for hiding this comment

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

Any chance to reduce noise by having size_t len and thus avoiding all the subsequent casts? We know xSnprintf can't be negative, which we already check for.

Suggested change
len = xSnprintf(buffer, sizeof(buffer), "%5.1f%% ", this->values[CPU_METER_NORMAL]);
size_t len = (size_t)xSnprintf(buffer, sizeof(buffer), "%5.1f%% ", this->values[CPU_METER_NORMAL]);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This approach just moves the casts from the RichString append functions to xSnprintf. Unless we alter the API of xSnprintf, I don't think it reduces any noise overall.

How about this alternate solution: Create a RichString_appendAsciiFormat() function that internally combines xSnprintf and RichString_appendnAscii()?

The prototype could look something like this:

/* The "buf" argument is optional. If "buf" is NULL, then this function uses an internally allocated buffer (using xAsprintf).
It's still recommended to supply "buf" for performance reasons. */
ATTR_NONNULL_N(1, 5)
size_t RichString_appendAsciiFormat(RichString* this, int attrs, char* buf, size_t bufsize, const char* format, ...);

ATTR_NONNULL_N(1, 5)
size_t RichString_appendAsciiVFormat(RichString* this, int attrs, char* buf, size_t bufsize, const char* format, va_list vl);

Copy link
Contributor Author

@Explorer09 Explorer09 Jan 20, 2025

Choose a reason for hiding this comment

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

Update: I've added the commit aa74b30.

RichString_appendAscii(out, CRT_colors[METER_TEXT], ":");
RichString_appendnAscii(out, CRT_colors[CPU_NORMAL], buffer, len);
RichString_appendnAscii(out, CRT_colors[CPU_NORMAL], buffer, (unsigned int)len);
if (settings->detailedCPUTime) {
len = xSnprintf(buffer, sizeof(buffer), "%5.1f%% ", this->values[CPU_METER_KERNEL]);
RichString_appendAscii(out, CRT_colors[METER_TEXT], "sy:");
RichString_appendnAscii(out, CRT_colors[CPU_SYSTEM], buffer, len);
RichString_appendnAscii(out, CRT_colors[CPU_SYSTEM], buffer, (unsigned int)len);
len = xSnprintf(buffer, sizeof(buffer), "%5.1f%% ", this->values[CPU_METER_NICE]);
RichString_appendAscii(out, CRT_colors[METER_TEXT], "ni:");
RichString_appendnAscii(out, CRT_colors[CPU_NICE_TEXT], buffer, len);
RichString_appendnAscii(out, CRT_colors[CPU_NICE_TEXT], buffer, (unsigned int)len);
len = xSnprintf(buffer, sizeof(buffer), "%5.1f%% ", this->values[CPU_METER_IRQ]);
RichString_appendAscii(out, CRT_colors[METER_TEXT], "hi:");
RichString_appendnAscii(out, CRT_colors[CPU_IRQ], buffer, len);
RichString_appendnAscii(out, CRT_colors[CPU_IRQ], buffer, (unsigned int)len);
len = xSnprintf(buffer, sizeof(buffer), "%5.1f%% ", this->values[CPU_METER_SOFTIRQ]);
RichString_appendAscii(out, CRT_colors[METER_TEXT], "si:");
RichString_appendnAscii(out, CRT_colors[CPU_SOFTIRQ], buffer, len);
RichString_appendnAscii(out, CRT_colors[CPU_SOFTIRQ], buffer, (unsigned int)len);
if (isNonnegative(this->values[CPU_METER_STEAL])) {
len = xSnprintf(buffer, sizeof(buffer), "%5.1f%% ", this->values[CPU_METER_STEAL]);
RichString_appendAscii(out, CRT_colors[METER_TEXT], "st:");
RichString_appendnAscii(out, CRT_colors[CPU_STEAL], buffer, len);
RichString_appendnAscii(out, CRT_colors[CPU_STEAL], buffer, (unsigned int)len);
}
if (isNonnegative(this->values[CPU_METER_GUEST])) {
len = xSnprintf(buffer, sizeof(buffer), "%5.1f%% ", this->values[CPU_METER_GUEST]);
RichString_appendAscii(out, CRT_colors[METER_TEXT], "gu:");
RichString_appendnAscii(out, CRT_colors[CPU_GUEST], buffer, len);
RichString_appendnAscii(out, CRT_colors[CPU_GUEST], buffer, (unsigned int)len);
}
len = xSnprintf(buffer, sizeof(buffer), "%5.1f%% ", this->values[CPU_METER_IOWAIT]);
RichString_appendAscii(out, CRT_colors[METER_TEXT], "wa:");
RichString_appendnAscii(out, CRT_colors[CPU_IOWAIT], buffer, len);
RichString_appendnAscii(out, CRT_colors[CPU_IOWAIT], buffer, (unsigned int)len);
} else {
len = xSnprintf(buffer, sizeof(buffer), "%5.1f%% ", this->values[CPU_METER_KERNEL]);
RichString_appendAscii(out, CRT_colors[METER_TEXT], "sys:");
RichString_appendnAscii(out, CRT_colors[CPU_SYSTEM], buffer, len);
RichString_appendnAscii(out, CRT_colors[CPU_SYSTEM], buffer, (unsigned int)len);
len = xSnprintf(buffer, sizeof(buffer), "%5.1f%% ", this->values[CPU_METER_NICE]);
RichString_appendAscii(out, CRT_colors[METER_TEXT], "low:");
RichString_appendnAscii(out, CRT_colors[CPU_NICE_TEXT], buffer, len);
RichString_appendnAscii(out, CRT_colors[CPU_NICE_TEXT], buffer, (unsigned int)len);
if (isNonnegative(this->values[CPU_METER_IRQ])) {
len = xSnprintf(buffer, sizeof(buffer), "%5.1f%% ", this->values[CPU_METER_IRQ]);
RichString_appendAscii(out, CRT_colors[METER_TEXT], "vir:");
RichString_appendnAscii(out, CRT_colors[CPU_GUEST], buffer, len);
RichString_appendnAscii(out, CRT_colors[CPU_GUEST], buffer, (unsigned int)len);
}
}

Expand All @@ -187,7 +187,7 @@ static void CPUMeter_display(const Object* cast, RichString* out) {
len = xSnprintf(cpuFrequencyBuffer, sizeof(cpuFrequencyBuffer), "N/A ");
}
RichString_appendAscii(out, CRT_colors[METER_TEXT], "freq: ");
RichString_appendnWide(out, CRT_colors[METER_VALUE], cpuFrequencyBuffer, len);
RichString_appendnAscii(out, CRT_colors[METER_VALUE], cpuFrequencyBuffer, (unsigned int)len);
}

#ifdef BUILD_WITH_CPU_TEMP
Expand All @@ -202,7 +202,7 @@ static void CPUMeter_display(const Object* cast, RichString* out) {
len = xSnprintf(cpuTemperatureBuffer, sizeof(cpuTemperatureBuffer), "%5.1f%sC", cpuTemperature, CRT_degreeSign);
}
RichString_appendAscii(out, CRT_colors[METER_TEXT], "temp:");
RichString_appendnWide(out, CRT_colors[METER_VALUE], cpuTemperatureBuffer, len);
RichString_appendnWide(out, CRT_colors[METER_VALUE], cpuTemperatureBuffer, (unsigned int)len);
}
#endif
}
Expand Down
2 changes: 1 addition & 1 deletion CRT.c
Original file line number Diff line number Diff line change
Expand Up @@ -825,7 +825,7 @@ static int CRT_colorSchemes[LAST_COLORSCHEME][LAST_COLORELEMENT] = {

static bool CRT_retainScreenOnExit = false;

int CRT_scrollHAmount = 5;
unsigned int CRT_scrollHAmount = 5;

int CRT_scrollWheelVAmount = 10;

Expand Down
2 changes: 1 addition & 1 deletion CRT.h
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ extern const int* CRT_colors;

extern int CRT_cursorX;

extern int CRT_scrollHAmount;
extern unsigned int CRT_scrollHAmount;

extern int CRT_scrollWheelVAmount;

Expand Down
2 changes: 1 addition & 1 deletion DiskIOMeter.c
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ static void DiskIOMeter_display(ATTR_UNUSED const Object* cast, RichString* out)

int color = cached_utilisation_diff > 40.0 ? METER_VALUE_NOTICE : METER_VALUE;
int len = xSnprintf(buffer, sizeof(buffer), "%.1f%%", cached_utilisation_diff);
Copy link
Member

Choose a reason for hiding this comment

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

Similar consideration re size_t. Maybe update signature of xSnprintf overall?

RichString_appendnAscii(out, CRT_colors[color], buffer, len);
RichString_appendnAscii(out, CRT_colors[color], buffer, (unsigned int)len);

RichString_appendAscii(out, CRT_colors[METER_TEXT], " read: ");
RichString_appendAscii(out, CRT_colors[METER_VALUE_IOREAD], cached_read_diff_str);
Expand Down
4 changes: 2 additions & 2 deletions FileDescriptorMeter.c
Original file line number Diff line number Diff line change
Expand Up @@ -89,14 +89,14 @@ static void FileDescriptorMeter_display(const Object* cast, RichString* out) {

RichString_appendAscii(out, CRT_colors[METER_TEXT], "used: ");
len = xSnprintf(buffer, sizeof(buffer), "%.0lf", this->values[0]);
RichString_appendnAscii(out, CRT_colors[FILE_DESCRIPTOR_USED], buffer, len);
RichString_appendnAscii(out, CRT_colors[FILE_DESCRIPTOR_USED], buffer, (unsigned int)len);

RichString_appendAscii(out, CRT_colors[METER_TEXT], " max: ");
if (FD_EFFECTIVE_UNLIMITED(this->values[1])) {
RichString_appendAscii(out, CRT_colors[FILE_DESCRIPTOR_MAX], "unlimited");
} else {
len = xSnprintf(buffer, sizeof(buffer), "%.0lf", this->values[1]);
RichString_appendnAscii(out, CRT_colors[FILE_DESCRIPTOR_MAX], buffer, len);
RichString_appendnAscii(out, CRT_colors[FILE_DESCRIPTOR_MAX], buffer, (unsigned int)len);
}
}

Expand Down
8 changes: 4 additions & 4 deletions LoadAverageMeter.c
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,11 @@ static void LoadAverageMeter_display(const Object* cast, RichString* out) {
int len;

len = xSnprintf(buffer, sizeof(buffer), "%.2f ", this->values[0]);
RichString_appendnAscii(out, CRT_colors[LOAD_AVERAGE_ONE], buffer, len);
RichString_appendnAscii(out, CRT_colors[LOAD_AVERAGE_ONE], buffer, (unsigned int)len);
len = xSnprintf(buffer, sizeof(buffer), "%.2f ", this->values[1]);
RichString_appendnAscii(out, CRT_colors[LOAD_AVERAGE_FIVE], buffer, len);
RichString_appendnAscii(out, CRT_colors[LOAD_AVERAGE_FIVE], buffer, (unsigned int)len);
len = xSnprintf(buffer, sizeof(buffer), "%.2f ", this->values[2]);
RichString_appendnAscii(out, CRT_colors[LOAD_AVERAGE_FIFTEEN], buffer, len);
RichString_appendnAscii(out, CRT_colors[LOAD_AVERAGE_FIFTEEN], buffer, (unsigned int)len);
}

static void LoadMeter_updateValues(Meter* this) {
Expand Down Expand Up @@ -98,7 +98,7 @@ static void LoadMeter_display(const Object* cast, RichString* out) {
int len;

len = xSnprintf(buffer, sizeof(buffer), "%.2f ", this->values[0]);
RichString_appendnAscii(out, CRT_colors[LOAD], buffer, len);
RichString_appendnAscii(out, CRT_colors[LOAD], buffer, (unsigned int)len);
}

const MeterClass LoadAverageMeter_class = {
Expand Down
3 changes: 2 additions & 1 deletion MainPanel.c
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ in the source distribution for its full text.
#include "MainPanel.h"

#include <ctype.h>
#include <stddef.h>
#include <stdlib.h>
#include <sys/types.h>

Expand Down Expand Up @@ -81,7 +82,7 @@ static HandlerResult MainPanel_eventHandler(Panel* super, int ch) {

if (EVENT_IS_HEADER_CLICK(ch)) {
int x = EVENT_HEADER_CLICK_GET_X(ch);
int hx = super->scrollH + x + 1;
size_t hx = super->scrollH + (unsigned int)x + 1;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the second cast also be size_t?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BenBE Let me explain a bit:

  • signed int cast to size_t: Perform a sign extension.
  • unsigned int cast to size_t: Perform a zero extension.

When I made this patch, I made a personal rule that, if a zero extension would suffice, I would avoid sign extension. This also applies to the noisy-as-you-called-it changes about me casting the xSnprintf results to size_t.

RowField field = RowField_keyAt(settings, hx);
if (ss->treeView && ss->treeViewAlwaysByPID) {
ss->treeView = false;
Expand Down
77 changes: 38 additions & 39 deletions Meter.c
Original file line number Diff line number Diff line change
Expand Up @@ -96,46 +96,44 @@ static void BarMeterMode_draw(Meter* this, int x, int y, int w) {
// The text in the bar is right aligned;
// Pad with maximal spaces and then calculate needed starting position offset
RichString_begin(bar);
RichString_appendChr(&bar, 0, ' ', w);
RichString_appendChr(&bar, 0, ' ', (unsigned int)w);
Copy link
Member

Choose a reason for hiding this comment

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

This looks overall kinda noisy. The source code shouldn't be a casting show … ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the problem of policy and whether the compiler would complain about implicit casts to unsigned. Given there is a conditional if (w < 1) return; a few lines earlier, compiler should detect that the w value cannot go negative in this case, but I can't say for sure.

If htop is compiled with gcc or clang's -Wsign-conversion flag, will that cause a warning?

RichString_appendWide(&bar, 0, this->txtBuffer);

int startPos = RichString_sizeVal(bar) - w;
if (startPos > w) {
size_t startPos = RichString_sizeVal(bar) - (unsigned int)w;
if (startPos > (unsigned int)w) {
// Text is too large for bar
// Truncate meter text at a space character
for (int pos = 2 * w; pos > w; pos--) {
for (unsigned int pos = 2 * (unsigned int)w; pos > (unsigned int)w; pos--) {
if (RichString_getCharVal(bar, pos) == ' ') {
while (pos > w && RichString_getCharVal(bar, pos - 1) == ' ')
while (pos > (unsigned int)w && RichString_getCharVal(bar, pos - 1) == ' ')
pos--;
startPos = pos - w;
startPos = pos - (unsigned int)w;
break;
}
}

// If still too large, print the start not the end
startPos = MINIMUM(startPos, w);
startPos = MINIMUM(startPos, (unsigned int)w);
}

assert(startPos >= 0);
assert(startPos <= w);
assert(startPos + w <= RichString_sizeVal(bar));
assert(startPos <= (unsigned int)w);
assert(startPos + (unsigned int)w <= RichString_sizeVal(bar));

int blockSizes[10];
unsigned int blockSizes[10];

// First draw in the bar[] buffer...
int offset = 0;
unsigned int offset = 0;
for (uint8_t i = 0; i < this->curItems; i++) {
double value = this->values[i];
if (isPositive(value) && this->total > 0.0) {
value = MINIMUM(value, this->total);
blockSizes[i] = ceil((value / this->total) * w);
blockSizes[i] = (unsigned int)(int)ceil((value / this->total) * w);
blockSizes[i] = MINIMUM(MINIMUM(INT_MAX - (unsigned int)x, (unsigned int)w) - offset, blockSizes[i]);
} else {
blockSizes[i] = 0;
}
int nextOffset = offset + blockSizes[i];
// (Control against invalid values)
nextOffset = CLAMP(nextOffset, 0, w);
for (int j = offset; j < nextOffset; j++)
unsigned int nextOffset = offset + blockSizes[i];
for (unsigned int j = offset; j < nextOffset; j++)
if (RichString_getCharVal(bar, startPos + j) == ' ') {
if (CRT_colorScheme == COLORSCHEME_MONOCHROME) {
assert(i < strlen(BarMeterMode_characters));
Expand All @@ -152,13 +150,12 @@ static void BarMeterMode_draw(Meter* this, int x, int y, int w) {
for (uint8_t i = 0; i < this->curItems; i++) {
int attr = this->curAttributes ? this->curAttributes[i] : Meter_attributes(this)[i];
RichString_setAttrn(&bar, CRT_colors[attr], startPos + offset, blockSizes[i]);
RichString_printoffnVal(bar, y, x + offset, startPos + offset, MINIMUM(blockSizes[i], w - offset));
RichString_printoffnVal(bar, y, x + (int)offset, startPos + offset, (int)blockSizes[i]);
offset += blockSizes[i];
offset = CLAMP(offset, 0, w);
}
if (offset < w) {
RichString_setAttrn(&bar, CRT_colors[BAR_SHADOW], startPos + offset, w - offset);
RichString_printoffnVal(bar, y, x + offset, startPos + offset, w - offset);
if (offset < (unsigned int)w) {
RichString_setAttrn(&bar, CRT_colors[BAR_SHADOW], startPos + offset, (unsigned int)w - offset);
RichString_printoffnVal(bar, y, x + (int)offset, startPos + offset, w - (int)offset);
}

RichString_delete(&bar);
Expand Down Expand Up @@ -319,26 +316,28 @@ static void LEDMeterMode_draw(Meter* this, int x, int y, int w) {
attrset(CRT_colors[LED_COLOR]);
const char* caption = Meter_getCaption(this);
mvaddstr(yText, x, caption);
int xx = x + strlen(caption);
int len = RichString_sizeVal(out);
for (int i = 0; i < len; i++) {
int c = RichString_getCharVal(out, i);
if (c >= '0' && c <= '9') {
if (xx - x + 4 > w)
break;

LEDMeterMode_drawDigit(xx, y, c - '0');
xx += 4;
} else {
if (xx - x + 1 > w)
break;
if (strlen(caption) <= INT_MAX - (unsigned int)x) {
int xx = x + (int)strlen(caption);
size_t len = RichString_sizeVal(out);
for (size_t i = 0; i < len; i++) {
int c = RichString_getCharVal(out, i);
if (c >= '0' && c <= '9') {
if (xx - x + 4 > w)
break;

LEDMeterMode_drawDigit(xx, y, c - '0');
xx += 4;
} else {
if (xx - x + 1 > w)
break;
#ifdef HAVE_LIBNCURSESW
const cchar_t wc = { .chars = { c, '\0' }, .attr = 0 }; /* use LED_COLOR from attrset() */
mvadd_wch(yText, xx, &wc);
const cchar_t wc = { .chars = { c, '\0' }, .attr = 0 }; /* use LED_COLOR from attrset() */
mvadd_wch(yText, xx, &wc);
#else
mvaddch(yText, xx, c);
mvaddch(yText, xx, c);
#endif
xx += 1;
xx += 1;
}
}
}
attrset(CRT_colors[RESET_COLOR]);
Expand Down
2 changes: 1 addition & 1 deletion NetworkIOMeter.c
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ static void NetworkIOMeter_display(ATTR_UNUSED const Object* cast, RichString* o
RichString_appendAscii(out, CRT_colors[METER_VALUE_IOWRITE], "iB/s");

int len = xSnprintf(buffer, sizeof(buffer), " (%u/%u pkts/s) ", cached_rxp_diff, cached_txp_diff);
RichString_appendnAscii(out, CRT_colors[METER_TEXT], buffer, len);
RichString_appendnAscii(out, CRT_colors[METER_TEXT], buffer, (unsigned int)len);
}

const MeterClass NetworkIOMeter_class = {
Expand Down
2 changes: 1 addition & 1 deletion OptionItem.c
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ static void NumberItem_display(const Object* cast, RichString* out) {
} else {
written = xSnprintf(buffer, sizeof(buffer), "%d", NumberItem_get(this));
}
RichString_appendnAscii(out, CRT_colors[CHECK_MARK], buffer, written);
RichString_appendnAscii(out, CRT_colors[CHECK_MARK], buffer, (unsigned int)written);
RichString_appendAscii(out, CRT_colors[CHECK_BOX], "]");
for (int i = written; i < 5; i++) {
RichString_appendAscii(out, CRT_colors[CHECK_BOX], " ");
Expand Down
Loading
Loading