Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugfix: SrsMetaCache memleak; getaddrinfo use delete. #2880

Closed
wants to merge 5 commits into from
Closed

bugfix: SrsMetaCache memleak; getaddrinfo use delete. #2880

wants to merge 5 commits into from

Conversation

bluestn
Copy link
Contributor

@bluestn bluestn commented Jan 12, 2022

Summary

  1. SrsMetaCache memleak.
  2. use freeaddrinfo instread of delete to release addrinfo pointer.

@@ -171,29 +171,32 @@ srs_error_t srs_tcp_connect(string server, int port, srs_utime_t tm, srs_netfd_t
hints.ai_socktype = SOCK_STREAM;

addrinfo* r = NULL;
SrsAutoFree(addrinfo, r);
Copy link
Member

@winlinvip winlinvip Jan 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we use SrsAutoFree?

If you need to use free instead of delete for deallocation, you can use SrsAutoFreeF.

Automatic freeing can significantly improve code readability.

TRANS_BY_GPT3

@bluestn
Copy link
Contributor Author

bluestn commented Jan 13, 2022 via email

@winlinvip
Copy link
Member

winlinvip commented Jan 13, 2022

If it is not possible to release using free or delete, you must use freeaddrinfo to release. In that case, it can be changed like this:

string do_srs_dns_resolve(string host, int& family, addrinfo*& r)
{
    addrinfo hints;
    memset(&hints, 0, sizeof(hints));
    hints.ai_family = family;

    if(getaddrinfo(host.c_str(), NULL, &hints, &r)) {
        return "";
    }
    
    char shost[64];
    memset(shost, 0, sizeof(shost));
    if (getnameinfo(r->ai_addr, r->ai_addrlen, shost, sizeof(shost), NULL, 0, NI_NUMERICHOST)) {
        return "";
    }

   family = r->ai_family;
   return string(shost);
}

string srs_dns_resolve(string host, int& family)
{
    addrinfo* r = NULL;
    string r0 = do_srs_dns_resolve(host, family, r);
    freeaddrinfo(r);

    return r0;
}

Transform the resource allocation and handling into a sub-function, and then release it once at the calling location.

If you find it troublesome, you can use the goto method in C, you can refer to FFmpeg for specific details.

TRANS_BY_GPT3

@winlinvip
Copy link
Member

winlinvip commented Jan 13, 2022

There is another way, to improve SrsAutoFree, allowing to pass a release function into it.

TRANS_BY_GPT3

@winlinvip
Copy link
Member

winlinvip commented Jan 13, 2022

I add a macro SrsAutoFreeH to allow release with hook: 3881c4c

    addrinfo* r = NULL;
    SrsAutoFreeH(addrinfo, r, freeaddrinfo);

Please use this macro to free the addrinfo, does it OK?

@winlinvip winlinvip self-assigned this Jan 13, 2022
@winlinvip
Copy link
Member

winlinvip commented Jan 17, 2022

@bluestn Could you please make the changes according to the new modifications? Can you also merge the 4.0 release branch?

TRANS_BY_GPT3

@winlinvip
Copy link
Member

winlinvip commented Jan 17, 2022

@bluestn If you don't know how to operate git, you can contact me on WeChat.

There is no need to resubmit a new PR, you can continuously update this PR.

TRANS_BY_GPT3

@codecov-commenter
Copy link

codecov-commenter commented Jan 22, 2022

Codecov Report

Merging #2880 (416eb02) into 4.0release (dc43a11) will decrease coverage by 0.00%.
The diff coverage is 75.00%.

Impacted file tree graph

@@              Coverage Diff               @@
##           4.0release    #2880      +/-   ##
==============================================
- Coverage       60.22%   60.21%   -0.01%     
==============================================
  Files             121      121              
  Lines           51076    51078       +2     
==============================================
  Hits            30758    30758              
- Misses          20318    20320       +2     

| Impacted Files | Coverage Δ | |'

Translated to English while maintaining the markdown structure:

'| Impacted Files | Coverage Δ | |
|---|---|---|
| trunk/src/app/srs_app_source.cpp | 0.66% <0.00%> (-0.01%) | ⬇️ |
| trunk/src/kernel/srs_kernel_utility.cpp | 92.37% <100.00%> (ø) | |
| trunk/src/protocol/srs_service_st.cpp | 78.38% <100.00%> (ø) | |
| trunk/src/core/srs_core_autofree.hpp | 93.33% <0.00%> (ø) | |

'

Translated to English while maintaining the markdown structure:

'| trunk/src/kernel/srs_kernel_utility.cpp | 92.37% <100.00%> (ø) | |
| trunk/src/protocol/srs_service_st.cpp | 78.38% <100.00%> (ø) | |
| trunk/src/core/srs_core_autofree.hpp | 93.33% <0.00%> (ø) | |


Continue to review full report at Codecov.

Legend - Click here to learn more
| Δ = absolute <relative> (impact), ø = not affected, ? = missing data'

Translated to English while maintaining the markdown structure:

'| Δ = absolute <relative> (impact), ø = not affected, ? = missing data

Powered by Codecov. Last update dc43a11...416eb02. Read the comment docs.

TRANS_BY_GPT3

@bluestn bluestn closed this Jan 22, 2022
@winlinvip winlinvip added the TransByAI Translated by AI/GPT. label Jul 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TransByAI Translated by AI/GPT.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants