Upgrade lengths of RichString to `size_t` & many related changes.

[Explorer09]

The main goal of this PR is to upgrade the "length" parameters and properties of RichString class from int to size_t. While doing the change, there are other code quality improvements that are related or required to the data type migrations.

Important changes of these include:

1. The /proc/*/cmdline basename matching algorithm is slightly changed to utilize the /proc/*/exe matching when possible. (Commit d384376d90b321c55795ed3d1a39d670a24e4c9f)
2. Add a special case to /proc/*/comm string highlighting. (This is required to prevent an internal variable commStart to go negative.)
3. Convert to size_t for cmdlineBasenameStart, cmdlineBasenameEnd and procExeBasenameOffset values in Process class. (#1588)
4. Add additional limit clamping to Panel.cursorX value.
5. Convert to size_t for selectedLen and scrollH values in Panel class. (This incorporates #1585)
6. Incorporate #1586 and fix mbstowcs_nonfatal(). (It's better for that PR to apply after the RichString lengths migrated to size_t type.)

[Explorer09]

There'S a lot of noise introduced due to the (size_t)len casts on RichString_appendnAscii calls, most of which could be avoided if we made xSnprintf return size_t. Can you take a look in that direction?

Because snprintf(3) only returns int, the return type that makes more sense for xSnprintf is unsigned int, not size_t.

Even though I can review all calls of xSnprintf and update the return types, I fear that would create more noise for the changes, not less. More importantly, this would take more time of me.

[fasterit]

I don't like these type of "my vscode refactoring plugin had a go at the codebase" changes. There's lots of noise and it fully breaks backportability of changes to stable releases (see my Debian hat). Granted, you have done nice, small commits and for 3-4 of them there is a rationale, like "Fix potential out of bound access" or the N-in-Unicode != N-in-Ascii. I'd cherry pick these to a 3.4.1 release. /DLange

[Explorer09]

Granted, you have done nice, small commits and for 3-4 of them there is a rationale, like "Fix potential out of bound access" or the N-in-Unicode != N-in-Ascii. I'd cherry pick these to a 3.4.1 release. /DLange

Thank you. I did intend you maintainers to cherry-pick some small commits before merging the whole thing into the mainline. (There are still style issues that need to be discussed, as you can see.)

I don't like these type of "my vscode refactoring plugin had a go at the codebase" changes. There's lots of noise and it fully breaks backportability of changes to stable releases (see my Debian hat).

It's not the "vscode refactoring plugin" or anything like that. In fact I didn't use VSCode when refactoring this. It is my defensive coding style because of my previous experiences working in a company (that does use MSVC for compiling projects).

I'm not expecting this one gets merged quickly. What I was suggesting are things that should have been done to htop for long term maintainability. Perhaps for the next major release (when backportability is less of an issue).

[Explorer09]

@BenBE I have a question to ask when I try to improve the code of RichString...

I can't get the idea of what the commit a6306f090abcb1316a81444dc5514d236409349b (#1434) was trying to fix. While the PR mentioned a compiler warning about a memory leak, was that leak actually possible, or was that a false positive and the change was meant to silence it?

I could't think of a case where the buffer can actually leak, and thus I thought it was a false positive. Yet I'm not sure about that.

[BenBE]

Yes, the leak was possible. Have a really close look at the trace in the compiler warning from the referenced issue.

I'd have to look at the previous state of that code before that fix, but IIRC the issue occurred, when the buffer was allocated but resized to be below the allocation limit in a later code path. In that case, the buffer no longer gets free'd since it's too small for the logic that frees the memory in the end.

[Explorer09]

@BenBE I still didn't get how the leak can happen. For what I've known, the chlen property is only controlled by the RichString_setLen() function. Unless chlen is modified from outside the function, a RichString instance should have a consistent state: this->chlen <= RICHSTRING_MAXLEN if and only if this->chptr == this->chstr. What could possibly go wrong?