Update unity_fixture.c

[bryonglodencissp]

[../Unity-master/extras/fixture/src/unity_fixture.c:210]: (error) Memory leak: guard

Found by https://github.com/bryongloden/cppcheck

[mvandervoord]

This submission fails the automated self-tests. I suspect it's because it is only valid #ifndef UNITY_EXCLUDE_STDLIB_MALLOC?

[bryonglodencissp]

Nope! My tool is reporting the error when no tokens have been defined (but not when UNITY_EXCLUDE_STDLIB_MALLOC is defined) i.e.

Checking ../Unity-master/extras/fixture/src/unity_fixture.c...
[../Unity-master/extras/fixture/src/unity_fixture.c:210]: (error) Memory leak: guard
Checking ../Unity-master/extras/fixture/src/unity_fixture.c: UNITY_EXCLUDE_STDLIB_MALLOC...
19/47 files checked 17% done

I suspect the error is related to the command "make -s default noStdlibMalloc" exited with 2. But because I'm unfamiliar with this command, do I close the PR?

[bryonglodencissp]

LOL. I believe you now @mvandervoord! It's related to UNITY_EXCLUDE_STDLIB_MALLOC 😁. The answer was just hidden inside the Makefile.

[bryonglodencissp]

Hi @mvandervoord ! I believe I edited unity_fixture.c correctly in 0737b41 (by including #ifndef around my patch code). However, the new submission failed the automated self-tests for presumably the same reason. Any more hints before I close this and #206?

[mvandervoord]

It seems like it is disagreeing with your change altogether. The error you are getting at this point is "*** glibc detected *** build/framework_test.exe: double free or corruption (fasttop): 0x0000000001444010 **"... I *think this happens when release_memory is called, because you are now removing the guard object before then.

Why is it that you thought this was necessary? What situation are you covering for?

[bryonglodencissp]

Hi @mvandervoord. Thank you for looking over this. We've done studies you know; 60% of the time cppcheck works all the time 👍.

Here we think it's necessary to free() memory because we believe its output. And here Steve Jessop enumerates the lists of situations we're 'covering' for (he even includes some situations we're not covering for 👍).

If you want/need a specific answer detailing the reasons our program detected an error in your code, then I suspect it'll have to wait until after dinner (at the very least 👍).

Although, if you're confident in your implementation of memory management, then just close this and PR #206 and chalk it all up as volunteerism 😁

[mvandervoord]

I believe your tool has identified a problem as I look through this... I think your solution is wrong, though. I could be mistaken (this isn't actually my code... this file is an addon submitted by a third party. It's used by roughly a quarter of the Unity users, though, so I'd love to fix it up).

Anyway, your solution frees the memory before it's even called upon to be used. That's why you're running into an error about double-freeing memory.

In the most common case, I think the current system works. Memory is allocated in unity_malloc and it is freed again in unity_free. However, part of the point of this subsystem is to catch errors that people make, like not calling free when they should... and now that I am staring at it, I don't actually SEE a place where it checks to see if anyone ever came back and cleared that memory!

I think this is probably intended to happen somewhere in UnityConcludeFixtureTest?

Let me know what you think after dinner. ;)

[bryonglodencissp]

Commits b1d8388 and 0737b41 fixed with 4fd5f79, thank you @mvandervoord

[jsalling]

Oops. This tool is suggesting to free guard, but it should never be freed during unity_malloc.
The guard has the job of, well, guarding the malloc'd memory passed to the caller. It's also how the implementation frees the total memory.

I'm going to revert this commit in another PR. The preprocessor conditional is never going to be true, so this is dead code, which is why it works.

Quote:

However, part of the point of this subsystem is to catch errors that people make, like not calling free when they should... and now that I am staring at it, I don't actually SEE a place where it checks to see if anyone ever came back and cleared that memory!
I think this is probably intended to happen somewhere in UnityConcludeFixtureTest?

@mvandervoord This happens in the UnityTestRunner. Each test that uses this part of the fixture must free for each malloc, calloc, or realloc.

[bryonglodencissp]

Yes, unfortunately, sound and complete static analysis is shown to be undecidable. We're led to this false positive because we've adopted some unsound techniques in our tool.