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.

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

Fix mbstowcs_nonfatal() that might convert string shorter than desired #1586

Closed
wants to merge 4 commits into from
Closed

Fix mbstowcs_nonfatal() that might convert string shorter than desired #1586

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e06fe48
RichString_setLen() minor logic fix
Explorer09 Jan 13, 2025
f5d9fe8
Add size limit check in RichString_extendLen()
Explorer09 Jan 13, 2025
64e9142
Prevent length wraparound in RichString_rewind()
Explorer09 Jan 13, 2025
ffcc552
Fix mbstowcs_nonfatal() that might convert string shorter than desired
Explorer09 Jan 13, 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
21 changes: 13 additions & 8 deletions RichString.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ in the source distribution for its full text.
#include <assert.h>
#include <ctype.h>
#include <limits.h> // IWYU pragma: keep
#include <stdint.h>
#include <stdlib.h>
#include <string.h>

Expand All @@ -22,11 +23,17 @@ in the source distribution for its full text.
#define charBytes(n) (sizeof(CharType) * (n))

static void RichString_extendLen(RichString* this, int len) {
// TODO: Remove the "len" type casts once all the length properties
// of RichString have been upgraded to size_t.
if ((size_t)len > (SIZE_MAX - 1) / sizeof(CharType)) {
Copy link
Member

Choose a reason for hiding this comment

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

Given INT_MAX <= SIZE_MAX, the only check needs to be len < 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.

@BenBE
On 32-bit systems INT_MAX can be SIZE_MAX / 2 (rounded down) and it could be larger than (SIZE_MAX - 1) / sizeof(CharType). That's what the check is for (although it seems useless in 64-bit systems).

Copy link
Member

Choose a reason for hiding this comment

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

Either way that's not right to check for overflow as is intended …

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should have changed the conditional to (len < 0 && (size_t)len > (SIZE_MAX - 1) / sizeof(CharType)) ...

fail();
}

if (this->chptr == this->chstr) {
// String is in internal buffer
if (len > RICHSTRING_MAXLEN) {
// Copy from internal buffer to allocated string
this->chptr = xMalloc(charBytes(len + 1));
this->chptr = xMalloc(charBytes((size_t)len + 1));
memcpy(this->chptr, this->chstr, charBytes(this->chlen));
} else {
// Still fits in internal buffer, do nothing
Expand All @@ -36,7 +43,7 @@ static void RichString_extendLen(RichString* this, int len) {
// String is managed externally
if (len > RICHSTRING_MAXLEN) {
// Just reallocate the buffer accordingly
this->chptr = xRealloc(this->chptr, charBytes(len + 1));
this->chptr = xRealloc(this->chptr, charBytes((size_t)len + 1));
} else {
// Move string into internal buffer and free resources
memcpy(this->chstr, this->chptr, charBytes(len));
Expand All @@ -50,7 +57,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 +66,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);
}

#ifdef HAVE_LIBNCURSESW
Expand All @@ -69,16 +76,15 @@ static size_t mbstowcs_nonfatal(wchar_t* restrict dest, const char* restrict src
mbstate_t ps = { 0 };
bool broken = false;

while (n > 0) {
size_t ret = mbrtowc(dest, src, n, &ps);
while (written < n) {
size_t ret = mbrtowc(dest, src, SIZE_MAX, &ps);
if (ret == (size_t)-1 || ret == (size_t)-2) {
if (!broken) {
broken = true;
*dest++ = L'\xFFFD';
written++;
}
src++;
n--;
continue;
}

Expand All @@ -91,7 +97,6 @@ static size_t mbstowcs_nonfatal(wchar_t* restrict dest, const char* restrict src
dest++;
written++;
src += ret;
n -= ret;
}

return written;
Expand Down
Loading