Bugfix/ubasic-string-tokenizer-null-termination (#1196)

* Fixed tokenizer_string() off-by-one buffer overflow when processing string literals longer than the buffer limit.

* Fixed ubasic potential string buffer overflows by using snprintf instead of sprintf.

* Fixed ubasic label strings to use UBASIC_LABEL_LEN_MAX as buffer limit.

* Fixed ubasic string variables to initialize with zeros.

* Fixed compile errors when UBASIC_DEBUG_STRINGVARIABLES is defined.

* Added ubasic string variables user accessor API and unit testing for ubasic string variables.

* Fixed tokenizer_label() off-by-one buffer overflow when processing string literals longer than the buffer limit.

CHANGELOG.md

@@ -12,12 +12,20 @@ The git repositories are hosted at the following sites:
 * https://bacnet.sourceforge.net/
 * https://github.com/bacnet-stack/bacnet-stack/
 
-## [Unreleased] - 2025-12-04
+## [Unreleased] - 2026-01-03
 
 ### Security
 
 * Secured npdu_is_expected_reply() function where the MS/TP reply matcher
   could have an out-of-bounds read. (#1178)
+* Secured ubasic interpreter tokenizer_string() and tokenizer_label()
+  off-by-one buffer overflow when processing string literals longer
+  than the buffer limit.
+  Fixed ubasic potential string buffer overflows by using snprintf.
+  Fixed ubasic label strings to use UBASIC_LABEL_LEN_MAX as buffer limit.
+  Fixed ubasic string variables to initialize with zeros.
+  Fixed compile errors when UBASIC_DEBUG_STRINGVARIABLES is defined.
+  Added ubasic string variables user accessor API and unit testing. (#1196)
 
 ### Added
 

src/bacnet/basic/program/ubasic/tokenizer.c

@@ -556,8 +556,9 @@ void tokenizer_string(struct ubasic_tokenizer *tree, char *dest, uint8_t len)
     } while (*(string_end - 1) == '\\');
 
     string_len = string_end - tree->ptr - 1;
-    if (len < string_len) {
+    if (string_len > len - 1) {
-        string_len = len;
+        /* space for null terminator */
+        string_len = len - 1;
     }
     memcpy(dest, tree->ptr + 1, string_len);
     dest[string_len] = 0;
@@ -586,8 +587,9 @@ void tokenizer_label(struct ubasic_tokenizer *tree, char *dest, uint8_t len)
         }
         break;
     }
-    if (string_len > len) {
+    if (string_len > len - 1) {
-        string_len = len;
+        /* space for null terminator */
+        string_len = len - 1;
     }
     memcpy(dest, tree->ptr, string_len);
     dest[string_len] = 0;

src/bacnet/basic/program/ubasic/ubasic.c

@@ -837,7 +837,9 @@ static int16_t sstr(
     *(data->stringstack + bp) = 0;
     bp++;
 
-    sprintf((char *)(data->stringstack + bp), "%ld", (long)j);
+    snprintf(
+        (char *)(data->stringstack + bp), sizeof(data->stringstack) - bp, "%ld",
+        (long)j);
 
     data->freebufptr = bp + strlen((char *)(data->stringstack + bp)) + 1;
 
@@ -886,7 +888,7 @@ static int16_t sfactor(struct ubasic_data *data)
 {
     /* string form of factor */
     int16_t r = 0, s = 0;
-    char tmpstring[UBASIC_STRINGLEN_MAX];
+    char tmpstring[UBASIC_STRINGLEN_MAX] = { 0 };
     UBASIC_VARIABLE_TYPE i, j;
     struct ubasic_tokenizer *tree = &data->tree;
 
@@ -898,7 +900,7 @@ static int16_t sfactor(struct ubasic_data *data)
             break;
 
         case UBASIC_TOKENIZER_STRING:
-            tokenizer_string(tree, tmpstring, UBASIC_STRINGLEN_MAX);
+            tokenizer_string(tree, tmpstring, sizeof(tmpstring));
             r = scpy(data, tmpstring);
             accept(data, UBASIC_TOKENIZER_STRING);
             break;
@@ -1572,13 +1574,13 @@ static uint8_t jump_label(struct ubasic_data *data, char *label)
 
 static void gosub_statement(struct ubasic_data *data)
 {
-    char tmpstring[UBASIC_STRINGLEN_MAX];
+    char tmplabel[UBASIC_LABEL_LEN_MAX] = { 0 };
     struct ubasic_tokenizer *tree = &data->tree;
 
     accept(data, UBASIC_TOKENIZER_GOSUB);
     if (tokenizer_token(tree) == UBASIC_TOKENIZER_LABEL) {
         /* copy label */
-        tokenizer_label(tree, tmpstring, UBASIC_STRINGLEN_MAX);
+        tokenizer_label(tree, tmplabel, sizeof(tmplabel));
         tokenizer_next(tree);
         /* check for the end of line */
         while (tokenizer_token(tree) == UBASIC_TOKENIZER_EOL) {
@@ -1588,7 +1590,7 @@ static void gosub_statement(struct ubasic_data *data)
             data->gosub_stack[data->gosub_stack_ptr] =
                 tokenizer_save_offset(tree);
             data->gosub_stack_ptr++;
-            jump_label(data, tmpstring);
+            jump_label(data, tmplabel);
             return;
         }
     }
@@ -1616,15 +1618,15 @@ static void return_statement(struct ubasic_data *data)
 
 static void goto_statement(struct ubasic_data *data)
 {
-    char tmpstring[UBASIC_STRINGLEN_MAX];
+    char tmplabel[UBASIC_LABEL_LEN_MAX] = { 0 };
     struct ubasic_tokenizer *tree = &data->tree;
 
     accept(data, UBASIC_TOKENIZER_GOTO);
 
     if (tokenizer_token(tree) == UBASIC_TOKENIZER_LABEL) {
-        tokenizer_label(tree, tmpstring, sizeof(tmpstring));
+        tokenizer_label(tree, tmplabel, sizeof(tmplabel));
         tokenizer_next(tree);
-        jump_label(data, tmpstring);
+        jump_label(data, tmplabel);
         return;
     }
 
@@ -1860,7 +1862,7 @@ static void bac_write_statement(struct ubasic_data *data)
 static void print_statement(struct ubasic_data *data, uint8_t println)
 {
     uint8_t print_how = 0; /*0-xp, 1-hex, 2-oct, 3-dec, 4-bin*/
-    char tmpstring[UBASIC_STRINGLEN_MAX];
+    char tmpstring[UBASIC_STRINGLEN_MAX] = { 0 };
     struct ubasic_tokenizer *tree = &data->tree;
 
     /* string additions */
@@ -1879,30 +1881,37 @@ static void print_statement(struct ubasic_data *data, uint8_t println)
         }
 #if defined(UBASIC_VARIABLE_TYPE_STRING)
         if (tokenizer_token(tree) == UBASIC_TOKENIZER_STRING) {
-            tokenizer_string(tree, tmpstring, UBASIC_STRINGLEN_MAX);
+            tokenizer_string(tree, tmpstring, sizeof(tmpstring));
             tokenizer_next(tree);
         } else
 #endif
             if (tokenizer_token(tree) == UBASIC_TOKENIZER_COMMA) {
-            sprintf(tmpstring, " ");
+            snprintf(tmpstring, sizeof(tmpstring), " ");
             tokenizer_next(tree);
         } else {
 #if defined(UBASIC_VARIABLE_TYPE_STRING)
             if (tokenizer_stringlookahead(tree)) {
-                sprintf(tmpstring, "%s", strptr(data, sexpr(data)));
+                snprintf(
+                    tmpstring, sizeof(tmpstring), "%s",
+                    strptr(data, sexpr(data)));
             } else
 #endif
             {
                 if (print_how == 1) {
-                    sprintf(tmpstring, "%lx", (unsigned long)relation(data));
+                    snprintf(
+                        tmpstring, sizeof(tmpstring), "%lx",
+                        (unsigned long)relation(data));
                 } else if (print_how == 2) {
-                    sprintf(tmpstring, "%ld", (long)relation(data));
+                    snprintf(
+                        tmpstring, sizeof(tmpstring), "%ld",
+                        (long)relation(data));
                 } else {
 #if defined(UBASIC_VARIABLE_TYPE_FLOAT_AS_FIXEDPT_24_8) || \
     defined(UBASIC_VARIABLE_TYPE_FLOAT_AS_FIXEDPT_22_10)
                     fixedpt_str(relation(data), tmpstring, FIXEDPT_FBITS / 3);
 #else
-                    sprintf(tmpstring, "%ld", relation(data));
+                    snprintf(
+                        tmpstring, sizeof(tmpstring), "%ld", relation(data));
 #endif
                 }
             }
@@ -2407,7 +2416,8 @@ static void serial_getline_completed(struct ubasic_data *data)
 #if defined(UBASIC_VARIABLE_TYPE_FLOAT_AS_FIXEDPT_24_8) || \
     defined(UBASIC_VARIABLE_TYPE_FLOAT_AS_FIXEDPT_22_10)
                 r = str_fixedpt(
-                    data->statement, UBASIC_STRINGLEN_MAX, FIXEDPT_FBITS >> 1);
+                    data->statement, sizeof(data->statement),
+                    FIXEDPT_FBITS >> 1);
 #else
                 r = atoi(data->statement);
 #endif
@@ -3090,9 +3100,9 @@ void ubasic_set_stringvariable(
 #if defined(UBASIC_DEBUG_STRINGVARIABLES)
         serial_write_string(data, "set_stringvar:");
         char msg[12];
-        sprintf(msg, "[%d]", stringvariables[svarnum]);
+        snprintf(msg, sizeof(msg), "[%d]", data->stringvariables[svarnum]);
         serial_write_string(data, msg);
-        serial_write_string(data, strptr(stringvariables[svarnum]));
+        serial_write_string(data, strptr(data, data->stringvariables[svarnum]));
         serial_write_string(data, "\n");
 #endif
     }
@@ -3106,9 +3116,9 @@ int16_t ubasic_get_stringvariable(struct ubasic_data *data, uint8_t varnum)
 #if defined(UBASIC_DEBUG_STRINGVARIABLES)
         serial_write_string(data, "get_stringvar:");
         char msg[12];
-        sprintf(msg, "[%d]", stringvariables[varnum]);
+        snprintf(msg, sizeof(msg), "[%d]", data->stringvariables[varnum]);
         serial_write_string(data, msg);
-        serial_write_string(data, strptr(stringvariables[varnum]));
+        serial_write_string(data, strptr(data, data->stringvariables[varnum]));
         serial_write_string(data, "\n");
 #endif
 
@@ -3117,6 +3127,31 @@ int16_t ubasic_get_stringvariable(struct ubasic_data *data, uint8_t varnum)
 
     return (-1);
 }
+
+/**
+ * @brief Get pointer to string variable value.
+ * @param data ubasic data structure.
+ * @param variable string variable 'a' through 'z' or 'A' through 'Z'.
+ * @return Pointer to the string value, or NULL if variable is invalid.
+ */
+const char *ubasic_ptr_stringvariable(struct ubasic_data *data, char variable)
+{
+    uint8_t varnum;
+
+    if (isupper(variable)) {
+        varnum = variable - 'A';
+    } else if (islower(variable)) {
+        varnum = variable - 'a';
+    } else {
+        return NULL;
+    }
+    if (varnum < UBASIC_STRING_VAR_LEN_MAX) {
+        return strptr(data, data->stringvariables[varnum]);
+    }
+
+    return NULL;
+}
+
 /* */
 /* end of string additions */
 /* */

src/bacnet/basic/program/ubasic/ubasic.h

@@ -270,6 +270,8 @@ int16_t ubasic_get_stringvariable(struct ubasic_data *data, uint8_t varnum);
 BACNET_STACK_EXPORT
 void ubasic_set_stringvariable(
     struct ubasic_data *data, uint8_t varnum, int16_t size);
+BACNET_STACK_EXPORT
+const char *ubasic_ptr_stringvariable(struct ubasic_data *data, char variable);
 #endif
 
 /* API to interface and initialize the ported hardware drivers */

test/bacnet/basic/program/ubasic/src/main.c

@@ -383,6 +383,62 @@ static void test_ubasic_math(void)
     return;
 }
 
+/**
+ * @brief Verify uBASIC string variable handling.
+ * @details Ensures that string variables are truncated at UBASIC_STRINGLEN_MAX
+ * and remain correctly null-terminated when assigned and read back.
+ */
+#if defined(CONFIG_ZTEST_NEW_API)
+ZTEST(ubasic_tests, test_ubasic_strings)
+#else
+static void test_ubasic_strings(void)
+#endif
+{
+    struct ubasic_data data = { 0 };
+    const char *program1 =
+        /* program listing with either \0, \n, or ';' at the end of each line.
+           note: indentation is not required */
+        "println 'Demo - Strings'\n"
+        "let a$ = \"1234567890123456789012345678901234567890X\"\n"
+        "print a$\n";
+    const char *program2 =
+        /* program listing with either \0, \n, or ';' at the end of each line.
+           note: indentation is not required */
+        "println 'Demo - Strings'\n"
+        "let z$ = \"1\"\n"
+        "print z$\n";
+    const char *tmpstring;
+
+    ubasic_load_program(&data, program1);
+    zassert_equal(data.status.bit.isRunning, 1, NULL);
+    zassert_equal(data.status.bit.Error, 0, NULL);
+    while (!ubasic_finished(&data)) {
+        ubasic_run_program(&data);
+    }
+    zassert_equal(data.status.bit.Error, 0, NULL);
+    tmpstring = ubasic_ptr_stringvariable(&data, 'a');
+    zassert_not_null(tmpstring, "string variable not found");
+    zassert_equal(
+        strlen(tmpstring), UBASIC_STRINGLEN_MAX - 1, "string length=%d",
+        (int)strlen(tmpstring));
+    zassert_equal(
+        strcmp(tmpstring, "123456789012345678901234567890123456789"), 0,
+        "string value=%s", tmpstring);
+
+    ubasic_load_program(&data, program2);
+    zassert_equal(data.status.bit.isRunning, 1, NULL);
+    zassert_equal(data.status.bit.Error, 0, NULL);
+    while (!ubasic_finished(&data)) {
+        ubasic_run_program(&data);
+    }
+    zassert_equal(data.status.bit.Error, 0, NULL);
+    tmpstring = ubasic_ptr_stringvariable(&data, 'z');
+    zassert_not_null(tmpstring, "string variable not found");
+    zassert_equal(
+        strlen(tmpstring), 1, "string length=%d", (int)strlen(tmpstring));
+    zassert_equal(strcmp(tmpstring, "1"), 0, "string value=%s", tmpstring);
+}
+
 /**
  * @brief Test
  */
@@ -439,8 +495,8 @@ void test_main(void)
 {
     ztest_test_suite(
         ubasic_tests, ztest_unit_test(test_ubasic),
-        ztest_unit_test(test_ubasic_math), ztest_unit_test(test_ubasic_bacnet),
+        ztest_unit_test(test_ubasic_strings), ztest_unit_test(test_ubasic_math),
-        ztest_unit_test(test_ubasic_gpio));
+        ztest_unit_test(test_ubasic_bacnet), ztest_unit_test(test_ubasic_gpio));
 
     ztest_run_test_suite(ubasic_tests);
 }