bblanchon / ArduinoJson

📟 JSON library for Arduino and embedded C++. Simple and efficient.
https://arduinojson.org
MIT License
6.7k stars 1.12k forks source link

[v6] Buffer size calculation issue #1077

Closed mkfrey closed 5 years ago

mkfrey commented 5 years ago

While trying to migrate the Homie framework to v6 I noticed that the size calculations don't seem to work any more. JSON generated by Homie now lacks the last object which was added and JSON parsing fails due to the buffer being full.

Adding another JSON_OBJECT_SIZE(1) to the previously calculated sizes gets rid of the problems, but according to the Assistant the current calculations should still be sufficient.

Here you can see an example of code that has now stopped working. Using the JSON {"enable":true} the function will now return ✖ Invalid or too big JSON.

Pull request where the issue was discovered: https://github.com/homieiot/homie-esp8266/pull/618

bblanchon commented 5 years ago

Hi @codefrey,

When calling deserializeJson() with a read-only input, you must increase the capacity of the JsonDocument because it needs to duplicate the strings from the input ("enable" in your case).

This code used to work with version 5, because DynamicJsonBuffer was able to grow automatically.

Note that the ArduinoJson Assistant includes these extra bytes in the parsing program:

const size_t capacity = JSON_OBJECT_SIZE(1) + 10;
DynamicJsonDocument doc(capacity);
const char* json = "{\"enable\":true}";
deserializeJson(doc, json);
bool enable = doc["enable"]; // true

However, for such small documents, I recommend that you use a StaticJsonDocument instead of a DynamicJsonDocument because it avoids heap allocation.

BTW, since you are in the process of upgrading Homie, there is a lot of clean up to do in Config.cpp. In particular, the following pattern:

const char* reqWifiIp = "";
if (parsedJson["wifi"].as<JsonObject&>().containsKey("ip")) {
  reqWifiIp = parsedJson["wifi"]["ip"];
}
strlcpy(_configStruct.wifi.ip, reqWifiIp, MAX_IP_STRING_LENGTH);

can be reduced to:

strlcpy(_configStruct.wifi.ip, parsedJson["wifi"]["ip"] | "", MAX_IP_STRING_LENGTH);

Best Regards, Benoit

mkfrey commented 5 years ago

Thank you very much for your answer. After migrating the code to v6, we also planned to optimize it, but I haven't thought of using the operator|, so this is very useful information.

Is there a way to calculate how much space would be needed for copying/complete parsing in case I want to parse a bigger JSON with changing content?

And thank you for your great library! It seems like v6 unveils a few wrong assumptions about the memory allocations made in Homie.

bblanchon commented 5 years ago

Is there a way to calculate how much space would be needed for copying/complete parsing in case I want to parse a bigger JSON with changing content?

I recommend that you put the largest possible input in the Assistant, and that will give you the upper bound.

It seems like v6 unveils a few wrong assumptions about the memory allocations made in Homie.

Finally! Someone gets it right :-)