Improve the ByteBuffer interface in the next ABI version (#625)

* Improve the detailed description of ByteBuffer

* Document which methods are planned to be removed

* Include testMbstowcsInfiniteLoop in test when relevant

* Improve APRCharsetDecoder robustness

* Consistently use 'Bad character' in decode exception messages

Author: swebb2066

src/main/cpp/aprsocket.cpp

@@ -131,7 +131,7 @@ size_t APRSocket::write(ByteBuffer& buf)
 		apr_status_t status = apr_socket_send(_priv->socket, buf.current(), &written);
 #endif
 
-		buf.position(buf.position() + written);
+		buf.increment_position(written);
 		totalWritten += written;
 
 		if (status != APR_SUCCESS)

src/main/cpp/bytearrayinputstream.cpp

@@ -66,7 +66,7 @@ int ByteArrayInputStream::read(ByteBuffer& dst)
 		size_t bytesCopied = min(dst.remaining(), m_priv->buf.size() - m_priv->pos);
 		std::memcpy(dst.current(), &m_priv->buf[m_priv->pos], bytesCopied);
 		m_priv->pos += bytesCopied;
-		dst.position(dst.position() + bytesCopied);
+		dst.increment_position(bytesCopied);
 		return (int)bytesCopied;
 	}
 }

src/main/cpp/bytearrayoutputstream.cpp

@@ -59,7 +59,7 @@ void ByteArrayOutputStream::write(ByteBuffer& buf, Pool& /* p */ )
 
 	m_priv->array.resize(sz + count);
 	memcpy(&m_priv->array[sz], buf.current(), count);
-	buf.position(buf.limit());
+	buf.increment_position(count);
 }
 
 std::vector<unsigned char> ByteArrayOutputStream::toByteArray() const

src/main/cpp/bytebuffer.cpp

@@ -18,6 +18,7 @@
 #include <log4cxx/helpers/bytebuffer.h>
 #include <log4cxx/helpers/exception.h>
 #include <log4cxx/helpers/pool.h>
+#include <cstring>
 
 using namespace LOG4CXX_NS;
 using namespace LOG4CXX_NS::helpers;
@@ -48,12 +49,21 @@ void ByteBuffer::clear()
 	m_priv->pos = 0;
 }
 
+void ByteBuffer::carry()
+{
+	auto available = remaining();
+	memmove(m_priv->base, current(), available);
+	m_priv->lim = m_priv->cap;
+	m_priv->pos = available;
+}
+
 void ByteBuffer::flip()
 {
 	m_priv->lim = m_priv->pos;
 	m_priv->pos = 0;
 }
 
+#if LOG4CXX_ABI_VERSION <= 15
 void ByteBuffer::position(size_t newPosition)
 {
 	if (newPosition < m_priv->lim)
@@ -80,7 +90,7 @@ void ByteBuffer::limit(size_t newLimit)
 		m_priv->pos = m_priv->lim;
 	}
 }
-
+#endif
 
 bool ByteBuffer::put(char byte)
 {
@@ -128,3 +138,9 @@ size_t ByteBuffer::remaining() const
 	return m_priv->lim - m_priv->pos;
 }
 
+size_t ByteBuffer::increment_position(size_t byteCount)
+{
+    auto available = remaining();
+    m_priv->pos += byteCount < available ? byteCount : available;
+    return remaining();
+}

src/main/cpp/charsetdecoder.cpp

@@ -111,18 +111,22 @@ class APRCharsetDecoder : public CharsetDecoder
 				{
 					size_t inbytes_left = in.remaining();
 					size_t initial_inbytes_left = inbytes_left;
-					size_t pos = in.position();
 					apr_size_t outbytes_left = initial_outbytes_left;
 					{
 						std::lock_guard<std::mutex> lock(mutex);
 						stat = apr_xlate_conv_buffer((apr_xlate_t*) convset,
-								in.data() + pos,
+								in.current(),
 								&inbytes_left,
 								(char*) buf,
 								&outbytes_left);
 					}
 					out.append(buf, (initial_outbytes_left - outbytes_left) / sizeof(logchar));
-					in.position(pos + (initial_inbytes_left - inbytes_left));
+					if (inbytes_left == initial_inbytes_left && stat == APR_SUCCESS)
+					{
+						stat = APR_BADCH;
+						break;
+					}
+					in.increment_position(initial_inbytes_left - inbytes_left);
 				}
 			}
 
@@ -181,7 +185,7 @@ class MbstowcsCharsetDecoder : public CharsetDecoder
 				if (*src == 0)
 				{
 					out.append(1, (logchar) 0);
-					in.position(in.position() + 1);
+					in.increment_position(1);
 				}
 				else
 				{
@@ -194,7 +198,7 @@ class MbstowcsCharsetDecoder : public CharsetDecoder
 							BUFSIZE - 1,
 							&mbstate);
 					auto converted = src - cbuf;
-					in.position(in.position() + converted);
+					in.increment_position(converted);
 
 					if (wCharCount == (size_t) -1) // Illegal byte sequence?
 					{
@@ -260,10 +264,10 @@ class TrivialCharsetDecoder : public CharsetDecoder
 
 			if ( remaining > 0)
 			{
-				const logchar* src = (const logchar*) (in.data() + in.position());
-				size_t count = remaining / sizeof(logchar);
-				out.append(src, count);
-				in.position(in.position() + remaining);
+				auto src = in.current();
+				auto count = remaining / sizeof(logchar);
+				out.append(reinterpret_cast<const logchar*>(src), count);
+				in.increment_position(remaining);
 			}
 
 			return APR_SUCCESS;
@@ -299,30 +303,29 @@ class UTF8CharsetDecoder : public CharsetDecoder
 		virtual log4cxx_status_t decode(ByteBuffer& in,
 			LogString& out)
 		{
-			if (in.remaining() > 0)
+			auto availableByteCount = in.remaining();
+			std::string tmp(in.current(), availableByteCount);
+			std::string::const_iterator nextCodePoint = tmp.begin();
+
+			while (nextCodePoint != tmp.end())
 			{
-				std::string tmp(in.current(), in.remaining());
-				std::string::const_iterator iter = tmp.begin();
+				auto lastCodePoint = nextCodePoint;
+				auto sv = Transcoder::decode(tmp, nextCodePoint);
 
-				while (iter != tmp.end())
+				if (sv == 0xFFFF || nextCodePoint == lastCodePoint)
 				{
-					unsigned int sv = Transcoder::decode(tmp, iter);
-
-					if (sv == 0xFFFF)
-					{
-						size_t offset = iter - tmp.begin();
-						in.position(in.position() + offset);
-						return APR_BADARG;
-					}
-					else
-					{
-						Transcoder::encode(sv, out);
-					}
+					size_t offset = nextCodePoint - tmp.begin();
+					in.increment_position(offset);
+					return APR_BADCH;
+				}
+				else
+				{
+					Transcoder::encode(sv, out);
 				}
-
-				in.position(in.limit());
 			}
 
+			in.increment_position(availableByteCount);
+
 			return APR_SUCCESS;
 		}
 
@@ -351,20 +354,16 @@ class ISOLatinCharsetDecoder : public CharsetDecoder
 		virtual log4cxx_status_t decode(ByteBuffer& in,
 			LogString& out)
 		{
-			if (in.remaining() > 0)
-			{
-
-				const unsigned char* src = (unsigned char*) in.current();
-				const unsigned char* srcEnd = src + in.remaining();
+			auto availableByteCount = in.remaining();
+			auto src = in.current();
+			auto srcEnd = src + availableByteCount;
 
-				while (src < srcEnd)
-				{
-					unsigned int sv = *(src++);
-					Transcoder::encode(sv, out);
-				}
-
-				in.position(in.limit());
+			while (src < srcEnd)
+			{
+				auto sv = static_cast<unsigned int>(*src++);
+				Transcoder::encode(sv, out);
 			}
+			in.increment_position(availableByteCount);
 
 			return APR_SUCCESS;
 		}
@@ -399,30 +398,26 @@ class USASCIICharsetDecoder : public CharsetDecoder
 		{
 			log4cxx_status_t stat = APR_SUCCESS;
 
-			if (in.remaining() > 0)
+			auto availableByteCount = in.remaining();
+			auto src = in.current();
+			auto srcEnd = src + availableByteCount;
+			size_t byteCount = 0;
+			while (src < srcEnd)
 			{
+				auto sv = static_cast<unsigned int>(*src++);
 
-				const unsigned char* src = (unsigned char*) in.current();
-				const unsigned char* srcEnd = src + in.remaining();
-
-				while (src < srcEnd)
+				if (sv < 0x80)
 				{
-					unsigned char sv = *src;
-
-					if (sv < 0x80)
-					{
-						src++;
-						Transcoder::encode(sv, out);
-					}
-					else
-					{
-						stat = APR_BADARG;
-						break;
-					}
+					++byteCount;
+					Transcoder::encode(sv, out);
+				}
+				else
+				{
+					stat = APR_BADCH;
+					break;
 				}
-
-				in.position(src - (const unsigned char*) in.data());
 			}
+			in.increment_position(byteCount);
 
 			return stat;
 		}
@@ -446,44 +441,43 @@ class LocaleCharsetDecoder : public CharsetDecoder
 		log4cxx_status_t decode(ByteBuffer& in, LogString& out) override
 		{
 			log4cxx_status_t result = APR_SUCCESS;
-			const char* p = in.current();
-			size_t i = in.position();
-			size_t remain = in.limit() - i;
+			auto p = in.current();
+			auto availableByteCount = in.remaining();
+			size_t byteCount = 0;
 #if !LOG4CXX_CHARSET_EBCDIC
 			if (std::mbsinit(&this->state)) // ByteBuffer not partially decoded?
 			{
 				// Copy single byte characters
-				for (; 0 < remain && ((unsigned int) *p) < 0x80; --remain, ++i, p++)
+				for (; byteCount < availableByteCount && static_cast<unsigned int>(*p) < 0x80; ++byteCount, ++p)
 				{
 					out.append(1, *p);
 				}
 			}
 #endif
 			// Decode characters that may be represented by multiple bytes
-			while (0 < remain)
+			while (byteCount < availableByteCount)
 			{
 				wchar_t ch = 0;
-				size_t n = std::mbrtowc(&ch, p, remain, &this->state);
+				size_t n = std::mbrtowc(&ch, p, availableByteCount - byteCount, &this->state);
 				if (0 == n) // NULL encountered?
 				{
-					++i;
+					++byteCount;
 					break;
 				}
 				if (static_cast<std::size_t>(-1) == n) // decoding error?
 				{
-					result = APR_BADARG;
+					result = APR_BADCH;
 					break;
 				}
 				if (static_cast<std::size_t>(-2) == n) // incomplete sequence?
 				{
 					break;
 				}
 				Transcoder::encode(static_cast<unsigned int>(ch), out);
-				remain -= n;
-				i += n;
+				byteCount += n;
 				p += n;
 			}
-			in.position(i);
+			in.increment_position(byteCount);
 			return result;
 		}