Fix stack overflow and memory leak when running tests #1120
Conversation
|
It makes sense. Just let's us review this during this week, please. |
| @@ -502,6 +503,7 @@ SCENARIO("parse RTP packets", "[parser][rtp]") | |||
| 0x00, 0x00, 0x00, 0x00, | |||
| 0x00, 0x00, 0x00, 0x00, | |||
| 0x00, 0x00, 0x00, 0x00, | |||
| 0x00, 0x00, 0x00, 0x00, | |||
There was a problem hiding this comment.
Might have been beneficial to expand comment on why extra buffer is needed and maybe have buffer size be a fixed size array calculate statically, otherwise this seems to be somewhat accidental that it works at all. In other tests you've set buffer to 1500 bytes, but not here.
There was a problem hiding this comment.
Totally agree, but I don't know how the buffer size is calculated.
There was a problem hiding this comment.
In other tests you've set buffer to 1500 bytes, but not here.
Yes, let's be consistent and just define the buffer length rather than adding this extra zeroes please.
| @@ -28,15 +28,15 @@ class RtpMyRetransmissionBuffer : public RtpRetransmissionBuffer | |||
| void Insert(uint16_t seq, uint32_t timestamp) | |||
| { | |||
| // clang-format off | |||
| uint8_t rtpBuffer[] = | |||
| uint8_t rtpBuffer[1500] = | |||
There was a problem hiding this comment.
Why does it need to be this large, is there a way to calculate it from some constant rather than using a magic constant here?
worker/test/src/RTC/RTCP/TestXr.cpp
Outdated
| @@ -57,7 +57,7 @@ SCENARIO("RTCP XrDelaySinceLastRt parsing", "[parser][rtcp][xr-dlrr]") | |||
| packet1->Serialize(bufferPacket1); | |||
|
|
|||
| // Create a new packet out of the external buffer. | |||
| auto packet2 = ExtendedReportPacket::Parse(bufferPacket1, packet1->GetSize()); | |||
| auto packet2 = std::unique_ptr<ExtendedReportPacket>(ExtendedReportPacket::Parse(bufferPacket1, packet1->GetSize())); | |||
There was a problem hiding this comment.
So Parse returns heap-allocated data structure. I think we should change auto to auto* to make that clearer and just call delete at the end for consistency.
We can also use std::unique_ptr, which I guess will be beneficial due to not leaking memory even when test fails, but then we might want to use it in all other tests too consistently.
| @@ -665,6 +667,8 @@ SCENARIO("parse RTP packets", "[parser][rtp]") | |||
| 0x00, 0x00, 0x00, 0x00, | |||
| 0x00, 0x00, 0x00, 0x00, | |||
| 0x00, 0x00, 0x00, 0x00, | |||
| 0x00, 0x00, 0x00, 0x00, | |||
There was a problem hiding this comment.
same here, please define the correct length to the array rather than adding this extra zeroes.
|
Duplicated with #1318 |
I didn't know how to find the buffer size of the packet, so I added free buffer as I did for the other tests.