This should also carry the buffer size, so the internal EncodeWChar routines can perform bound checking.

Also note, that the CGroup code deliberately uses two distinct functions (one for counting the size, another for actually writing) and that these functions return success (character properly counted/written to buffer) or failure (OOB write). Similar could be done here too.

I choose to make the size counting and the actual writing the same routine for a reason: I expect the "rules" for filtering characters be tweakable in the future. If there are additional characters besides !iswprint that need to be filtered out (replaced with U+FFFD), people only need to modify one routine for that.

I'm no sure about bound checking yet. I can add it into the struct, but it would be for assertions only, as I always expect the buffer allocated would just fit. (That is, no truncation.)

Update: I've changed my mind about the bound checking here. Considering the standard library functions mbsrtowcs(3) and wcsrtombs(3) both allow limits to the destination buffer size, I can add it to the WCharEncoderState as a feature.

I still don't recommend it though. The string buffer would not be null-terminated when the truncation occurs, and I won't fix that (for multi-byte encodings other than UTF-8, it is allowed that wcrtomb(buf, L'\0', ps) returns a length greater than 1).

That's what "Failure to write to the buffer" could be used for. If the EncodeWChar function sees that the buffer is full and not NUL-terminated, it should return false (AKA failure), which all callees could propagate accordingly. Similar to what the CGROUP compression code¹ does …

¹Actually that code over-allocates by one byte and forces the NUL-termination in that over-allocated byte. Cf. linux/CGroupUtils.c in functions CGroup_filterName and CGroup_filterContainer.

@BenBE Nope. After a bit of consideration, this length check cannot be done in EncodeWChar like in the CGROUP code. I wrote a comment on why instead.

It's a limitation of the wcrtomb(3) interface. When we can detect wcrtomb writing out of bounds we might be too late (access violation already happens). wcrtomb doesn't give us the "length check before writing" we need. Trying to workaround that would make code unnecessarily complicated (wcrtomb called one more time - three times in total).

Also, we cannot put the '\0' byte in arbitrary position. Unlike in the CGROUP code, we can only put '\0' on character boundaries here. So overall there's nothing I can do.

Yes. And with the implementation used in the CGroup code, a false returned from the buffer write callback would immediately terminate processing. That's basically the same semantics used there too.

@BenBE That means an additional memcpy call, slowing things down slightly. For checking an error that should never happen unless there's a bug in code. I don't really like this.

That memcpy is from L1 cache to a buffer that's likely in L1 too. As mentioned elsewhere: If in doubt, I prefer correctness over performance.

And if you want to avoid this, over-allocate by MB_LEN_MAX + 1 and you can always write to ps->buf, if you force the NUL terminator on error.

And if you want to avoid this, over-allocate by MB_LEN_MAX + 1 and you can always write to ps->buf, if you force the NUL terminator on error.

Over-allocation to solve the (actually non-existent) problem of under-allocation. Sorry, but I'm irritated by this logic.

The whole point of adding the bound check by your suggestion was to correct the case of under-allocation (allocated buffer size is less than required). While I argued first that the problem literally won't exist, I agree on the point that we can assert and detect the error, at least as a precaution. The buffer should be allocated with the right size already, thus any code that attempts to correct or recover from any potential error is useless. I might just make the error fatal instead and call it a day.

if (ps->buf && len <= ps->size - ps->pos) {
   fail();
}

As for why there is no NUL termination for the string when odds happen here: We are using a multibyte encoder wcrtomb that is capable of handling any encoding, not just UTF-8. When interpreting the standard behavior of wcrtomb in the strict sense, it is not necessary for a null wide character to be encoded as 1 byte. The standard wcrtomb will output shift sequences (if any) that restore the shift state to the initial state before outputting the null byte itself. Since that would require extra buffer space (up to MB_CUR_MAX bytes), any truncation of a multibyte string would have to be decided beforehand, and it won't be an escape hatch for any buffer size problem.

ACK on making the error fatal.

Anyway, a good API makes it impossible to cause issues by abusing it. Thus if wcrtomb may write up to MB_LEN_MAX bytes you better should pass a buffer that can hold MB_LEN_MAX bytes, i.e. ensure the remaining space is large enough. Even if the actual call is known to write less.

And adding MB_LEN_MAX + 1 to the allocation won't cause any noticeable overhead (malloc will just allocate the same and in rare occasions or under memory pressure may delay due to the OOM killer sweeping some swap …

Why introduce this extra variable? Was this patch written by AI? ;-)

Technically no AI is used :)

The ptr is a temporary variable holding a pointer because of the API design I explained in https://github.com/htop-dev/htop/pull/1642/changes/BASE..6b82bcef3b8f7aecd9be6c83d66e84af152a1d58#r3211053429

Naming consistency String_encodeWChar vs. String_decodeNextWChar.

Also, any reason to have this function exported from this TU?

There's a reason for asymmetric naming here. (You may suggest a better naming.)

encodeWChar() and decodeNextWChar() are for low-level encoder and decoder with states.

For the decoder:

bool String_decodeNextWChar(MBStringDecoderState* ps);

The input string is specified as ps->str and the next character decoded will be outputted in ps->ch.

For the encoder:

/* The typedef is to allow different encoders to coexist. I have thought of an encoder that would present all non-printable characters as "\xXXXX" escape sequence. */
typedef ATTR_NONNULL void (*EncodeWChar)(WCharEncoderState* ps, /*wchar_t or int*/ wc);
void String_encodeWChar(WCharEncoderState* ps, /*wchar_t or int*/ wc);

The next character that would be encoded is specified as wc argument here and will be appended to the end of ps->buf. The WCharEncoderState object does not otherwise know the next wchar_t to encode.

Perhaps the name String_encodeWChar is obscure on what it does. Maybe the name WCharEncoder_appendWChar will be better? I'm not sure yet.

Why have the public API of this function modify an input argument to move to the end of the function?

This function has two outputs: One is the terminal width (i.e. num of terminal columns) and the other is the end character position after calculating the width.

The second output is useful to tell, for example, how many Unicode characters would it take to fit the number of terminal columns.

This functions has two uses combined in a single API: (1) to count the number of terminal columns for a given multibyte string; (2) to count the number characters it takes to fit the given terminal width.

It might seem redundant, but I'd prefer if this output parameter was a separate (NULL-able) argument to the function. This separates concerns (input argument vs. optional additional output) and makes the API clearer. This also allows to just skip the extra local variable in case you don't care AND don't have it unexpectedly be changed.

It might seem redundant, but I'd prefer if this output parameter was a separate (NULL-able) argument to the function. This separates concerns (input argument vs. optional additional output) and makes the API clearer. This also allows to just skip the extra local variable in case you don't care AND don't have it unexpectedly be changed.

A static inline function that wraps around this String_mbswidth() for a more convenient output interface? That wouldn't hurt, I guess.

By the way, the "input pointer that would be modified on output" is not my original idea. There are precedents in mbsrtowcs(3) and mbsnrtowcs(3) (compare mbsrtowcs(3) with mbstowcs(3) and I would argue that the const char** argument in the former makes the API more flexible).

I gave both variants of the API to Copilot to compare (without initially disclosing what the function would do) and except for "if it runs in a loop for parsing" the additional out argument was the preferred variant. This only solidified further once I gave Copilot also some more semantics of the function it just had rated. One interesting aspect was that modern compiler tend to optimize the additional out parameter variant better. So there's several aspects that lean towards that variant of the API.

Using the C standard library's string function API as a baseline for API quality sets a very low bar for quality … ;-)

I gave both variants of the API to Copilot to compare (without initially disclosing what the function would do) and except for "if it runs in a loop for parsing" the additional out argument was the preferred variant.

What variants of the API did you give to Copilot in particular?

One interesting aspect was that modern compiler tend to optimize the additional out parameter variant better. So there's several aspects that lean towards that variant of the API.

Again I'm not sure what variant you are referring to.

Using the C standard library's string function API as a baseline for API quality sets a very low bar for quality … ;-)

I personally value utility of the API over the "quality" which I'm not sure what it is. Convenience of passing the arguments and calling the function? Register and stack usage?

It's not I disagree with changing the API, but I didn't see an API suggestion to compare with, thus I cannot judge anything regarding what Copilot said.

ACK on making the error fatal.

Anyway, a good API makes it impossible to cause issues by abusing it. Thus if wcrtomb may write up to MB_LEN_MAX bytes you better should pass a buffer that can hold MB_LEN_MAX bytes, i.e. ensure the remaining space is large enough. Even if the actual call is known to write less.

And adding MB_LEN_MAX + 1 to the allocation won't cause any noticeable overhead (malloc will just allocate the same and in rare occasions or under memory pressure may delay due to the OOM killer sweeping some swap …

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

#!/bin/bash
# Verify callers that may loop on String_lineBreakWidth/String_mbswidth
# and require forward progress when malformed bytes are present.
rg -n -C3 '\bString_lineBreakWidth\s*\(|\bString_mbswidth\s*\('

# Get the size of XUtils.c first to determine reading strategy
wc -l XUtils.c

# Read the decoder function around lines 411-415
sed -n '385,420p' XUtils.c

# Read the String_lineBreakWidth function around lines 442-480
sed -n '442,480p' XUtils.c

# Read the full String_decodeNextWChar to understand the complete context
sed -n '370,440p' XUtils.c

sed -n '442,533p' XUtils.c

Consume bytes on invalid multibyte sequences to ensure forward progress.

Invalid UTF-8 at line 411 returns false without advancing ps->str, causing String_lineBreakWidth to return without updating *str (line 495 is unreached). Callers retrying on the same pointer risk infinite loops. The incomplete sequence case (lines 418-421) correctly advances pointers; invalid sequences must do the same.

   switch (len) {
   case (size_t)-1:
-      // Invalid sequence
-      ps->ch = WEOF;
-      return false;
+      // Invalid sequence: consume one byte to guarantee progress
+      ps->str += 1;
+      ps->maxLen -= 1;
+      ps->ch = WEOF;
+      return false;

   case (size_t)-2:

-      if (!String_decodeNextWChar(&state))
+      if (!String_decodeNextWChar(&state)) {
+         *str = state.str;
          break;
+      }

✅ Addressed in commits 1374d7c to 9d9301f

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Rename EncodePrintableString to module-prefixed style.

This new public function name breaks the project’s C naming convention and makes the API inconsistent with surrounding String_* utilities.

As per coding guidelines, use ModuleName_functionName() naming convention for C functions.