From 629c999336b059cbe1824955979335ebd46a145c Mon Sep 17 00:00:00 2001
From: BodgeMaster <>
Date: Sat, 27 Aug 2022 22:35:10 +0200
Subject: [PATCH] lib/nbt: Return correct error code from read functions (fixes
 #17)

---
 src/lib/nbt.cpp                     | 46 ++++++++++++++++++++---------
 src/test/nbt_read_write_helpers.cpp | 13 ++++----
 2 files changed, 39 insertions(+), 20 deletions(-)

diff --git a/src/lib/nbt.cpp b/src/lib/nbt.cpp
index d65fe42..2bd8840 100644
--- a/src/lib/nbt.cpp
+++ b/src/lib/nbt.cpp
@@ -38,17 +38,19 @@
 namespace NBT {
     namespace helper {
         ErrorOr<int8_t> readInt8(uint8_t data[], uint64_t dataSize, uint64_t currentPosition) {
-            if (dataSize<currentPosition+1) return ErrorOr<int8_t>(true, ErrorCodes::OUT_OF_RANGE);
+            if (currentPosition>=dataSize) return ErrorOr<int8_t>(true, ErrorCodes::OUT_OF_RANGE);
             return ErrorOr<int8_t>((int8_t) data[currentPosition]);
         }
 
         ErrorOr<int16_t> readInt16(uint8_t data[], uint64_t dataSize, uint64_t currentPosition) {
-            if (dataSize<currentPosition+2) return ErrorOr<int16_t>(true, ErrorCodes::OUT_OF_RANGE);
+            if (currentPosition>=dataSize) return ErrorOr<int16_t>(true, ErrorCodes::OUT_OF_RANGE);
+            if (dataSize<currentPosition+2) return ErrorOr<int16_t>(true, ErrorCodes::OVERRUN);
             return ErrorOr<int16_t>((int16_t) ((static_cast<int16_t>(data[currentPosition]) << 8) | static_cast<int16_t>(data[currentPosition+1])));
         }
 
         ErrorOr<int32_t> readInt32(uint8_t data[], uint64_t dataSize, uint64_t currentPosition) {
-            if (dataSize<currentPosition+4) return ErrorOr<int32_t>(true, ErrorCodes::OUT_OF_RANGE);
+            if (currentPosition>=dataSize) return ErrorOr<int32_t>(true, ErrorCodes::OUT_OF_RANGE);
+            if (dataSize<currentPosition+4) return ErrorOr<int32_t>(true, ErrorCodes::OVERRUN);
             return ErrorOr<int32_t>((int32_t) (
                 (static_cast<int32_t>(data[currentPosition  ]) << 24) |
                 (static_cast<int32_t>(data[currentPosition+1]) << 16) |
@@ -58,7 +60,8 @@ namespace NBT {
         }
 
         ErrorOr<int64_t> readInt64(uint8_t data[], uint64_t dataSize, uint64_t currentPosition) {
-            if (dataSize<currentPosition+8) return ErrorOr<int64_t>(true, ErrorCodes::OUT_OF_RANGE);
+            if (currentPosition>=dataSize) return ErrorOr<int64_t>(true, ErrorCodes::OUT_OF_RANGE);
+            if (dataSize<currentPosition+8) return ErrorOr<int64_t>(true, ErrorCodes::OVERRUN);
             return ErrorOr<int64_t>((int64_t) (
                 (static_cast<int64_t>(data[currentPosition  ]) << 56) |
                 (static_cast<int64_t>(data[currentPosition+1]) << 48) |
@@ -132,9 +135,14 @@ namespace NBT {
         }
 
         ErrorOr<std::vector<int8_t>> readInt8Array(uint8_t data[], uint64_t dataSize, uint64_t currentPosition) {
-            // get size prefix
+
             ErrorOr<int32_t> size = readInt32(data, dataSize, currentPosition);
-            if (size.isError) return ErrorOr<std::vector<int8_t>>(true, size.errorCode);
+            if (size.isError) {
+                // It is okay to pass up the error code here without further
+                // processing because both OVERRUN and OUT_OF_RANGE errors will
+                // still apply.
+                return ErrorOr<std::vector<int8_t>>(true, size.errorCode);
+            }
 
             // get content
             if (currentPosition+4+size.value > dataSize) return ErrorOr<std::vector<int8_t>>(true, ErrorCodes::OVERRUN);
@@ -146,12 +154,12 @@ namespace NBT {
         }
 
         ErrorOr<tiny_utf8::string> readString(uint8_t data[], uint64_t dataSize, uint64_t currentPosition) {
-            if(currentPosition > dataSize){
-                return ErrorOr<tiny_utf8::string>(true, ErrorCodes::OVERRUN);
-            }
 
             ErrorOr<int16_t> stringSize = readInt16(data, dataSize, currentPosition);
             if (stringSize.isError) {
+                // It is okay to pass up the error code here without further
+                // processing because both OVERRUN and OUT_OF_RANGE errors will
+                // still apply.
                 return ErrorOr<tiny_utf8::string>(true, stringSize.errorCode);
             }
             if (currentPosition + (uint64_t) stringSize.value + 2 > dataSize) {
@@ -166,9 +174,14 @@ namespace NBT {
         }
 
         ErrorOr<std::vector<int32_t>> readInt32Array(uint8_t data[], uint64_t dataSize, uint64_t currentPosition) {
-            // get size prefix
+
             ErrorOr<int32_t> size = readInt32(data, dataSize, currentPosition);
-            if (size.isError) return ErrorOr<std::vector<int32_t>>(true, size.errorCode);
+            if (size.isError) {
+                // It is okay to pass up the error code here without further
+                // processing because both OVERRUN and OUT_OF_RANGE errors will
+                // still apply.
+                return ErrorOr<std::vector<int32_t>>(true, size.errorCode);
+            }
 
             // get content
             if (currentPosition+4+(size.value*4) > dataSize) return ErrorOr<std::vector<int32_t>>(true, ErrorCodes::OVERRUN);
@@ -182,9 +195,14 @@ namespace NBT {
         }
 
         ErrorOr<std::vector<int64_t>> readInt64Array(uint8_t data[], uint64_t dataSize, uint64_t currentPosition) {
-            // get size prefix
+
             ErrorOr<int32_t> size = readInt32(data, dataSize, currentPosition);
-            if (size.isError) return ErrorOr<std::vector<int64_t>>(true, size.errorCode);
+            if (size.isError) {
+                // It is okay to pass up the error code here without further
+                // processing because both OVERRUN and OUT_OF_RANGE errors will
+                // still apply.
+                return ErrorOr<std::vector<int64_t>>(true, size.errorCode);
+            }
 
             // get content
             if (currentPosition+4+(size.value*8) > dataSize) return ErrorOr<std::vector<int64_t>>(true, ErrorCodes::OVERRUN);
@@ -389,7 +407,7 @@ namespace NBT {
         // feature as compound tags and lists need to be dealt with
         // separately to avoid unnecessarily long and complex code.
         //
-        // Regardinng lists specifically: The size of some lists can can
+        // Regarding lists specifically: The size of some lists can can
         // be determined easily by looking at the contained data type and
         // size information but cases like string lists or compound lists
         // are significantly more difficult to deal with. Parsing their
diff --git a/src/test/nbt_read_write_helpers.cpp b/src/test/nbt_read_write_helpers.cpp
index e6cb5a1..b53a9ae 100644
--- a/src/test/nbt_read_write_helpers.cpp
+++ b/src/test/nbt_read_write_helpers.cpp
@@ -86,7 +86,7 @@ int main(){
     // partially out of bounds
     currentPosition = 9;
     ASSERT(NBT::helper::readInt16(dataForIntTest, dataSize, currentPosition).isError == true);
-    ASSERT(NBT::helper::readInt16(dataForIntTest, dataSize, currentPosition).errorCode == ErrorCodes::OUT_OF_RANGE);
+    ASSERT(NBT::helper::readInt16(dataForIntTest, dataSize, currentPosition).errorCode == ErrorCodes::OVERRUN);
     // fully out of bounds
     currentPosition = 10;
     ASSERT(NBT::helper::readInt16(dataForIntTest, dataSize, currentPosition).isError == true);
@@ -122,7 +122,7 @@ int main(){
     // partially out of bounds
     currentPosition = 7;
     ASSERT(NBT::helper::readInt32(dataForIntTest, dataSize, currentPosition).isError == true);
-    ASSERT(NBT::helper::readInt32(dataForIntTest, dataSize, currentPosition).errorCode == ErrorCodes::OUT_OF_RANGE);
+    ASSERT(NBT::helper::readInt32(dataForIntTest, dataSize, currentPosition).errorCode == ErrorCodes::OVERRUN);
     // fully out of bounds
     currentPosition = 10;
     ASSERT(NBT::helper::readInt32(dataForIntTest, dataSize, currentPosition).isError == true);
@@ -164,7 +164,7 @@ int main(){
     // partially out of bounds
     currentPosition = 3;
     ASSERT(NBT::helper::readInt64(dataForIntTest, dataSize, currentPosition).isError == true);
-    ASSERT(NBT::helper::readInt64(dataForIntTest, dataSize, currentPosition).errorCode == ErrorCodes::OUT_OF_RANGE);
+    ASSERT(NBT::helper::readInt64(dataForIntTest, dataSize, currentPosition).errorCode == ErrorCodes::OVERRUN);
     // fully out of bounds
     currentPosition = 10;
     ASSERT(NBT::helper::readInt64(dataForIntTest, dataSize, currentPosition).isError == true);
@@ -226,7 +226,7 @@ int main(){
     // read with size partially out of bounds
     currentPosition = 114;
     ASSERT(NBT::helper::readInt8Array(dataForIntArrayTest, dataSize, currentPosition).isError == true);
-    ASSERT(NBT::helper::readInt8Array(dataForIntArrayTest, dataSize, currentPosition).errorCode == ErrorCodes::OUT_OF_RANGE);
+    ASSERT(NBT::helper::readInt8Array(dataForIntArrayTest, dataSize, currentPosition).errorCode == ErrorCodes::OVERRUN);
     // read out of bounds
     currentPosition = 200;
     ASSERT(NBT::helper::readInt8Array(dataForIntArrayTest, dataSize, currentPosition).isError == true);
@@ -282,7 +282,7 @@ int main(){
     // read with size partially out of bounds
     currentPosition = 114;
     ASSERT(NBT::helper::readInt32Array(dataForIntArrayTest, dataSize, currentPosition).isError == true);
-    ASSERT(NBT::helper::readInt32Array(dataForIntArrayTest, dataSize, currentPosition).errorCode == ErrorCodes::OUT_OF_RANGE);
+    ASSERT(NBT::helper::readInt32Array(dataForIntArrayTest, dataSize, currentPosition).errorCode == ErrorCodes::OVERRUN);
     // read out of bounds
     currentPosition = 200;
     ASSERT(NBT::helper::readInt32Array(dataForIntArrayTest, dataSize, currentPosition).isError == true);
@@ -347,7 +347,7 @@ int main(){
     // read with size partially out of bounds
     currentPosition = 114;
     ASSERT(NBT::helper::readInt64Array(dataForIntArrayTest, dataSize, currentPosition).isError == true);
-    ASSERT(NBT::helper::readInt64Array(dataForIntArrayTest, dataSize, currentPosition).errorCode == ErrorCodes::OUT_OF_RANGE);
+    ASSERT(NBT::helper::readInt64Array(dataForIntArrayTest, dataSize, currentPosition).errorCode == ErrorCodes::OVERRUN);
     // read out of bounds
     currentPosition = 200;
     ASSERT(NBT::helper::readInt64Array(dataForIntArrayTest, dataSize, currentPosition).isError == true);
@@ -537,6 +537,7 @@ int main(){
 
     javaStdString[0] = '1';
     ASSERT(NBT::helper::readString(reinterpret_cast<uint8_t*>(javaStdString.data()), javaStdString.size(), 0).errorCode == ErrorCodes::OVERRUN);
+    ASSERT(NBT::helper::readString(reinterpret_cast<uint8_t*>(javaStdString.data()), javaStdString.size(), javaStdString.size()).errorCode == ErrorCodes::OUT_OF_RANGE);
 
     //reading data from the blob at the top of this file
     currentPositionInBlob = 0x1cf;