libc: fix some overflow scenarios in vis(3)

The previous incarnation of this would call wcrtomb() on the destination
buffer, and only check for overflow *after* it's happened.
Additionally, the conversion error / VIS_NOLOCALE path also didn't check
for overflow, and the overflow check at the end didn't account for the
fact that we still need to write a NUL terminator afterward.

Start by only doing the multibyte conversion into mbdst directly if we
have enough buffer space to guarantee it'll fit.  An additional
MB_CUR_MAX buffer has been stashed on the stack to write into if we're
cutting it close at the end of the buffer, since we don't really have a
good way to determine the length of the wchar_t without just doing the
conversion.  We'll do the conversion into the buffer that's guaranteed
to fit, then copy it over if the copy won't overflow.

The byte-for-byte overflow is a little bit easier, as we simply check
for overflow with each byte written and make sure we can still NUL
terminate after.

Tests added to exercise these edge cases.

Reviewed by:	des
Sponsored by:	Klara, Inc.
Differential Revision:	https://reviews.freebsd.org/D41328

contrib/libc-vis/vis.c

@@ -395,14 +395,16 @@ static int
 istrsenvisx(char **mbdstp, size_t *dlen, const char *mbsrc, size_t mblength,
     int flags, const char *mbextra, int *cerr_ptr)
 {
+	char mbbuf[MB_CUR_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 = 0, cerr, error = -1, i, shft;
-	char *mbdst, *mdst;
-	ssize_t mbslength, maxolen;
+	char *mbdst, *mbwrite, *mdst;
+	ssize_t mbslength;
+	size_t maxolen;
 	mbstate_t mbstate;
 
 	_DIAGASSERT(mbdstp != NULL);
@@ -541,8 +543,33 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *mbsrc, size_t mblength,
 	olen = 0;
 	bzero(&mbstate, sizeof(mbstate));
 	for (dst = start; len > 0; len--) {
-		if (!cerr)
-			clen = 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 = mbdst;
+			else
+				mbwrite = mbbuf;
+			clen = wcrtomb(mbwrite, *dst, &mbstate);
+			if (clen > 0 && mbwrite != 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 >= maxolen) {
+					errno = ENOSPC;
+					goto out;
+				}
+
+				memcpy(mbdst, mbwrite, clen);
+			}
+		}
 		if (cerr || clen < 0) {
 			/*
 			 * Conversion error, process as a byte(s) instead.
@@ -557,16 +584,27 @@ istrsenvisx(char **mbdstp, size_t *dlen, const char *mbsrc, size_t mblength,
 				shft = i * NBBY;
 				bmsk = (uint64_t)0xffLL << shft;
 				wmsk |= bmsk;
-				if ((*dst & wmsk) || i == 0)
+				if ((*dst & wmsk) || i == 0) {
+					if (olen + clen + 1 >= maxolen) {
+						errno = ENOSPC;
+						goto out;
+					}
+
 					mbdst[clen++] = (char)(
 					    (uint64_t)(*dst & bmsk) >>
 					    shft);
+				}
 			}
 			cerr = 1;
 		}
-		/* If this character would exceed our output limit, stop. */
-		if (olen + clen > (size_t)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 += clen;
 		/* Advance buffer character pointer. */

contrib/netbsd-tests/lib/libc/gen/t_vis.c

@@ -175,6 +175,68 @@ ATF_TC_BODY(strvis_locale, tc)
 }
 #endif /* VIS_NOLOCALE */
 
+#ifdef __FreeBSD__
+#define	STRVIS_OVERFLOW_MARKER	0xff	/* Arbitrary */
+
+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[] = "\xf0\x9f\xa5\x91";
+	/* Extra byte to detect overflow */
+	char dst[sizeof(src) + 1];
+	int n;
+
+	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 = strnvis(dst, 4, src, VIS_SAFE);
+	ATF_REQUIRE(dst[4] == STRVIS_OVERFLOW_MARKER);
+	ATF_REQUIRE(n == -1);
+
+	n = strnvis(dst, sizeof(src), src, VIS_SAFE);
+	ATF_REQUIRE(n == sizeof(src) - 1);
+}
+
+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[] = "AAAA";
+	/* Extra byte to detect overflow */
+	char dst[sizeof(src) + 1];
+	int n;
+
+	/* 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 = strnvis(dst, 4, src, VIS_SAFE | VIS_NOLOCALE);
+	ATF_REQUIRE(dst[4] == STRVIS_OVERFLOW_MARKER);
+	ATF_REQUIRE(n == -1);
+
+	n = strnvis(dst, sizeof(src), src, VIS_SAFE | VIS_NOLOCALE);
+	ATF_REQUIRE(n == sizeof(src) - 1);
+}
+#endif /* __FreeBSD__ */
+
 ATF_TP_ADD_TCS(tp)
 {
 
@@ -185,6 +247,10 @@ ATF_TP_ADD_TCS(tp)
 #ifdef VIS_NOLOCALE
 	ATF_TP_ADD_TC(tp, strvis_locale);
 #endif /* VIS_NOLOCALE */
+#ifdef __FreeBSD__
+	ATF_TP_ADD_TC(tp, strvis_overflow_mb);
+	ATF_TP_ADD_TC(tp, strvis_overflow_c);
+#endif
 
 	return atf_no_error();
 }