Heap corruption issue

[garygumdrops]

Hi

I've seen some heap corruption on our embedded platform inside the parser function with large-ish strings (around 6k chars).

The platform is on an iMX.RT1050 using GCC and the newlib C library. We do use FreeRTOS as well but I've tried removing that. I saw the problem when I was using the newlib library for malloc/free and also seen it when using the heap allocation routines provided by FreeRTOS. I have been using the latter more recently because I have a modified version of heap4.c that includes the use of canaries around the allocated blocks to detect buffer overruns.

It's repeatable on my platform but hard to track where the actual problem is. With the modified heap allocation I can see the area of memory that is getting overwritten and when I trace back to where that memory is being free'd, it occurs in the destroy function of basic_json, specifically line 1041 (I am using version 3.7.3). This is a while loop emptying a stack. This area of the code may be a red herring, it's difficult to pinpoint where the issue is.

[garygumdrops]

Just a quick update before I disappear for the weekend :-)

I commented out the entire contents of the destroy function and I don't see any issues even after lots of parsing. I do obviously see memory leaks though after each pass.

[nickaein]

I imagine you are referring to this line:

json/include/nlohmann/json.hpp

Line 1041 in 973c52d

current_item.m_value.object->clear();

Can you comment it out to see if does have any effect? Clearing a move vector is safe but I know sometimes the embedded compilers could have some quirks.

Another test would be to comment out the code from the start of destroy() function until switch (t) (line 1001 to 1046, which are basically the changes introduced in this PR #1436. You are fine without them if your object is not deeply nested) and see if you still experience heap corruption or memory leaks?

Also, are you sure that the program doesn't run out of memory? Note that memory requirement for a parsed JSON object can sometimes be higher than JSON contents itself (#1613).

[garygumdrops]

Hi, thanks for the reply. I did try commenting out just that line and it caused problems further up in the *destroy *function (I can't remember what exactly). I shall try commenting out the section you suggested in destroy first thing on Monday. I did comment out the whole function and that was good in that there wasn't any heap corruption, it just meant there were expected memory leaks. I did get some compiler warning messages about the moves that I dutifully ignored until recently about the GCC semantics changing since version 7.1. I have checked the version of GCC that I'm using as part of the toolchain and it's 7.3.1 so I have put the pragma in to remove that warning as it is a bit noisy, so hard to see the wood for the trees. Maybe I should have another look at that and see if there's a something about the moves that the compiler isn't happy about. Maybe if there was a simple--ish change that could be made that was less efficient method, it would stop the corruption. I'm quite confident the system isn't running out of memory. It has 4MB and the problem happens while it still has over 3MB free space and I've checked that the heap isn't corrupted (there was a single free block of over 3MB). I don't mind the parsing being a bit memory hungry, it's to be expected considering the work it's doing and I only have to parse JSON strings in chunks, the largest of which will hopefully only be around 6KB. Unfortunately they can be quite nested though with arrays of objects etc. I've pasted in a string with an example which I hope is as big as I have to encounter. I'll report back on Monday after I've tried again. Gary

…

On Fri, 7 Feb 2020 at 18:25, Isaac Nickaein ***@***.***> wrote: I imagine you are referring to this line: https://github.com/nlohmann/json/blob/973c52dd4a9e92ede5ca4afe8b3d01ef677dc57f/include/nlohmann/json.hpp#L1041 Can you comment it out to see if does have any effect? Clearing a move vector is safe <https://stackoverflow.com/a/9168917/161640> but I know sometimes the embedded compilers could have some quirks. Another test would be to comment out the code from the start of destroy() function until switch (t) (line 1001 to 1046, which are basically the changes introduced in this PR #1436 <#1436>. You are fine without them if your object is not deeply nested) and see if you still experience heap corruption or memory leaks? Also, are you sure that the program doesn't run out of memory? Note that memory requirement for a parsed JSON object can sometimes be higher than JSON contents itself (#1613 <#1613> ). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1929?email_source=notifications&email_token=ADEYP5OSABK4OH45RXT3TY3RBWRTDA5CNFSM4KRPISI2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELECDRA#issuecomment-583541188>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADEYP5MGNP65X5YVQLX3DNDRBWRTDANCNFSM4KRPISIQ> .

[garygumdrops]

Hiya I did try commenting out the single line and it still causes heap corruption as it does if I comment out the code up until the switch statement as suggested. It was only when I commented out the whole function that it stopped any corruption, at the expense of quite large memory leaks. Gary.

…

On Fri, 7 Feb 2020 at 18:25, Isaac Nickaein ***@***.***> wrote: I imagine you are referring to this line: https://github.com/nlohmann/json/blob/973c52dd4a9e92ede5ca4afe8b3d01ef677dc57f/include/nlohmann/json.hpp#L1041 Can you comment it out to see if does have any effect? Clearing a move vector is safe <https://stackoverflow.com/a/9168917/161640> but I know sometimes the embedded compilers could have some quirks. Another test would be to comment out the code from the start of destroy() function until switch (t) (line 1001 to 1046, which are basically the changes introduced in this PR #1436 <#1436>. You are fine without them if your object is not deeply nested) and see if you still experience heap corruption or memory leaks? Also, are you sure that the program doesn't run out of memory? Note that memory requirement for a parsed JSON object can sometimes be higher than JSON contents itself (#1613 <#1613> ). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1929?email_source=notifications&email_token=ADEYP5OSABK4OH45RXT3TY3RBWRTDA5CNFSM4KRPISI2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELECDRA#issuecomment-583541188>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADEYP5MGNP65X5YVQLX3DNDRBWRTDANCNFSM4KRPISIQ> .

[nickaein]

There might be a memory corruption somewhere else in the program which remains undetected and causes crash sometime later during execution, e.g. when calling free()/delete.

Can you run the code ASAN (and TSAN if you suspect a race condition) to get more diagnostics? I'm not sure how well they support ARM architecture though, you might have to build and run the code on x86/64.

[garygumdrops]

Hi I did briefly look at ASAN but I think it would be quite a lot of work to get it working on a bare metal platform such as mine. It's undoubtedly got more bells and whistles than what I have now but it's basically doing the same thing of wrapping malloc/free and keeping a record of heap allocations. I would love the bug to be in my application code somewhere as it would be easy to fix, but after a week debugging this, I don't think it is. I can run all my code except for a single line that creates a JSON object using *parse *and it will run for days without leaks or corruption. I was more than willing to accept it was in my processing of the parsed JSON object which is very lengthy and uses iterators etc, but I still see the heap corruption if I just parse the string and don't do anything with the object. It doesn't happen on all strings, so maybe there is something about the string I am parsing. It's a valid JSON string but it's quite nested. Is it worth trying another mod to the destroy function? It is true that the corruption occurs after *free *is called inside *destroy *because I'm running the heap verification function inside free to catch the buffer overrun as soon as possible. I'd like to be able to say off hand what object it was that allocates the memory that eventually gets overwritten, but I can't remember. Maybe that's something I try tomorrow (Tuesday).

…

On Mon, 10 Feb 2020 at 14:39, Isaac Nickaein ***@***.***> wrote: There might be a memory corruption somewhere else in the program which remains undetected and causes crash sometime later during execution, e.g. when calling free()/delete. Can you run the code ASAN <https://github.com/google/sanitizers/wiki/AddressSanitizer> (and TSAN <https://github.com/google/sanitizers/wiki/ThreadSanitizerCppManual> if you suspect a race condition) to get more diagnostics? I'm not sure how well they support ARM architecture though, you might have to build and run the code on x86/64. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1929?email_source=notifications&email_token=ADEYP5LMK65NG523TTPC6W3RCFRKHA5CNFSM4KRPISI2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELIXUZQ#issuecomment-584153702>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADEYP5IAF45GQ6LALUWB7PDRCFRKHANCNFSM4KRPISIQ> .

[garygumdrops]

So I've played around a bit more this morning and if leave the first half of the destroy function untouched but comment out lines 1048 to 1078 (the switch statement) then I have no heap corruption, but just memory leaks, which is expected.

My C++ knowledge is a bit weak regarding the use of those std::allocator_trait objects, is there maybe another way I could destroy the top-level t object? I guess the fact that the while loop is running with no problems, the flattened object created in the stack local variable (and current_item as it walks through the stack) must be all cleaned up nicely. The leaked memory is about 9K per string that I'm parsing, so it doesn't take long for the heap to start noticeably going down.

[gary-metalle-rvl]

I did further reduce the memory leaks by just commenting out lines 1053, 1061 and 1069 but leaving the deallocates in.

[nickaein]

Using ASAN/TSAN on non-x86/64 platforms can indeed be very cumbersome, if not impossible. What we did was to run the code on x86 platform (and simulate all hardware dependencies) for testing. Also, building the program in debug mode and using gdb debugger can be helpful.

I don't think commenting out different parts of destroy() function can lead to something helpful as most of the code there is required otherwise you will have memory leaks. Even ignoring the memory leaks, the program still has memory corruption which can lead to incorrect behavior.

The memory corruption is either a bug in the program itself (buffer overrun, double-free, etc) or some side-effects like stack overflow. Here are a few examples of such bugs which the program crashed not at the problematic code, but after awhile during deconstruction/free:

https://stackoverflow.com/questions/11726746
https://stackoverflow.com/questions/3945167
https://stackoverflow.com/questions/16119762
https://stackoverflow.com/questions/27230890

[gary-metalle-rvl]

I'm 90% sure it's not a corruption issue with the main program because I can run it for days with no leaks or corruption. I have one major line of interest in a try/catch block:

           try
            {
                json object = json::parse(candidate); // candidate is an std::string

                if (callback)
                {
                    callback(object);
                }
            }
            catch (detail::parse_error& ex)
            {
                LOG_WARN("JSON parse error: %s.", ex.what());
            }

If I comment out the contents of the try block then I can run indefinitely, but if I uncomment the single line that parses a string into a json object (but leave out the code that calls the callback function that does the processing) then I get heap problems. So that's just by creating a json object from a string and not using it. There is plenty of stack and heap at the time the call to parse is called.

I'd be willing to accept it's something iffy with the compiler version I'm using, but I'm not sure how much work would be involved to try a newer release of GCC, especially given it might not be the problem.

[nickaein]

You might then record the content of each candidates before parsing (e.g. dumping to a file or sending it over network) so that the bug can be reproduced easier.

[garygumdrops]

Hiya I have done that yeah. Well what I actually did was create some static std::strings with those values. Normally these strings would come in pieces over a USB interface but because that just complicates things, I bypassed the USB and hardcoded the strings in the code and passed them in manually to the parse function. The first string (very small) always parsed with no problems, I only saw heap corruption with the second string which is larger and nested. Reproducing is quite easy even if I do things the natural way and have the strings come in over USB, it's just difficult to take it further because all I can see is that the corruption occurs during destroy(). I can tell this because the heap integrity check runs after each free() and it knows there has been a buffer overrun for memory that was allocated during the json::parse() call (or maybe within the destroy() call itself). I did include the strings a few days back. There are three in a row and I think the problem used to occur with the second string which is 334 bytes long. If I get time I'll create a vanilla x86 project on Windows or Linux that parses those strings and see what happens. I'd be surprised if it showed any problems though, the testing stuff for the JSON library looks pretty good.

…

On Wed, 12 Feb 2020 at 12:22, Isaac Nickaein ***@***.***> wrote: You might then record the content of each candidates before parsing (e.g. dumping to a file or sending it over network) so that the bug can be reproduced easier. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1929?email_source=notifications&email_token=ADEYP5KOAZKZKUYCSQRPHDDRCPSXVA5CNFSM4KRPISI2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELQSLKI#issuecomment-585180585>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADEYP5MOKFNSTNV4CC4JKJTRCPSXVANCNFSM4KRPISIQ> .

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.