bug: Undefined behavior regression

Previously we had allowed strings to be sent via `note-c` without a newline-terminator.
Subsequently, we decided this was undefined behavior that should not be supported.
This caused a regression in downstream testing, and we have restored the original
behavior (although undefined by the Notecard communication specification).

n_request.c

@@ -287,8 +287,9 @@ J *NoteRequestResponseWithRetry(J *req, uint32_t timeoutSeconds)
  @brief Send a request to the Notecard and return the response.
 
  Unlike `NoteRequestResponse`, this function expects the request to be a valid
- JSON C-string, rather than a `J` object. This string MUST be newline-terminated.
- The response is returned as a dynamically allocated JSON C-string. The response
+ JSON C-string, rather than a `J` object. The string is expected to be
+ newline-terminated, otherwise the call results in undefined behavior. The
+ response is returned as a dynamically allocated JSON C-string. The response
  string is verbatim what was sent by the Notecard, which IS newline-terminated.
  The caller is responsible for freeing the response string. If the request was a
  command (i.e. it uses "cmd" instead of "req"), this function returns NULL,
@@ -319,11 +320,32 @@ char * NoteRequestResponseJSON(const char *reqJSON)
 
  // Manually tokenize the string to search for multiple embedded commands (cannot use strtok)
  for (;;) {
- const char * const endPtr = strchr(reqJSON, '\n');
+ const char *endPtr;
+ const char * const newlinePtr = strchr(reqJSON, '\n');
+
+ // If string is not newline-terminated, then allocate a new string and terminate it
+ if (NULL == newlinePtr) {
+ // All JSON strings should be newline-terminated to meet the specification, however
+ // this is required to ensure backward compatibility with the previous implementation.
+ const size_t tempLen = strlen(reqJSON);
+ if (0 == tempLen) {
+ NOTE_C_LOG_ERROR(ERRSTR("request: jsonbuf zero length", c_bad));
+ break;
+ }
 
- // If string is not newline-terminated, then do not process
- if (endPtr == NULL) {
- break;
+ NOTE_C_LOG_WARN(ERRSTR("Memory allocation due to malformed request (not newline-terminated)", c_bad));
+ char * const temp = _Malloc(tempLen + 1);
+ if (temp == NULL) {
+ NOTE_C_LOG_ERROR(ERRSTR("request: jsonbuf malloc failed", c_mem));
+ break;
+ }
+
+ memcpy(temp, reqJSON, tempLen);
+ temp[tempLen] = '\n';
+ reqJSON = temp;
+ endPtr = &reqJSON[tempLen];
+ } else {
+ endPtr = newlinePtr;
  }
  const size_t reqLen = ((endPtr - reqJSON) + 1);
 
@@ -334,25 +356,35 @@ char * NoteRequestResponseJSON(const char *reqJSON)
  J *jsonObj = JParse(reqJSON);
  if (!jsonObj) {
  // Invalid JSON.
- return NULL;
+ if (NULL == newlinePtr) {
+ _Free((void *)reqJSON);
+ }
+ break;
  }
  isCmd = JIsPresent(jsonObj, "cmd");
  JDelete(jsonObj);
  }
 
- if (isCmd) {
+ if (!isCmd) {
+ _Transaction(reqJSON, reqLen, &rspJSON, transactionTimeoutMs);
+ if (NULL == newlinePtr) {
+ _Free((void *)reqJSON);
+ }
+ break;
+ } else {
  // If it's a command, the Notecard will not respond, so we pass NULL for
  // the response parameter.
  _Transaction(reqJSON, reqLen, NULL, transactionTimeoutMs);
  reqJSON = (endPtr + 1);
- } else {
- _Transaction(reqJSON, reqLen, &rspJSON, transactionTimeoutMs);
- break;
+ }
+
+ // Clean up if we allocated a new string
+ if (NULL == newlinePtr) {
+ _Free((void *)reqJSON);
  }
  }
 
  _UnlockNote();
-
  _TransactionStop();
 
  return rspJSON;

test/src/NoteRequestResponseJSON_test.cpp

@@ -19,9 +19,12 @@
 #include "n_lib.h"
 
 DEFINE_FFF_GLOBALS
+FAKE_VALUE_FUNC(N_CJSON_PUBLIC(J *), JParse, const char *)
 FAKE_VALUE_FUNC(bool, noteTransactionStart, uint32_t)
 FAKE_VOID_FUNC(noteTransactionStop)
 FAKE_VOID_FUNC(noteLockNote)
+FAKE_VALUE_FUNC(void *, NoteMalloc, size_t)
+FAKE_VOID_FUNC(NoteFree, void *)
 FAKE_VOID_FUNC(noteUnlockNote)
 FAKE_VALUE_FUNC(const char *, noteJSONTransaction, const char *, size_t, char **, uint32_t)
 
@@ -32,19 +35,42 @@ SCENARIO("NoteRequestResponseJSON")
 {
  NoteSetFnDefault(malloc, free, NULL, NULL);
 
+ JParse_fake.custom_fake = [](const char *value) -> J * {
+ return JParseWithOpts(value,0,0);
+ };
+ NoteMalloc_fake.custom_fake = malloc;
+ NoteFree_fake.custom_fake = free;
  noteTransactionStart_fake.return_val = true;
 
  GIVEN("The request is NULL") {
  WHEN("NoteRequestResponseJSON is called") {
  char *rsp = NoteRequestResponseJSON(NULL);
 
+ THEN("noteJSONTransaction is not called") {
+ CHECK(noteJSONTransaction_fake.call_count == 0);
+ }
+
  THEN("The response is NULL") {
  CHECK(rsp == NULL);
  }
  }
  }
 
- GIVEN("The request is a command") {
+ GIVEN("The request length is zero (0)") {
+ WHEN("NoteRequestResponseJSON is called") {
+ char *rsp = NoteRequestResponseJSON("");
+
+ THEN("noteJSONTransaction is not called") {
+ CHECK(noteJSONTransaction_fake.call_count == 0);
+ }
+
+ THEN("The response is NULL") {
+ CHECK(rsp == NULL);
+ }
+ }
+ }
+
+ GIVEN("The request is a command (cmd)") {
  char req[] = "{\"cmd\":\"card.sleep\"}\n";
 
  WHEN("NoteRequestResponseJSON is called") {
@@ -87,7 +113,7 @@ SCENARIO("NoteRequestResponseJSON")
  }
  }
 
- GIVEN("The request is not a command") {
+ GIVEN("The request is not a command (req)") {
  char req[] = "{\"req\":\"card.version\"}\n";
 
  WHEN("NoteRequestResponseJSON is called") {
@@ -121,14 +147,65 @@ SCENARIO("NoteRequestResponseJSON")
  GIVEN("The request doesn't have a terminating newline") {
  char req[] = "{\"req\":\"card.version\"}";
 
- WHEN("NoteRequestResponseJSON is called") {
+ WHEN("NoteMalloc fails to duplicate the request") {
+ NoteMalloc_fake.custom_fake = nullptr;
+ NoteMalloc_fake.return_val = NULL;
  char *rsp = NoteRequestResponseJSON(req);
 
- THEN("noteJSONTransaction isn't called") {
+ THEN("noteJSONTransaction is not called") {
+ REQUIRE(NoteMalloc_fake.call_count > 0);
  CHECK(noteJSONTransaction_fake.call_count == 0);
  }
 
- THEN("NULL is returned") {
+ THEN("NoteFree is not called") {
+ REQUIRE(NoteMalloc_fake.call_count > 0);
+ CHECK(NoteFree_fake.call_count == 0);
+ }
+
+ THEN("NoteRequestResponseJSON returns NULL") {
+ REQUIRE(NoteMalloc_fake.call_count > 0);
+ CHECK(rsp == NULL);
+ }
+ }
+
+ WHEN("NoteMalloc is able to duplicate the request") {
+ char *rsp = NoteRequestResponseJSON(req);
+
+ THEN("NoteMalloc is called") {
+ REQUIRE(noteJSONTransaction_fake.call_count > 0);
+ CHECK(NoteMalloc_fake.call_count > 0);
+ }
+
+ THEN("noteJSONTransaction is called with the newline appended") {
+ REQUIRE(noteJSONTransaction_fake.call_count > 0);
+ REQUIRE(noteJSONTransaction_fake.arg0_history[0] != NULL);
+ CHECK(noteJSONTransaction_fake.arg0_history[0][(sizeof(req) - 1)] == '\n');
+ CHECK(noteJSONTransaction_fake.arg1_history[0] == sizeof(req));
+ }
+
+ THEN("NoteFree is called to free the memory allocated by malloc") {
+ REQUIRE(noteJSONTransaction_fake.call_count > 0);
+ CHECK(NoteFree_fake.call_count == NoteMalloc_fake.call_count);
+ }
+ }
+
+ WHEN("The request fails to parse") {
+ char cmd[] = "{\"cmd\":\"card.version\"}";
+ JParse_fake.custom_fake = nullptr;
+ JParse_fake.return_val = NULL;
+ char *rsp = NoteRequestResponseJSON(cmd);
+
+ THEN("noteJSONTransaction is not called") {
+ REQUIRE(NoteMalloc_fake.call_count > 0);
+ CHECK(noteJSONTransaction_fake.call_count == 0);
+ }
+
+ THEN("NoteFree is called") {
+ REQUIRE(NoteMalloc_fake.call_count > 0);
+ CHECK(NoteFree_fake.call_count == NoteMalloc_fake.call_count);
+ }
+
+ THEN("NoteRequestResponseJSON returns NULL") {
  CHECK(rsp == NULL);
  }
  }
@@ -152,6 +229,16 @@ SCENARIO("NoteRequestResponseJSON")
  CHECK(noteTransactionStart_fake.call_count == 1);
  CHECK(noteTransactionStop_fake.call_count == 1);
  }
+
+ THEN("NoteMalloc is not called") {
+ REQUIRE(noteJSONTransaction_fake.call_count > 0);
+ CHECK(NoteMalloc_fake.call_count == 0);
+ }
+
+ THEN("NoteFree is not called") {
+ REQUIRE(noteJSONTransaction_fake.call_count > 0);
+ CHECK(NoteFree_fake.call_count == 0);
+ }
  }
 
  AND_GIVEN("The transaction with the Notecard fails to start") {
@@ -168,6 +255,9 @@ SCENARIO("NoteRequestResponseJSON")
  }
  }
 
+ RESET_FAKE(JParse);
+ RESET_FAKE(NoteMalloc);
+ RESET_FAKE(NoteFree);
  RESET_FAKE(noteTransactionStart);
  RESET_FAKE(noteTransactionStop);
  RESET_FAKE(noteLockNote);