Fix memory leaks from errors skipping resource release #4308
Conversation
| // free pkts when exit | ||
| vector<SrsRtpPacket*>* pkts_ptr = &pkts; | ||
| SrsAutoFreeH(vector<SrsRtpPacket*>, pkts_ptr, free_packets); | ||
|
|
||
| if (merge_nalus && nn_samples > 1) { | ||
| if ((err = package_nalus(msg, samples, pkts)) != srs_success) { | ||
| return srs_error_wrap(err, "package nalus as one"); |
There was a problem hiding this comment.
When an error occurs, the SrsRtpPacket within the vector is not released, which could potentially lead to a memory leak.
TRANS_BY_GPT4
duiniuluantanqin
requested review from
suzp1984,
xiaozhihong and
chundonglinlin
|
|
||
| // free pkts when exit | ||
| vector<SrsRtpPacket*>* pkts_ptr = &pkts; | ||
| SrsAutoFreeH(vector<SrsRtpPacket*>, pkts_ptr, free_packets); |
There was a problem hiding this comment.
Yes, this is a memory leak.
But, SrsAutoFreeH is already deprecated, the recommended smart pointer is SrsSharedPtr and SrsUniquePtr. And in this case, vector<SrsSharedPtr<SrsRtpPacket>> seems can replace SrsAutoFreeH here, the package_nalus, package_single_nalue, package_fu_a & consume_packets need a little patch then.
Oh, I do think vector<SrsSharedPtr<SrsRtpPacket>> is better here.
There was a problem hiding this comment.
Converting to smart pointers is feasible, but it would affect several functions. The functions consume_packets, package_fu_a, package_single_nalu, and package_nalus would all require modifications.
TRANS_BY_GPT4
There was a problem hiding this comment.
Alternatively, refine the SrsSharedPtr to support a custom deleter.
TRANS_BY_GPT4
There was a problem hiding this comment.
No custom deleter here, I do think only tiny effort is needed to do the replacement.
e.g.
srs_error_t SrsRtcRtpBuilder::package_nalus(SrsSharedPtrMessage* msg, const vector<SrsSample*>& samples, vector<SrsSharedPtr<SrsRtpPacket>& pkts)
{
...
if (nn_bytes < kRtpMaxPayloadSize) {
SrsSharedPtr<SrsRtpPacket> pkt(new SrsRtpPacket());
pkts.push_back(pkt);
...
}
...
}
srs_error_t SrsRtcRtpBuilder::consume_packets(vector<SrsSharedPtr<SrsRtpPacket>>& pkts)
{
srs_error_t err = srs_success;
// TODO: FIXME: Consume a range of packets.
for (int i = 0; i < (int)pkts.size(); i++) {
SrsSharedPtr<SrsRtpPacket> pkt = pkts[i];
if ((err = bridge_->on_rtp(pkt.get())) != srs_success) {
err = srs_error_wrap(err, "consume sps/pps");
break;
}
}
return err;
}
There was a problem hiding this comment.
SrsAutoFreeH is deprecated, but it can still be used, especially no alternative there. A better solution is to support a custom free handler in SrsUniquePtr to replace SrsAutoFreeH. SrsSharedPtr is not ideal here because it's a hot path and may cause a performance decline. See #4309
There was a problem hiding this comment.
SrsAutoFreeH is faster than SrsSharedPtr. OK, I accept this view.
|
|
||
| // free pkts when exit | ||
| vector<SrsRtpPacket*>* pkts_ptr = &pkts; | ||
| SrsAutoFreeH(vector<SrsRtpPacket*>, pkts_ptr, free_packets); |
There was a problem hiding this comment.
SrsAutoFreeH is faster than SrsSharedPtr. OK, I accept this view.
winlinvip
changed the title
winlinvip
changed the title
No description provided.