possible resource leak - Variable res going out of scope leaks the storage it points to. The Static Analysis tool Coverity added, ice_unittest.cpp::120

Comment on attachment 8734296 [details] [diff] [review] bug_1259369.diff Review of attachment 8734296 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/mtransport/test/ice_unittest.cpp @@ +115,5 @@ > > if (!strlen(str_addr)) { > std::cerr << "inet_ntop failed" << std::endl; > } > + free(res); This is not correct. See the man page for getaddrinfo. All of the information returned by getaddrinfo() is dynamically allocated: the addrinfo structures themselves as well as the socket address structures and the canonical host name strings included in the addrinfo structures. Memory allocated for the dynamically allocated structures created by a successful call to getaddrinfo() is released by the freeaddrinfo() function. The ai pointer should be an addrinfo structure created by a call to getaddrinfo().

thank you, I've changed the call from free to freeaddrinfo, see the second attachement

Comment on attachment 8734298 [details] [diff] [review] bug_1259369.diff Review of attachment 8734298 [details] [diff] [review]: ----------------------------------------------------------------- r+ with one change. ::: media/mtransport/test/ice_unittest.cpp @@ +111,3 @@ > std::cerr << "Got unexpected address family in DNS lookup: " > << res->ai_family << std::endl; > return ""; Let's remove this return; this will allow |res| to be cleaned up in this case, and doesn't significantly differ in behavior.

changes added

Comment on attachment 8734340 [details] [diff] [review] bug_1259369.diff Review of attachment 8734340 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/mtransport/test/ice_unittest.cpp @@ -108,5 @@ > sizeof(str_addr)); > break; > - default: > - std::cerr << "Got unexpected address family in DNS lookup: " > - << res->ai_family << std::endl; Leave the default case in; we would like to have this logging. Just don't return.

Comment on attachment 8734345 [details] [diff] [review] bug_1259369.diff Review of attachment 8734345 [details] [diff] [review]: ----------------------------------------------------------------- LGTM

Radu, is this patch ready to land?

I can land it

(In reply to Andi-Bogdan Postelnicu from comment #13) > > *** This bug has been marked as a duplicate of bug 1268449 *** do you want to still land the patch here? The patch from Bug 1268449 seems has landed ?

No, the patch from bug 1268449 covers it.