/rfid endpoint - Serve with chunked response #280

tueddy · 2023-12-12T10:51:23Z

/rfid endpoint:

A static buffer of 8KB is currently used to list the RFID tags. If this buffer is full, the returned JSON is truncated and not all entries are displayed in the web interface. . This happens with me from approx. 60 entries (depends on path length). It could also lead to a low memory situation and, in the worst case, to a crash.

With this PR, a list is first created holding the keys only and later the details such as path and game mode are sent as a chunked response with a smaller buffer.
No changes in the delivered JSON except that it is always complete

SZenglein · 2023-12-12T19:31:14Z

src/Web.cpp

+}
+
+static String tagIdToJsonStr(const char *key) {
+	StaticJsonDocument<512> doc;


Isn't 512 bytes a bit large for a single tag id?

A single tag id looks like:

{ "id": "003108198106", "fileOrUrl": "/Die Schönsten Lieder Zum Einschlafen", "playMode": 3, "lastPlayPos": 1842837, "trackLastPlayed": 0 }

maxLen of "fileOrUrl" is MAX_FILEPATH_LENTGH = 256, size calculation with https://arduinojson.org/v6/assistant gives about 350 Bytes needed. Recommended is power of 2, so 512 Bytes is best size.

SZenglein · 2023-12-12T19:38:14Z

src/Web.cpp

 	}
+	static std::vector<String> nvsKeys {};


This vector will never shrink, just something to be aware of. Though it's good to avoid frequent allocations...

nvsKeys is declared as local variable and i assume it gets out of scope after function handleGetRFIDRequest() finished.
Am i wrong here?

It's declared as static, meaning it lives as long as the entire program.
If you didn't add nvsKeys.clear() directly afterwards, I it should continue to grow with each call.

The upside is that push_back does not need to reserve more memory every time and instead can re-use the same heap space. The downside is that the heap space that is the capacity of the vector is never reduced or freed.

The downside is that the heap space that is the capacity of the vector is never reduced or freed.

And because of this, I'd advice against static here. The main issue is, that we do not have any idea, how many entries there are and we are basically reserving heap space (at the minimum sizeof(String) * [maxEntries], propably way more).

Also what you have to keep in mind is, that as soon as std::vector runs out of space in it's internal array, it reserves a new memory block on the heap with the factor of 1.5 / 2 (so if an array of 12 elements is full and another object is pushed, the vector will reserve max 24 entries before moving the memory). This is to ensure an O(n) for reallocations over lifetime of the object.

SZenglein · 2023-12-12T19:50:53Z

src/Web.cpp

+			if (nvsIndex == 0) {
+				// start, write first tag
+				json = tagIdToJsonStr(nvsKeys[nvsIndex].c_str());
+				len += sprintf(((char *) buffer), "[%s", json.c_str());


What if maxlen < json len? Can we handle that case? We should make sure to never write more bytes to buffer than allowed.

Note: put the first tag also in the loop, then you won't forget the size check too

Good catch!
I never saw a buffer smaller than 1500 Bytes so JSON length never exceeds.
But i will do an additional check here for code safety.

Please always use snprintf instead of sprintf. sprintf is considered dangerous, since it does not check against buffer overflows. snprintf should always be preferred, since it takes a buffer size (here maxLen) and checks that we never write more than the buffer.

SZenglein · 2023-12-12T19:54:27Z

src/Web.cpp

+	nvsKeys.clear();
+	// Dumps all RFID-keys from NVS into key array
+	listNVSKeys("rfidTags", &nvsKeys, DumpNvsToArrayCallback);
+	if (nvsKeys.size() == 0) {


Would not need this special handling for size == 0 if in the chunked response there was no special handling for the first tag.

Writing the first tag there is no comma in JSON, it is starting with the second entry.
That's is the reason to handle the special case size==0 here and not even start a chunked response.

Not starting the chunked response in the first place is probably a good idea.

This looks ok for me, we do not start the chunked response here (just send back an empty array and return)

SZenglein · 2023-12-12T20:01:23Z

src/Web.cpp

+			}
+			if (nvsIndex == nvsKeys.size()) {
+				// finish
+				len += sprintf(((char *) buffer + len), "]");


Edge case: that single byte for ']' or ',' could be one byte more than maxLen.

Why not write '[' and ']' by themselves without sprintf?

+1 for not using sprintf but directly accessing the array

laszloh · 2023-12-13T11:54:43Z

src/Web.cpp

+		[](uint8_t *buffer, size_t maxLen, size_t index) {
+			maxLen = maxLen >> 1; // some sort of bug with actual size available, reduce the len
+			size_t len = 0;
+			static String json;


String json does not needs to be static.

Suggested change

static String json;

String json;

Here you can also use index to detect the start of the chunked response to reset the variable nvsIndex:

if (index==0) { // first call for us, reset index nvsIndex = 0; }

laszloh · 2023-12-13T11:58:10Z

src/Web.cpp

+			if (nvsIndex == 0) {
+				// start, write first tag
+				json = tagIdToJsonStr(nvsKeys[nvsIndex].c_str());
+				len += sprintf(((char *) buffer), "[%s", json.c_str());


Please always use snprintf instead of sprintf. sprintf is considered dangerous, since it does not check against buffer overflows. snprintf should always be preferred, since it takes a buffer size (here maxLen) and checks that we never write more than the buffer.

laszloh · 2023-12-13T12:00:47Z

src/Web.cpp

+	return serializedJsonString;
+}
+
+uint16_t nvsIndex;


Move nvsIndex either as a static variable into the lambda in line 1736 or at least make the variable static (so that it does not pollute the global namespace).

laszloh · 2023-12-13T12:36:24Z

src/Web.cpp

+	// construct chunked repsonse
+	nvsIndex = 0;
+	AsyncWebServerResponse *response = request->beginChunkedResponse("application/json",
+		[](uint8_t *buffer, size_t maxLen, size_t index) {


If nvsKeys is not static any more, we have to capture the variable here so it's available in the lambda. To prevent a copy of the vector due to capture by value, we call std::move to transfer the variable into the lambda (capture by reference would result in undefined behaviour since the local variable will not exists any more when we are called).

Suggested change

[](uint8_t *buffer, size_t maxLen, size_t index) {

[nvsKeys = std::move(nvsKeys)](uint8_t *buffer, size_t maxLen, size_t index) {

laszloh · 2023-12-13T12:38:19Z

src/Web.cpp

+	nvsKeys.clear();
+	// Dumps all RFID-keys from NVS into key array
+	listNVSKeys("rfidTags", &nvsKeys, DumpNvsToArrayCallback);
+	if (nvsKeys.size() == 0) {


This looks ok for me, we do not start the chunked response here (just send back an empty array and return)

laszloh · 2023-12-13T12:48:42Z

src/Web.cpp

+			}
+			if (nvsIndex == nvsKeys.size()) {
+				// finish
+				len += sprintf(((char *) buffer + len), "]");


+1 for not using sprintf but directly accessing the array

/rfid returns an array of tag-id keys /rfid/details returns an array of tag-ids and details. Optional GET param "id" to list only a single assignment.

SZenglein · 2023-12-13T21:56:38Z

src/Web.cpp

@@ -457,6 +457,7 @@ void webserverStart(void) {

 		// RFID
 		wServer.on("/rfid", HTTP_GET, handleGetRFIDRequest);
+		wServer.addRewrite(new OneParamRewrite("/rfid/details", "/rfid?details=true"));


OneParamRewrite is usually for path parameters in the form of /path/{some_param} -> /path?some_param={some_param} because AsyncWebServer cannot handle path params natively.

Does this even work and do something? Even if it does, using the "normal" AsyncWebRewrite makes more sense: https://github.com/me-no-dev/ESPAsyncWebServer#param-rewrite-with-matching

Yes it works fine, do you have a concrete improved code here?

Didn't test it, but just replacing 'OneParamRewrite' with 'AsyncWebRewrite' should do the same thing.

SZenglein · 2023-12-13T22:05:54Z

src/Web.cpp

-				json = tagIdToJsonStr(nvsKeys[nvsIndex].c_str());
-				len += sprintf(((char *) buffer), "[%s", json.c_str());
+				json = tagIdToJsonStr(nvsKeys[nvsIndex].c_str(), withDetails);
+				if (json.length() > maxLen) {


You check that, but 4 lines later you're writing json.length() + 1 bytes, so you should also check for json.length() + 1 available space

SZenglein · 2023-12-13T22:08:42Z

src/Web.cpp

 				nvsIndex++;
 			}
 			while (nvsIndex < nvsKeys.size()) {
 				// write tags as long we have enough room
-				json = tagIdToJsonStr(nvsKeys[nvsIndex].c_str());
+				json = tagIdToJsonStr(nvsKeys[nvsIndex].c_str(), withDetails);
 				if ((len + json.length()) > maxLen) {


Same issue, check is for 1 byte too few because of the comma

SZenglein · 2023-12-13T22:11:42Z

src/Web.cpp

 				nvsIndex++;
 			}
 			if (nvsIndex == nvsKeys.size()) {
 				// finish
-				len += sprintf(((char *) buffer + len), "]");
+				len += snprintf(((char *) buffer + len), maxLen, "]");


That byte is written without a length check at all.

SZenglein · 2023-12-13T22:16:39Z

Regarding the buffer copying, it's still calling for memory issues IMHO.
Most importantly, every code like this:

snprintf(((char *) buffer + len), maxLen, "...", ....);

Needs to be converted to

snprintf(((char *) buffer + len), maxLen-len, "...", ...);

that would already prevent any invalid memory access when writing the buffer. If the string doesn't fit, the json would still be invalid though.

laszloh · 2023-12-14T11:42:17Z

Needs to be converted to snprintf(((char *) buffer + len), maxLen-len, "...", ...);

Yes you are correct. It's probably easier if we introduce a local variable for the remaining capacity const size_t remaining = maxLen - len - 1; (-1 for the last ']').

If the string doesn't fit, the json would still be invalid though.

From my point of view still better than getting a buffer overflow, overwriting unknown memory blocks after the buffer and having at best an unhandled exception at the next malloc/free call. We can also check for an error from snprintf by (len < 0 || len >= remaining) and react accordingly (I'd need to look it up, if there is a possibility to abort a chunked response in ESPAsyncWebServer & return f.e. HTTP-Code 500).

endpoint /rfid/ids returns only tag ids buffer check

…mbda return type

laszloh

Looks good, I've added some changes to the code (mostly cosmetic). The code works as expected with entry count of over 300 dummy cards.

laszloh · 2023-12-18T10:40:50Z

src/Web.cpp

+		return;
+	}
+	// get tag details or just an array of id's
+	bool idsOnly = request->hasParam("ids");


See comment in line #460. Here we just grab the parameter

Suggested change

bool idsOnly = request->hasParam("ids");

const bool idsOnly = request->hasParam("ids-only");

laszloh · 2023-12-18T10:41:27Z

src/Web.cpp

@@ -457,6 +457,7 @@ void webserverStart(void) {

 		// RFID
 		wServer.on("/rfid", HTTP_GET, handleGetRFIDRequest);
+		wServer.addRewrite(new OneParamRewrite("/rfid/ids", "/rfid?ids=true"));


I propose, that the parameter ids should be called something like ids-only. We have a parapemeter id already, so a mixup could easily happen.

Suggested change

wServer.addRewrite(new OneParamRewrite("/rfid/ids", "/rfid?ids=true"));

wServer.addRewrite(new OneParamRewrite("/rfid/ids-only", "/rfid?ids-only=true"));

laszloh · 2023-12-18T10:46:43Z

src/Web.cpp

+			while (nvsIndex < nvsKeys.size()) {
+				// write tags as long we have enough room
+				json = tagIdToJsonStr(nvsKeys[nvsIndex].c_str(), idsOnly);
+				if ((len + json.length()) >= maxLen) {


This should be only >, maxLen is the length of the buffer, not an index.

Suggested change

if ((len + json.length()) >= maxLen) {

if ((len + json.length()) > maxLen) {

laszloh · 2023-12-18T10:52:07Z

src/Web.cpp

+			}
+			return len;
+		});
+	nvsKeys.clear();


I just saw, that we are accessing nvsKeys here. Since we called std::move on the variable, nvsKeys does not hold the array any more. As soons as the lambds is finished, the nvsKeys will go out of scope and it'll be destoryed.

Suggested change

nvsKeys.clear();

laszloh · 2023-12-18T10:57:27Z

src/Web.cpp

+		[nvsKeys = std::move(nvsKeys), idsOnly](uint8_t *buffer, size_t maxLen, size_t index) {
+			maxLen = maxLen >> 1; // some sort of bug with actual size available, reduce the len
+			size_t len = 0;
+			String json;


we can move nvsIndex into the lambda here. Also nvsIndex should have the same type as std::vector::size().

Suggested change

String json;

static size_t nvsIndex = 0;

String json;

laszloh · 2023-12-18T10:58:58Z

src/Web.cpp

+	bool idsOnly = request->hasParam("ids");
+
+	std::vector<String> nvsKeys {};
+	static uint16_t nvsIndex;


See comment on line 1747.

Suggested change

static uint16_t nvsIndex;

laszloh · 2023-12-18T10:59:46Z

src/Web.cpp

+
+	std::vector<String> nvsKeys {};
+	static uint16_t nvsIndex;
+	nvsKeys.clear();


Not needed since nvsKeys was just created (since it's not a static variable any more), so it's already empty.

Suggested change

nvsKeys.clear();

laszloh · 2023-12-18T11:08:57Z

src/Web.cpp

 	}
+	// construct chunked repsonse
+	nvsIndex = 0;


see 1747

Suggested change

nvsIndex = 0;

laszloh · 2023-12-18T12:00:22Z

src/Web.cpp

+				nvsIndex++;
+			}


We are finished here, so we can clean up here. (see comment on line 1775 regarding nvsKeys)

Suggested change

nvsIndex++;

}

nvsIndex++;

} else if (nvsIndex > nvsKeys.size()) {

nvsIndex = 0;

return 0;

}

laszloh · 2023-12-18T12:03:33Z

src/Web.cpp

+	// construct chunked repsonse
+	nvsIndex = 0;
+	AsyncWebServerResponse *response = request->beginChunkedResponse("application/json",
+		[nvsKeys = std::move(nvsKeys), idsOnly](uint8_t *buffer, size_t maxLen, size_t index) {


Fix the return type of the lambda to size_t here (so that we can write return 0).

Suggested change

[nvsKeys = std::move(nvsKeys), idsOnly](uint8_t *buffer, size_t maxLen, size_t index) {

[nvsKeys = std::move(nvsKeys), idsOnly](uint8_t *buffer, size_t maxLen, size_t index) -> size_t {

rfid chunked response

3e2b0f6

SZenglein reviewed Dec 12, 2023

View reviewed changes

laszloh reviewed Dec 13, 2023

View reviewed changes

tueddy added 2 commits December 13, 2023 21:25

fixes

788d4ff

/rfid returns an array of tag-id keys /rfid/details returns an array of tag-ids and details. Optional GET param "id" to list only a single assignment.

make clang check happy

0d9c7b3

SZenglein reviewed Dec 13, 2023

View reviewed changes

tueddy added 3 commits December 14, 2023 15:52

endpoint /rfid returns tag details (as before)

b080121

endpoint /rfid/ids returns only tag ids buffer check

clang

86fa4f4

fix error: inconsistent types 'int' and 'unsigned int' deduced for la…

871df02

…mbda return type

laszloh reviewed Dec 18, 2023

View reviewed changes

tueddy added 2 commits December 29, 2023 15:55

nvsIndex is size_t

403d2e5

ids->ids-only

9c3b512

tueddy merged commit a1a35c7 into biologist79:dev Dec 29, 2023
10 checks passed

	[](uint8_t *buffer, size_t maxLen, size_t index) {
	[nvsKeys = std::move(nvsKeys)](uint8_t *buffer, size_t maxLen, size_t index) {

	bool idsOnly = request->hasParam("ids");
	const bool idsOnly = request->hasParam("ids-only");

	wServer.addRewrite(new OneParamRewrite("/rfid/ids", "/rfid?ids=true"));
	wServer.addRewrite(new OneParamRewrite("/rfid/ids-only", "/rfid?ids-only=true"));

	if ((len + json.length()) >= maxLen) {
	if ((len + json.length()) > maxLen) {

	[nvsKeys = std::move(nvsKeys), idsOnly](uint8_t *buffer, size_t maxLen, size_t index) {
	[nvsKeys = std::move(nvsKeys), idsOnly](uint8_t *buffer, size_t maxLen, size_t index) -> size_t {

/rfid endpoint - Serve with chunked response #280

/rfid endpoint - Serve with chunked response #280

Conversation

tueddy commented Dec 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laszloh Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SZenglein commented Dec 13, 2023 • edited Loading

laszloh commented Dec 14, 2023

laszloh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laszloh Dec 13, 2023 •

edited

Loading

SZenglein commented Dec 13, 2023 •

edited

Loading