NetBSD-Bugs archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Re: lib/57573: Overflow possibilities in vis(3)
The following reply was made to PR lib/57573; it has been noted by GNATS.
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: kevans%FreeBSD.org@localhost
Cc: gnats-bugs%NetBSD.org@localhost
Subject: Re: lib/57573: Overflow possibilities in vis(3)
Date: Sat, 12 Aug 2023 15:01:36 +0000
This is a multi-part message in MIME format.
--=_La7nkjIJilZyzQ04m7iYdJZjSuvrF8DG
Turns out there's another buffer overrun lurking, when you do
strnvis(dst, 0, "", ...) -- it shouldn't write to dst[0], but it does.
I added an atf test for that, a branch to prevent it, and an assertion
to catch it.
I adapted your patch for the other overruns with some minor changes:
1. `char mbbuf[MB_LEN_MAX]' instead of `char mbbuf[MB_CUR_MAX]', since
MB_CUR_MAX is not a compile-time constant.
This avoids what is formally a variable-length array, which gets in
the way of stack protectors, &c. It may not pose a practical
problem for stack overflow, because MB_CUR_MAX <= MB_LEN_MAX, but
it's easier to keep measures that detect real problems elsewhere
happy this way.
2. Changed mbslength from ssize_t to size_t, since it can't go
negative, and added some assertions to support this.
This obviates the need to worry about what happens when mblength >
SIZE_MAX -- maybe impossible but I don't need to think about
proving that now!
3. Added another bounds check upstream of the callocs, in case
16*mbslength + 1 would overflow.
4. Eliminated a duplicate call wcslen(start).
5. Kept the #ifdef VIS_NOLOCALE in t_vis.c updated. Not sure why this
is here, but easier to just keep it updated than to investigate.
Attached is the patch series I committed to NetBSD for convenience if
you want to take a look. Tried to avoid unnecessary style differences
from yours to make merging easier if you want.
(I feel like this code shouldn't need to allocate multiple temporary
copies of everything -- should be able to work incrementally in a
stream from src to dst with only a constant-size intermediate buffer
like mbbuf, but I'm not feeling inclined to rewrite this now, so...)
Thanks for the report!
--=_La7nkjIJilZyzQ04m7iYdJZjSuvrF8DG
Content-Type: text/plain; charset="ISO-8859-1"; name="pr57573-vis-v2"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment; filename="pr57573-vis-v2.patch"
From e6466e0119be123f98a5385ac2101a0cba121ced Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sat, 12 Aug 2023 07:20:03 +0000
Subject: [PATCH 01/10] vis(3) tests: Add xfail test for encoding overflow.
From Kyle Evans <kevans%FreeBSD.org@localhost>.
PR lib/57573
XXX pullup-10
XXX pullup-9
XXX pullup-8
---
tests/lib/libc/gen/t_vis.c | 68 ++++++++++++++++++++++++++++++++++++++
1 file changed, 68 insertions(+)
diff --git a/tests/lib/libc/gen/t_vis.c b/tests/lib/libc/gen/t_vis.c
index adb0930a300a..18162565835c 100644
--- a/tests/lib/libc/gen/t_vis.c
+++ b/tests/lib/libc/gen/t_vis.c
@@ -175,6 +175,72 @@ ATF_TC_BODY(strvis_locale, tc)
}
#endif /* VIS_NOLOCALE */
=20
+#define STRVIS_OVERFLOW_MARKER 0xff /* Arbitrary */
+
+#ifdef VIS_NOLOCALE
+ATF_TC(strvis_overflow_mb);
+ATF_TC_HEAD(strvis_overflow_mb, tc)
+{
+ atf_tc_set_md_var(tc, "descr", "Test strvis(3) multi-byte overflow");
+}
+
+ATF_TC_BODY(strvis_overflow_mb, tc)
+{
+ const char src[] =3D "\xf0\x9f\xa5\x91";
+ /* Extra byte to detect overflow */
+ char dst[sizeof(src) + 1];
+ int n;
+
+ atf_tc_expect_fail("PR lib/57573: Overflow possibilities in vis(3)");
+
+ setlocale(LC_CTYPE, "en_US.UTF-8");
+
+ /* Arbitrary */
+ memset(dst, STRVIS_OVERFLOW_MARKER, sizeof(dst));
+
+ /*
+ * If we only provide four bytes of buffer, we shouldn't be able encode
+ * a full 4-byte sequence.
+ */
+ n =3D strnvis(dst, 4, src, VIS_SAFE);
+ ATF_REQUIRE(dst[4] =3D=3D STRVIS_OVERFLOW_MARKER);
+ ATF_REQUIRE(n =3D=3D -1);
+
+ n =3D strnvis(dst, sizeof(src), src, VIS_SAFE);
+ ATF_REQUIRE(n =3D=3D sizeof(src) - 1);
+}
+#endif
+
+ATF_TC(strvis_overflow_c);
+ATF_TC_HEAD(strvis_overflow_c, tc)
+{
+ atf_tc_set_md_var(tc, "descr", "Test strvis(3) C locale overflow");
+}
+
+ATF_TC_BODY(strvis_overflow_c, tc)
+{
+ const char src[] =3D "AAAA";
+ /* Extra byte to detect overflow */
+ char dst[sizeof(src) + 1];
+ int n;
+
+ atf_tc_expect_fail("PR lib/57573: Overflow possibilities in vis(3)");
+
+ /* Arbitrary */
+ memset(dst, STRVIS_OVERFLOW_MARKER, sizeof(dst));
+
+ /*
+ * If we only provide four bytes of buffer, we shouldn't be able encode
+ * 4 bytes of input.
+ */
+ n =3D strnvis(dst, 4, src, VIS_SAFE | VIS_NOLOCALE);
+ ATF_REQUIRE(dst[4] =3D=3D STRVIS_OVERFLOW_MARKER);
+ ATF_REQUIRE(n =3D=3D -1);
+
+ n =3D strnvis(dst, sizeof(src), src, VIS_SAFE | VIS_NOLOCALE);
+ ATF_REQUIRE(n =3D=3D sizeof(src) - 1);
+}
+
ATF_TP_ADD_TCS(tp)
{
=20
@@ -184,7 +250,9 @@ ATF_TP_ADD_TCS(tp)
ATF_TP_ADD_TC(tp, strunvis_hex);
#ifdef VIS_NOLOCALE
ATF_TP_ADD_TC(tp, strvis_locale);
+ ATF_TP_ADD_TC(tp, strvis_overflow_mb);
#endif /* VIS_NOLOCALE */
+ ATF_TP_ADD_TC(tp, strvis_overflow_c);
=20
return atf_no_error();
}
From 8e44622ed3027a113d5c39b7abb436a0d4a7f0bc Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sat, 12 Aug 2023 07:24:15 +0000
Subject: [PATCH 02/10] vis(3) tests: Expand tests and diagnostic outputs on
failure.
PR lib/57573
XXX pullup-10
XXX pullup-9
XXX pullup-8
---
tests/lib/libc/gen/t_vis.c | 64 +++++++++++++++++++++++---------------
1 file changed, 39 insertions(+), 25 deletions(-)
diff --git a/tests/lib/libc/gen/t_vis.c b/tests/lib/libc/gen/t_vis.c
index 18162565835c..cf29123dc992 100644
--- a/tests/lib/libc/gen/t_vis.c
+++ b/tests/lib/libc/gen/t_vis.c
@@ -175,7 +175,7 @@ ATF_TC_BODY(strvis_locale, tc)
}
#endif /* VIS_NOLOCALE */
=20
-#define STRVIS_OVERFLOW_MARKER 0xff /* Arbitrary */
+#define STRVIS_OVERFLOW_MARKER ((char)0xff) /* Arbitrary */
=20
#ifdef VIS_NOLOCALE
ATF_TC(strvis_overflow_mb);
@@ -189,25 +189,32 @@ ATF_TC_BODY(strvis_overflow_mb, tc)
const char src[] =3D "\xf0\x9f\xa5\x91";
/* Extra byte to detect overflow */
char dst[sizeof(src) + 1];
+ unsigned i;
int n;
=20
atf_tc_expect_fail("PR lib/57573: Overflow possibilities in vis(3)");
=20
setlocale(LC_CTYPE, "en_US.UTF-8");
=20
- /* Arbitrary */
- memset(dst, STRVIS_OVERFLOW_MARKER, sizeof(dst));
-
- /*
- * If we only provide four bytes of buffer, we shouldn't be able encode
- * a full 4-byte sequence.
- */
- n =3D strnvis(dst, 4, src, VIS_SAFE);
- ATF_REQUIRE(dst[4] =3D=3D STRVIS_OVERFLOW_MARKER);
- ATF_REQUIRE(n =3D=3D -1);
+ for (i =3D 0; i < sizeof(dst) - 1; i++) {
+ memset(dst, STRVIS_OVERFLOW_MARKER, sizeof(dst));
+ n =3D strnvis(dst, i, src, VIS_SAFE);
+ ATF_CHECK_EQ_MSG(dst[i], STRVIS_OVERFLOW_MARKER,
+ "[%u] dst=3D[%02hhx %02hhx %02hhx %02hhx %02hhx]"
+ " STRVIS_OVERFLOW_MARKER=3D%02hhx",
+ i, dst[0], dst[1], dst[2], dst[3], dst[4],
+ STRVIS_OVERFLOW_MARKER);
+ ATF_CHECK_EQ_MSG(n, -1, "[%u] n=3D%d", i, n);
+ }
=20
- n =3D strnvis(dst, sizeof(src), src, VIS_SAFE);
- ATF_REQUIRE(n =3D=3D sizeof(src) - 1);
+ memset(dst, STRVIS_OVERFLOW_MARKER, sizeof(dst));
+ n =3D strnvis(dst, sizeof(dst) - 1, src, VIS_SAFE);
+ ATF_CHECK_EQ_MSG(dst[sizeof(dst) - 1], STRVIS_OVERFLOW_MARKER,
+ "[%u] dst=3D[%02hhx %02hhx %02hhx %02hhx %02hhx %02hhx]"
+ " STRVIS_OVERFLOW_MARKER=3D%02hhx",
+ i, dst[0], dst[1], dst[2], dst[3], dst[4], dst[5],
+ STRVIS_OVERFLOW_MARKER);
+ ATF_CHECK_EQ_MSG(n, (int)sizeof(dst) - 2, "n=3D%d", n);
}
#endif
=20
@@ -222,23 +229,30 @@ ATF_TC_BODY(strvis_overflow_c, tc)
const char src[] =3D "AAAA";
/* Extra byte to detect overflow */
char dst[sizeof(src) + 1];
+ unsigned i;
int n;
=20
atf_tc_expect_fail("PR lib/57573: Overflow possibilities in vis(3)");
=20
- /* Arbitrary */
- memset(dst, STRVIS_OVERFLOW_MARKER, sizeof(dst));
-
- /*
- * If we only provide four bytes of buffer, we shouldn't be able encode
- * 4 bytes of input.
- */
- n =3D strnvis(dst, 4, src, VIS_SAFE | VIS_NOLOCALE);
- ATF_REQUIRE(dst[4] =3D=3D STRVIS_OVERFLOW_MARKER);
- ATF_REQUIRE(n =3D=3D -1);
+ for (i =3D 0; i < sizeof(dst) - 1; i++) {
+ memset(dst, STRVIS_OVERFLOW_MARKER, sizeof(dst));
+ n =3D strnvis(dst, i, src, VIS_SAFE | VIS_NOLOCALE);
+ ATF_CHECK_EQ_MSG(dst[i], STRVIS_OVERFLOW_MARKER,
+ "[%u] dst=3D[%02hhx %02hhx %02hhx %02hhx %02hhx]"
+ " STRVIS_OVERFLOW_MARKER=3D%02hhx",
+ i, dst[0], dst[1], dst[2], dst[3], dst[4],
+ STRVIS_OVERFLOW_MARKER);
+ ATF_CHECK_EQ_MSG(n, -1, "[%u] n=3D%d", i, n);
+ }
=20
- n =3D strnvis(dst, sizeof(src), src, VIS_SAFE | VIS_NOLOCALE);
- ATF_REQUIRE(n =3D=3D sizeof(src) - 1);
+ memset(dst, STRVIS_OVERFLOW_MARKER, sizeof(dst));
+ n =3D strnvis(dst, sizeof(dst) - 1, src, VIS_SAFE | VIS_NOLOCALE);
+ ATF_CHECK_EQ_MSG(dst[sizeof(dst) - 1], STRVIS_OVERFLOW_MARKER,
+ "[%u] dst=3D[%02hhx %02hhx %02hhx %02hhx %02hhx %02hhx]"
+ " STRVIS_OVERFLOW_MARKER=3D%02hhx",
+ i, dst[0], dst[1], dst[2], dst[3], dst[4], dst[5],
+ STRVIS_OVERFLOW_MARKER);
+ ATF_CHECK_EQ_MSG(n, (int)sizeof(dst) - 2, "n=3D%d", n);
}
=20
ATF_TP_ADD_TCS(tp)
From 2aff286fef53c7166eb876e5cc4798cd7ad917f7 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sat, 12 Aug 2023 12:26:33 +0000
Subject: [PATCH 03/10] vis(3) tests: Test another overflow edge case.
Related to PR lib/57573.
XXX pullup-10
XXX pullup-9
XXX pullup-8
---
tests/lib/libc/gen/t_vis.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/tests/lib/libc/gen/t_vis.c b/tests/lib/libc/gen/t_vis.c
index cf29123dc992..7ae0a6bc3885 100644
--- a/tests/lib/libc/gen/t_vis.c
+++ b/tests/lib/libc/gen/t_vis.c
@@ -116,6 +116,25 @@ ATF_TC_BODY(strvis_empty, tc)
ATF_REQUIRE(dst[0] =3D=3D '\0' && dst[1] =3D=3D 'a');
}
=20
+ATF_TC(strnvis_empty_empty);
+ATF_TC_HEAD(strnvis_empty_empty, tc)
+{
+ atf_tc_set_md_var(tc, "descr",
+ "Test strnvis(3) with empty source and destination");
+}
+
+ATF_TC_BODY(strnvis_empty_empty, tc)
+{
+ char dst[] =3D "fail";
+ int n;
+
+ atf_tc_expect_fail("PR lib/57573: Overflow possibilities in vis(3)");
+
+ n =3D strnvis(dst, 0, "", VIS_SAFE);
+ ATF_CHECK(memcmp(dst, "fail", sizeof(dst)) =3D=3D 0);
+ ATF_CHECK_EQ_MSG(n, -1, "n=3D%d", n);
+}
+
ATF_TC(strunvis_hex);
ATF_TC_HEAD(strunvis_hex, tc)
{
@@ -261,6 +280,7 @@ ATF_TP_ADD_TCS(tp)
ATF_TP_ADD_TC(tp, strvis_basic);
ATF_TP_ADD_TC(tp, strvis_null);
ATF_TP_ADD_TC(tp, strvis_empty);
+ ATF_TP_ADD_TC(tp, strnvis_empty_empty);
ATF_TP_ADD_TC(tp, strunvis_hex);
#ifdef VIS_NOLOCALE
ATF_TP_ADD_TC(tp, strvis_locale);
From 37b6975cd9b7df8286c00185a02a5c527ab29f9e Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sat, 12 Aug 2023 08:12:14 +0000
Subject: [PATCH 04/10] vis(3): Make maxolen unsigned size_t, not ssize_t.
It is initialized once either to *dlen, which is unsigned size_t, or
to wcslen(start) * MB_MAX_LEN + 1, and wcslen returns unsigned size_t
too. So there appears to have never been any reason for this to be
signed.
Part of PR lib/57573.
XXX pullup-10
XXX pullup-9
XXX pullup-8
---
lib/libc/gen/vis.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/lib/libc/gen/vis.c b/lib/libc/gen/vis.c
index ecc304c8015c..5f7a494287dc 100644
--- a/lib/libc/gen/vis.c
+++ b/lib/libc/gen/vis.c
@@ -403,7 +403,8 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *mb=
src, size_t mblength,
visfun_t f;
int clen =3D 0, cerr, error =3D -1, i, shft;
char *mbdst, *mdst;
- ssize_t mbslength, maxolen;
+ ssize_t mbslength;
+ size_t maxolen;
mbstate_t mbstate;
=20
_DIAGASSERT(mbdstp !=3D NULL);
@@ -569,7 +570,7 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *mb=
src, size_t mblength,
cerr =3D 1;
}
/* If this character would exceed our output limit, stop. */
- if (olen + clen > (size_t)maxolen)
+ if (olen + clen > maxolen)
break;
/* Advance output pointer by number of bytes written. */
mbdst +=3D clen;
From 238d2917c36ff5ba5b7c3685481d321db08d1827 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sat, 12 Aug 2023 08:43:21 +0000
Subject: [PATCH 05/10] vis(3): Make mbslength unsigned.
Sprinkle assertions and comments justifying the proposition that it
would never go negative if signed.
Obviates need to worry about mblength > SSIZE_MAX.
Prompted by PR lib/57573.
XXX pullup-10
XXX pullup-9
XXX pullup-8
---
lib/libc/gen/vis.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/lib/libc/gen/vis.c b/lib/libc/gen/vis.c
index 5f7a494287dc..5dc7c9e85907 100644
--- a/lib/libc/gen/vis.c
+++ b/lib/libc/gen/vis.c
@@ -403,7 +403,7 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *mb=
src, size_t mblength,
visfun_t f;
int clen =3D 0, cerr, error =3D -1, i, shft;
char *mbdst, *mdst;
- ssize_t mbslength;
+ size_t mbslength;
size_t maxolen;
mbstate_t mbstate;
=20
@@ -411,7 +411,7 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *mb=
src, size_t mblength,
_DIAGASSERT(mbsrc !=3D NULL || mblength =3D=3D 0);
_DIAGASSERT(mbextra !=3D NULL);
=20
- mbslength =3D (ssize_t)mblength;
+ mbslength =3D mblength;
/*
* When inputing a single character, must also read in the
* next character for nextc, the look-ahead character.
@@ -466,12 +466,15 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *=
mbsrc, size_t mblength,
memset(&mbstate, 0, sizeof(mbstate));
while (mbslength > 0) {
/* Convert one multibyte character to wchar_t. */
- if (!cerr)
+ if (!cerr) {
clen =3D mbrtowc(src, mbsrc,
(mbslength < MB_LEN_MAX
? mbslength
: MB_LEN_MAX),
&mbstate);
+ assert(clen < 0 || (size_t)clen <=3D mbslength);
+ assert(clen <=3D MB_LEN_MAX);
+ }
if (cerr || clen < 0) {
/* Conversion error, process as a byte instead. */
*src =3D (wint_t)(u_char)*mbsrc;
@@ -485,6 +488,20 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *m=
bsrc, size_t mblength,
*/
clen =3D 1;
}
+ /*
+ * Let n :=3D MIN(mbslength, MB_LEN_MAX). We have:
+ *
+ * mbslength >=3D 1
+ * mbrtowc(..., n, &mbstate) <=3D n,
+ * by the contract of mbrtowc
+ *
+ * clen is either
+ * (a) mbrtowc(..., n, &mbstate), in which case
+ * clen <=3D n <=3D mbslength; or
+ * (b) 1, in which case clen =3D 1 <=3D mbslength.
+ */
+ assert(clen > 0);
+ assert((size_t)clen <=3D mbslength);
/* Advance buffer character pointer. */
src++;
/* Advance input pointer by number of bytes read. */
From 598a93cb3ab010086e82ff5a6951c5f129edad06 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sat, 12 Aug 2023 08:46:46 +0000
Subject: [PATCH 06/10] vis(3): Avoid arithmetic overflow before calloc(3).
Prompted by PR lib/57573.
XXX pullup-10
XXX pullup-9
XXX pullup-8
---
lib/libc/gen/vis.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/lib/libc/gen/vis.c b/lib/libc/gen/vis.c
index 5dc7c9e85907..5c9222f6acd0 100644
--- a/lib/libc/gen/vis.c
+++ b/lib/libc/gen/vis.c
@@ -432,6 +432,14 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *m=
bsrc, size_t mblength,
* return to the caller.
*/
=20
+ /*
+ * Guarantee the arithmetic on input to calloc won't overflow.
+ */
+ if (mbslength > (SIZE_MAX - 1)/16) {
+ errno =3D ENOMEM;
+ return -1;
+ }
+
/* Allocate space for the wide char strings */
psrc =3D pdst =3D extra =3D NULL;
mdst =3D NULL;
From 229f19a501dc75592f596c0260664de5493e9a47 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sat, 12 Aug 2023 09:37:58 +0000
Subject: [PATCH 07/10] vis(3): Call wcslen(start) only once.
It had better not change between these two times!
Prompted by PR lib/57573.
XXX pullup-10
XXX pullup-9
XXX pullup-8
---
lib/libc/gen/vis.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/libc/gen/vis.c b/lib/libc/gen/vis.c
index 5c9222f6acd0..d061749b1d50 100644
--- a/lib/libc/gen/vis.c
+++ b/lib/libc/gen/vis.c
@@ -567,7 +567,7 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *mb=
src, size_t mblength,
* output byte-by-byte here. Else use wctomb().
*/
len =3D wcslen(start);
- maxolen =3D dlen ? *dlen : (wcslen(start) * MB_LEN_MAX + 1);
+ maxolen =3D dlen ? *dlen : (len * MB_LEN_MAX + 1);
olen =3D 0;
memset(&mbstate, 0, sizeof(mbstate));
for (dst =3D start; len > 0; len--) {
From f442e562a74a11ebcfe166f46c8cd42146555a62 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sat, 12 Aug 2023 09:40:00 +0000
Subject: [PATCH 08/10] vis(3): Avoid potential arithmetic overflow in maxol=
en.
Can't easily prove that this overflow is impossible, so let's add a
check.
Prompted by PR lib/57573.
XXX pullup-10
XXX pullup-9
XXX pullup-8
---
lib/libc/gen/vis.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/lib/libc/gen/vis.c b/lib/libc/gen/vis.c
index d061749b1d50..3e539fe22cd0 100644
--- a/lib/libc/gen/vis.c
+++ b/lib/libc/gen/vis.c
@@ -567,7 +567,15 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *m=
bsrc, size_t mblength,
* output byte-by-byte here. Else use wctomb().
*/
len =3D wcslen(start);
- maxolen =3D dlen ? *dlen : (len * MB_LEN_MAX + 1);
+ if (dlen) {
+ maxolen =3D *dlen;
+ } else {
+ if (len > (SIZE_MAX - 1)/MB_LEN_MAX) {
+ errno =3D ENOSPC;
+ goto out;
+ }
+ maxolen =3D len*MB_LEN_MAX + 1;
+ }
olen =3D 0;
memset(&mbstate, 0, sizeof(mbstate));
for (dst =3D start; len > 0; len--) {
From edea4ce6d025be7f375b9d7b6edb287d1edd250d Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sat, 12 Aug 2023 12:24:11 +0000
Subject: [PATCH 09/10] vis(3): Fix main part of PR lib/57573.
From Kyle Evans <kevans%FreeBSD.org@localhost>.
XXX pullup-10
XXX pullup-9
XXX pullup-8
---
lib/libc/gen/vis.c | 51 ++++++++++++++++++++++++++++++++------
tests/lib/libc/gen/t_vis.c | 4 ---
2 files changed, 44 insertions(+), 11 deletions(-)
diff --git a/lib/libc/gen/vis.c b/lib/libc/gen/vis.c
index 3e539fe22cd0..d291c80bcda4 100644
--- a/lib/libc/gen/vis.c
+++ b/lib/libc/gen/vis.c
@@ -396,13 +396,14 @@ static int
istrsenvisx(char **mbdstp, size_t *dlen, const char *mbsrc, size_t mblengt=
h,
int flags, const char *mbextra, int *cerr_ptr)
{
+ char mbbuf[MB_LEN_MAX];
wchar_t *dst, *src, *pdst, *psrc, *start, *extra;
size_t len, olen;
uint64_t bmsk, wmsk;
wint_t c;
visfun_t f;
int clen =3D 0, cerr, error =3D -1, i, shft;
- char *mbdst, *mdst;
+ char *mbdst, *mbwrite, *mdst;
size_t mbslength;
size_t maxolen;
mbstate_t mbstate;
@@ -579,8 +580,33 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *m=
bsrc, size_t mblength,
olen =3D 0;
memset(&mbstate, 0, sizeof(mbstate));
for (dst =3D start; len > 0; len--) {
- if (!cerr)
- clen =3D wcrtomb(mbdst, *dst, &mbstate);
+ if (!cerr) {
+ /*
+ * If we have at least MB_CUR_MAX bytes in the buffer,
+ * we'll just do the conversion in-place into mbdst. We
+ * need to be a little more conservative when we get to
+ * the end of the buffer, as we may not have MB_CUR_MAX
+ * bytes but we may not need it.
+ */
+ if (maxolen - olen > MB_CUR_MAX)
+ mbwrite =3D mbdst;
+ else
+ mbwrite =3D mbbuf;
+ clen =3D wcrtomb(mbwrite, *dst, &mbstate);
+ if (clen > 0 && mbwrite !=3D mbdst) {
+ /*
+ * Don't break past our output limit, noting
+ * that maxolen includes the nul terminator so
+ * we can't write past maxolen - 1 here.
+ */
+ if (olen + clen >=3D maxolen) {
+ errno =3D ENOSPC;
+ goto out;
+ }
+
+ memcpy(mbdst, mbwrite, clen);
+ }
+ }
if (cerr || clen < 0) {
/*
* Conversion error, process as a byte(s) instead.
@@ -595,16 +621,27 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *=
mbsrc, size_t mblength,
shft =3D i * NBBY;
bmsk =3D (uint64_t)0xffLL << shft;
wmsk |=3D bmsk;
- if ((*dst & wmsk) || i =3D=3D 0)
+ if ((*dst & wmsk) || i =3D=3D 0) {
+ if (olen + clen + 1 >=3D maxolen) {
+ errno =3D ENOSPC;
+ goto out;
+ }
+
mbdst[clen++] =3D (char)(
(uint64_t)(*dst & bmsk) >>
shft);
+ }
}
cerr =3D 1;
}
- /* If this character would exceed our output limit, stop. */
- if (olen + clen > maxolen)
- break;
+
+ /*
+ * We'll be dereferencing mbdst[clen] after this to write the
+ * nul terminator; the above paths should have checked for a
+ * possible overflow already.
+ */
+ assert(olen + clen < maxolen);
+
/* Advance output pointer by number of bytes written. */
mbdst +=3D clen;
/* Advance buffer character pointer. */
diff --git a/tests/lib/libc/gen/t_vis.c b/tests/lib/libc/gen/t_vis.c
index 7ae0a6bc3885..51b798b19f27 100644
--- a/tests/lib/libc/gen/t_vis.c
+++ b/tests/lib/libc/gen/t_vis.c
@@ -211,8 +211,6 @@ ATF_TC_BODY(strvis_overflow_mb, tc)
unsigned i;
int n;
=20
- atf_tc_expect_fail("PR lib/57573: Overflow possibilities in vis(3)");
-
setlocale(LC_CTYPE, "en_US.UTF-8");
=20
for (i =3D 0; i < sizeof(dst) - 1; i++) {
@@ -251,8 +249,6 @@ ATF_TC_BODY(strvis_overflow_c, tc)
unsigned i;
int n;
=20
- atf_tc_expect_fail("PR lib/57573: Overflow possibilities in vis(3)");
-
for (i =3D 0; i < sizeof(dst) - 1; i++) {
memset(dst, STRVIS_OVERFLOW_MARKER, sizeof(dst));
n =3D strnvis(dst, i, src, VIS_SAFE | VIS_NOLOCALE);
From 724c7fa1eee212dcf3feec7debb87208aa799702 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sat, 12 Aug 2023 12:28:37 +0000
Subject: [PATCH 10/10] vis(3): Fix one more buffer overrun in an edge case.
PR lib/57573
XXX pullup-10
XXX pullup-9
XXX pullup-8
---
lib/libc/gen/vis.c | 5 +++++
tests/lib/libc/gen/t_vis.c | 2 --
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/lib/libc/gen/vis.c b/lib/libc/gen/vis.c
index d291c80bcda4..a259d806d527 100644
--- a/lib/libc/gen/vis.c
+++ b/lib/libc/gen/vis.c
@@ -570,6 +570,10 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *m=
bsrc, size_t mblength,
len =3D wcslen(start);
if (dlen) {
maxolen =3D *dlen;
+ if (maxolen =3D=3D 0) {
+ errno =3D ENOSPC;
+ goto out;
+ }
} else {
if (len > (SIZE_MAX - 1)/MB_LEN_MAX) {
errno =3D ENOSPC;
@@ -651,6 +655,7 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *mb=
src, size_t mblength,
}
=20
/* Terminate the output string. */
+ assert(olen < maxolen);
*mbdst =3D '\0';
=20
if (flags & VIS_NOLOCALE) {
diff --git a/tests/lib/libc/gen/t_vis.c b/tests/lib/libc/gen/t_vis.c
index 51b798b19f27..4c085a59baf7 100644
--- a/tests/lib/libc/gen/t_vis.c
+++ b/tests/lib/libc/gen/t_vis.c
@@ -128,8 +128,6 @@ ATF_TC_BODY(strnvis_empty_empty, tc)
char dst[] =3D "fail";
int n;
=20
- atf_tc_expect_fail("PR lib/57573: Overflow possibilities in vis(3)");
-
n =3D strnvis(dst, 0, "", VIS_SAFE);
ATF_CHECK(memcmp(dst, "fail", sizeof(dst)) =3D=3D 0);
ATF_CHECK_EQ_MSG(n, -1, "n=3D%d", n);
--=_La7nkjIJilZyzQ04m7iYdJZjSuvrF8DG--
Home |
Main Index |
Thread Index |
Old Index