Stop ignoring real buffers shrunk to 0 size

[celskeggs]

Related Issue(s)	n/a
Has Unit Tests (y/n)	n
Documentation Included (y/n)	n

---

Change Description

If BufferManager is asked to allocate a buffer when it has no more buffers, it will return a null, zero-size buffer. Therefore, if BufferManager receives a request to deallocate a zero-sized buffer, it discards the buffer, on the assumption that it resulted from a failed allocation.

However, if a user allocates a buffer from BufferManager, they are allowed to decrease the size before they deallocate it. Therefore, a zero size buffer passed to the deallocation function cannot necessarily be discarded; it could be a real buffer that needs to be freed. Only discard deallocated buffers if they are empty and null.

Rationale

Without this fix, trying to return allocated buffers to a BufferManager with sizes updated to 0 can result in a memory leak.

Testing/Review Recommendations

No specific recommendations.

Future Work

No future work.

[LeStarch]

My first thought was to recommend using not buffer.isValid(); to check for invalid buffers to skip. However, to be valid according to the function, a buffer must be not null, and not zero-sized.

fprime/Fw/Buffer/Buffer.cpp

Lines 64 to 66 in e09462f

	bool Buffer::isValid() const {
	return (this->m_bufferData != nullptr) && (this->m_size > 0);
	}

I agree with you fix, because it is absolutely legal to shrink a buffer (even to zero) and still have it be deallocated. My question is, should the isValid buffer method be updated to reflect this?

@bocchino you added this Fw::Buffer::isValid method originally (if my memory holds). Was there a reason to consider a buffer valid only if it has positive size? Why not just check to see if the memory pointer is null

[LeStarch]

This would be a good issue to put into a Unit-Test to prevent regressions.

[LeStarch]

Fix looks solid.

[bocchino]

Was there a reason to consider a buffer valid only if it has positive size?

I think a buffer with a zero size and a non-null pointer is invalid. It can't be used to store any data, any more than a buffer with a positive size and a null pointer.

It seems like we are now using the pointer value to track side-band information of the form "this buffer was valid when the buffer manager gave it out, but it was made invalid by shrinking its size to zero" as opposed to "this buffer was invalid when the buffer manager gave it out." The size-shrinking and consequent valid-to-invalid transformation were not part of the original design.

I've never been entirely comfortable with the size-shrinking pattern, but I think it is now ingrained in F Prime, because there's no field in the buffer class for storing how much of the original allocation is used. In hindsight it might have been better to have two fields: one for the original allocation, and one for how much is used. But given how buffers currently work, the solution proposed in this PR seems good.

[bocchino]

Should we rename this event to NullEmptyBuffer instead of ZeroSizeBuffer? Or maybe just NullBuffer?

[bocchino]

It looks like the current BufferManager implementation allows a zero-size buffer allocation. Allocating and returning such a buffer would also cause a memory leak in the old code (though it's an unlikely use case). Checking for a null pointer instead of a zero size fixes this issue as well.

[bocchino]

It's not clear to me that the BufferManager should allow allocations of zero-size buffers, which are invalid, but it does.

[celskeggs]

Allowing zero-sized allocations is a strange edge case, but it can make some tests simpler for us to allow it. (I caught this bug in a test where I randomly generated buffers of length 0 to N, and occasionally it landed on 0.) I don't see any reason to forbid them outright in a core framework component. I agree that this PR also fixes that issue.