<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 05/12/2014 01:57 PM, Wangrui (K)
wrote:<br>
</div>
<blockquote
cite="mid:92984A2DD667AD43AADA1E474DC5E5965C0A9C56@szxema505-mbx.china.huawei.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1">
<meta name="Generator" content="Microsoft Word 12 (filtered
medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:SimSun;
panose-1:2 1 6 0 3 1 1 1 1 1;}
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:SimSun;
panose-1:2 1 6 0 3 1 1 1 1 1;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
text-align:justify;
text-justify:inter-ideograph;
font-size:10.5pt;
font-family:"Calibri","sans-serif";}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
span.EmailStyle17
{mso-style-type:personal-compose;
font-family:"Calibri","sans-serif";
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;}
/* Page Definitions */
@page WordSection1
{size:612.0pt 792.0pt;
margin:72.0pt 90.0pt 72.0pt 90.0pt;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
<div class="WordSection1">
<p class="MsoNormal"><span lang="EN-US">1.<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">As shown in the
definition of nl_recv, its return value is of INT type.<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">int nl_recv(struct
nl_handle *handle, struct sockaddr_nl *nla,<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"> unsigned
char **buf, struct ucred **creds)<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">However, in function
virNetlinkCommand, it uses an unsigned int param,<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">respbuflen, to receive
its return value. Thus, the branch "*respbuflen < 0"<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">is unreachable, negative
return values are handled incorrectly.
<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">2.<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">It's said in nl_recv's
description that "The caller is responsible for<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">freeing the buffer
allocated * in \c *buf if a positive value is returned."<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">which means, we needn't
to free it in case it fails.<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">Additionally, nl_recv
has a BUG in handling buf: in the error branch, it frees<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">the buf, but didn't set
it to NULL. Freeing it outside in the caller function<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">will cause double free.<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">Thus, we set but(resp)
to NULL if nl_recv fails.</span></p>
</div>
</blockquote>
<br>
Thanks for finding this! It was a good catch, and your patch
corrects the problem you describe, but I think maybe we should take
this opportunity to clean up the function further:<br>
<br>
1) the first error return from the function returns -EINVAL,
implying that the return value will contain -errno, which is almost
never the case.<br>
<br>
2) for either of the first 2 error conditions, the function returns
with garbage in *resp. for the next 5 (!) error conditions, *resp
not only contains garbage, but we then "goto error" which attempts
to VIR_FREE(*resp).<br>
<br>
3) "rc" defaults to 0, and is set to -1 on error, while usually our
code follows the style of calling it "ret", defaulting to -1, then
setting to 0 just after the last opportunity for error.<br>
<br>
4) as you pointed out in a separate patch, the code below the
"error" label is not only executed when there is an error, but for
all return paths, and in that case it's our convention to call it
"cleanup".<br>
<br>
I'm preparing a separate patch which will address all 4 of these
problems. I'll Cc you when it is ready (should be within a few
hours).<br>
<br>
<blockquote
cite="mid:92984A2DD667AD43AADA1E474DC5E5965C0A9C56@szxema505-mbx.china.huawei.com"
type="cite">
<div class="WordSection1">
<p class="MsoNormal"><span lang="EN-US"><o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">Signed-off-by: Zhang Bo
<a class="moz-txt-link-rfc2396E" href="mailto:oscar.zhangbo@huawei.com"><oscar.zhangbo@huawei.com></a><o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">---<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">src/util/virnetlink.c |
8 ++++++--<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">1 file changed, 6
insertions(+), 2 deletions(-)<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">diff --git
a/src/util/virnetlink.c b/src/util/virnetlink.c<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">index 0cf18f2..1a09567
100644<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">---
a/src/util/virnetlink.c<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">+++
b/src/util/virnetlink.c<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">@@ -192,6 +192,7 @@ int
virNetlinkCommand(struct nl_msg *nl_msg,<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"> int n;<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"> struct nlmsghdr
*nlmsg = nlmsg_hdr(nl_msg);<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"> virNetlinkHandle
*nlhandle = NULL;<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">+ int length = 0;<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"><o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"> if (protocol >=
MAX_LINKS) {<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">
virReportSystemError(EINVAL,<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">@@ -257,12 +258,15 @@
int virNetlinkCommand(struct nl_msg *nl_msg,<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"> goto error;<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"> }<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"><o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">- *respbuflen =
nl_recv(nlhandle, &nladdr,<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">-
(unsigned char **)resp, NULL);<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">+ length =
nl_recv(nlhandle, &nladdr,<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">+
(unsigned char **)resp, NULL);<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">- if (*respbuflen
<= 0) {<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">+ if (length <= 0)
{<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">
virReportSystemError(errno,<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">
"%s", _("nl_recv failed"));<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">+ *resp = NULL;<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"> rc = -1;<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">+ } else {<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">+ *respbuflen =
length;<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"> }<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"> error:<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"> if (rc == -1) {<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">-- <o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">1.7.12.4<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p>
</div>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">--
libvir-list mailing list
<a class="moz-txt-link-abbreviated" href="mailto:libvir-list@redhat.com">libvir-list@redhat.com</a>
<a class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/libvir-list">https://www.redhat.com/mailman/listinfo/libvir-list</a></pre>
</blockquote>
<br>
</body>
</html>