From 43bf2f17e1c8d17be979d4e489588baaecbd410a Mon Sep 17 00:00:00 2001
From: Georgi Gerganov <ggerganov@gmail.com>
Date: Tue, 21 Sep 2021 07:07:12 +0300
Subject: [PATCH] ggwave : fix out-of-bounds access in ggwave_decode (#53)

Also, provide a memory-safe overload called ggwave_ndecode()
The overload takes an extra parameter that specifies the size of
the output buffer and thus limits the size of the Rx payload that can be
decoded and stored.
---
 include/ggwave/ggwave.h | 24 +++++++++++++++++++++--
 src/ggwave.cpp          | 43 +++++++++++++++++++++++++++++++++++++++--
 tests/test-ggwave.c     | 18 +++++++++++++----
 3 files changed, 77 insertions(+), 8 deletions(-)

diff --git a/include/ggwave/ggwave.h b/include/ggwave/ggwave.h
index 3dac5fc..2cf6d60 100644
--- a/include/ggwave/ggwave.h
+++ b/include/ggwave/ggwave.h
@@ -206,8 +206,10 @@ extern "C" {
     //   If the return value is -1 then there was an error during the decoding process.
     //   Usually can occur if there is a lot of background noise in the audio.
     //
-    //   If the return value is greater than 0, then there will be that number of bytes
-    //   decoded in the outputBuffer
+    //   If the return value is greater than 0, then there are that number of bytes decoded.
+    //
+    //   IMPORTANT:
+    //   Notice that the decoded data written to the outputBuffer is NOT null terminated.
     //
     //   Example:
     //
@@ -235,6 +237,22 @@ extern "C" {
             int dataSize,
             char * outputBuffer);
 
+    // Memory-safe overload of ggwave_decode
+    //
+    //   outputSize     - optionally specify the size of the output buffer
+    //
+    //   If the return value is -2 then the provided outputBuffer was not big enough to
+    //   store the decoded data.
+    //
+    //   See ggwave_decode for more information
+    //
+    GGWAVE_API int ggwave_ndecode(
+            ggwave_Instance instance,
+            const char * dataBuffer,
+            int dataSize,
+            char * outputBuffer,
+            int outputSize);
+
 #ifdef __cplusplus
 }
 
@@ -414,6 +432,8 @@ class GGWave {
     void setRxProtocols(const RxProtocols & rxProtocols) { m_rxProtocols = rxProtocols; }
     const RxProtocols & getRxProtocols() const { return m_rxProtocols; }
 
+    int lastRxDataLength() const { return m_lastRxDataLength; }
+
     const TxRxData & getRxData()            const { return m_rxData; }
     const RxProtocol & getRxProtocol()      const { return m_rxProtocol; }
     const RxProtocolId & getRxProtocolId()  const { return m_rxProtocolId; }
diff --git a/src/ggwave.cpp b/src/ggwave.cpp
index acdb5cf..081447b 100644
--- a/src/ggwave.cpp
+++ b/src/ggwave.cpp
@@ -125,7 +125,7 @@ int ggwave_decode(
 
     ggWave->decode(cbWaveformInp);
 
-    // todo : avoid allocation
+    // TODO : avoid allocation
     GGWave::TxRxData rxData;
 
     auto rxDataLength = ggWave->takeRxData(rxData);
@@ -133,7 +133,46 @@ int ggwave_decode(
         // failed to decode message
         return -1;
     } else if (rxDataLength > 0) {
-        std::copy(rxData.begin(), rxData.end(), outputBuffer);
+        memcpy(outputBuffer, rxData.data(), rxDataLength);
+    }
+
+    return rxDataLength;
+}
+
+extern "C"
+int ggwave_ndecode(
+        ggwave_Instance instance,
+        const char * dataBuffer,
+        int dataSize,
+        char * outputBuffer,
+        int outputSize) {
+    // TODO : avoid duplicated code
+    GGWave * ggWave = (GGWave *) g_instances[instance];
+
+    GGWave::CBWaveformInp cbWaveformInp = [&](void * data, uint32_t nMaxBytes) -> uint32_t {
+        uint32_t nCopied = std::min((uint32_t) dataSize, nMaxBytes);
+        std::copy(dataBuffer, dataBuffer + nCopied, (char *) data);
+
+        dataSize -= nCopied;
+        dataBuffer += nCopied;
+
+        return nCopied;
+    };
+
+    ggWave->decode(cbWaveformInp);
+
+    // TODO : avoid allocation
+    GGWave::TxRxData rxData;
+
+    auto rxDataLength = ggWave->takeRxData(rxData);
+    if (rxDataLength == -1) {
+        // failed to decode message
+        return -1;
+    } else if (rxDataLength > outputSize) {
+        // the outputBuffer is not big enough to store the data
+        return -2;
+    } else if (rxDataLength > 0) {
+        memcpy(outputBuffer, rxData.data(), rxDataLength);
     }
 
     return rxDataLength;
diff --git a/tests/test-ggwave.c b/tests/test-ggwave.c
index ebc756f..82a457b 100644
--- a/tests/test-ggwave.c
+++ b/tests/test-ggwave.c
@@ -25,17 +25,27 @@ int main() {
 
     int ret;
     const char * payload = "test";
-    char decoded[256];
+    char decoded[16];
 
     int n = ggwave_encode(instance, payload, 4, GGWAVE_TX_PROTOCOL_AUDIBLE_FAST, 50, NULL, 1);
     char waveform[n];
 
-    ret = ggwave_encode(instance, payload, 4, GGWAVE_TX_PROTOCOL_AUDIBLE_FAST, 50, waveform, 0);
-    CHECK(ret > 0);
+    int ne = ggwave_encode(instance, payload, 4, GGWAVE_TX_PROTOCOL_AUDIBLE_FAST, 50, waveform, 0);
+    CHECK(ne > 0);
 
-    ret = ggwave_decode(instance, waveform, sizeof(signed short)*ret, decoded);
+    // not enough output buffer size to store the decoded message
+    ret = ggwave_ndecode(instance, waveform, sizeof(signed short)*ne, decoded, 3);
+    CHECK(ret == -2); // fail
+
+    // just enough size to store it
+    ret = ggwave_ndecode(instance, waveform, sizeof(signed short)*ne, decoded, 4);
+    CHECK(ret == 4);  // success
+
+    // unsafe method - will write the decoded output to the output buffer regardless of the size
+    ret = ggwave_decode(instance, waveform, sizeof(signed short)*ne, decoded);
     CHECK(ret == 4);
 
+    decoded[ret] = 0; // null-terminate the received data
     CHECK(strcmp(decoded, payload) == 0);
 
     ggwave_free(instance);