[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR [XS]: 8234968: check calloc rv in libinstrument InvocationAdapter
From: serguei.spitsyn () oracle ! com
Date: 2019-12-13 22:01:56
Message-ID: 97f4c96f-73bb-b5fd-1b2f-91ef18012269 () oracle ! com
[Download RAW message or body]
Hi Matthias,
+1
Thanks,
Serguei
On 12/12/19 2:00 AM, Langer, Christoph wrote:
>
> Hi Matthias,
>
> I think your current patch is good as it is – at least it wouldn’t
> make things worse, AFAICS.
>
> Further improvements can probably be done under another issue.
>
> Cheers
>
> Christoph
>
> *From:* serviceability-dev
> <serviceability-dev-bounces@openjdk.java.net> *On Behalf Of *Baesken,
> Matthias
> *Sent:* Freitag, 29. November 2019 08:18
> *To:* Thomas Stüfe <thomas.stuefe@gmail.com>
> *Cc:* serviceability-dev@openjdk.java.net
> *Subject:* [CAUTION] RE: RFR [XS]: 8234968: check calloc rv in
> libinstrument InvocationAdapter
>
> Hi Thomas, Christoph, thanks for the comments . Of course the init
> of * decodedLen must be added .
>
> In case of returning NULL from decodePath , we would have
> tmp == NULL (in char* tmp = func; ) , assign tmp to res and
> then we jplis_assert , see :
>
> #define TRANSFORM(res,func) { \
>
> char* tmp = func; \
>
> if (tmp != res) { \
>
> free(res); \
>
> res = tmp; \
>
> } \
>
> jplis_assert((void*)res != (void*)NULL); \
>
> }
>
> ….
>
> TRANSFORM(path, decodePath(path,&len));
>
> New webrev :
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8234968.2/
>
> Best regards, Matthias
>
> *From:* Thomas Stüfe <thomas.stuefe@gmail.com
> <mailto:thomas.stuefe@gmail.com>>
> *Sent:* Freitag, 29. November 2019 07:30
> *To:* Baesken, Matthias <matthias.baesken@sap.com
> <mailto:matthias.baesken@sap.com>>
> *Cc:* serviceability-dev@openjdk.java.net
> <mailto:serviceability-dev@openjdk.java.net>
> *Subject:* Re: RFR [XS]: 8234968: check calloc rv in libinstrument
> InvocationAdapter
>
> Hi Matthias,
>
> I am not certain the callers are prepared to handle NULL.
>
> This is used in a chain of TRANSFORM macro calls which AFAICS do not
> handle NULL; e.g. , at 872, we pass the returned pointer to
> convertUft8ToPlatformString which passes it on (on Windows) to
> MultiByteToWideChar, which does not handle NULL input.
>
> So I wonder whether a clear error message with an exit would be better
> in this case. Otherwise we may get a crash just some instructions later.
>
> Cheers, Thomas
>
> On Thu, Nov 28, 2019 at 5:21 PM Baesken, Matthias
> <matthias.baesken@sap.com <mailto:matthias.baesken@sap.com>> wrote:
>
> Hello, please review this small patch .
>
> It adds return value checking for calloc at one place where it is
> missing .
>
> Thanks, Matthias
>
> Bug/webrev :
>
> https://bugs.openjdk.java.net/browse/JDK-8234968
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8234968.1/
>
[Attachment #3 (text/html)]
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
Hi Matthias,<br>
<br>
+1<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<div class="moz-cite-prefix">On 12/12/19 2:00 AM, Langer, Christoph
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:AM6PR02MB55410D34D1D0E92EF886DE808A550@AM6PR02MB5541.eurprd02.prod.outlook.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<meta name="Generator" content="Microsoft Word 15 (filtered
medium)">
<style><!--
/* Font Definitions */
@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;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:11.0pt;
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;}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-margin-top-alt:auto;
margin-right:0cm;
mso-margin-bottom-alt:auto;
margin-left:0cm;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
span.EmailStyle18
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:windowtext;}
span.EmailStyle20
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:70.85pt 70.85pt 2.0cm 70.85pt;}
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">Hi Matthias,<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">I think your current patch is good as it is
– at least it wouldn’t make things worse, AFAICS.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Further improvements can probably be done
under another issue.<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Cheers<o:p></o:p></p>
<p class="MsoNormal">Christoph<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0cm
0cm 0cm 4.0pt">
<div>
<div style="border:none;border-top:solid #E1E1E1
1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal"><b>From:</b> serviceability-dev
<a class="moz-txt-link-rfc2396E" \
href="mailto:serviceability-dev-bounces@openjdk.java.net"><serviceability-dev-bounces@openjdk.java.net></a>
<b>On Behalf Of </b>Baesken, Matthias<br>
<b>Sent:</b> Freitag, 29. November 2019 08:18<br>
<b>To:</b> Thomas Stüfe <a class="moz-txt-link-rfc2396E" \
href="mailto:thomas.stuefe@gmail.com"><thomas.stuefe@gmail.com></a><br>
<b>Cc:</b> <a class="moz-txt-link-abbreviated" \
href="mailto:serviceability-dev@openjdk.java.net">serviceability-dev@openjdk.java.net</a><br>
<b>Subject:</b> [CAUTION] RE: RFR [XS]: 8234968: check
calloc rv in libinstrument InvocationAdapter<o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Hi Thomas, Christoph, thanks for the
comments . Of course the init of * decodedLen must be
added .<o:p></o:p></p>
<p class="MsoNormal">In case of returning NULL from
decodePath , we would have tmp == NULL (in char* tmp
= func; ) , assign tmp to res and then we
jplis_assert , see :<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">#define TRANSFORM(res,func) { \<o:p></o:p></p>
<p class="MsoNormal"> char* tmp = func; \<o:p></o:p></p>
<p class="MsoNormal"> if (tmp != res) { \<o:p></o:p></p>
<p class="MsoNormal"> free(res); \<o:p></o:p></p>
<p class="MsoNormal"> res = tmp; \<o:p></o:p></p>
<p class="MsoNormal"> } \<o:p></o:p></p>
<p class="MsoNormal"> jplis_assert((void*)res !=
(void*)NULL); \<o:p></o:p></p>
<p class="MsoNormal">}<o:p></o:p></p>
<p class="MsoNormal">….<o:p></o:p></p>
<p class="MsoNormal">TRANSFORM(path,
decodePath(path,&len));<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">New webrev :<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"><span lang="DE"><a
href="http://cr.openjdk.java.net/~mbaesken/webrevs/8234968.2/"
moz-do-not-send="true"><span \
lang="EN-US">http://cr.openjdk.java.net/~mbaesken/webrevs/8234968.2/</span></a><o:p></o:p></span></p>
<p class="MsoNormal"><span lang="DE"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="DE"><o:p> </o:p></span></p>
<p class="MsoNormal">Best regards, Matthias<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<div style="border:none;border-left:solid blue
1.5pt;padding:0cm 0cm 0cm 4.0pt">
<div>
<div style="border:none;border-top:solid #E1E1E1
1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal"><b>From:</b> Thomas Stüfe <<a
href="mailto:thomas.stuefe@gmail.com"
moz-do-not-send="true">thomas.stuefe@gmail.com</a>>
<br>
<b>Sent:</b> Freitag, 29. November 2019 07:30<br>
<b>To:</b> Baesken, Matthias <<a
href="mailto:matthias.baesken@sap.com"
moz-do-not-send="true">matthias.baesken@sap.com</a>><br>
<b>Cc:</b> <a
href="mailto:serviceability-dev@openjdk.java.net"
moz-do-not-send="true">serviceability-dev@openjdk.java.net</a><br>
<b>Subject:</b> Re: RFR [XS]: 8234968: check calloc rv
in libinstrument InvocationAdapter<o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"><span lang="DE"><o:p> </o:p></span></p>
<div>
<p class="MsoNormal"><span lang="DE">Hi Matthias,<o:p></o:p></span></p>
<div>
<p class="MsoNormal"><span lang="DE"><o:p> </o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span lang="DE">I am not certain
the callers are prepared to handle NULL. <o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span lang="DE"><o:p> </o:p></span></p>
</div>
<p class="MsoNormal"><span lang="DE">This is used in a
chain of TRANSFORM macro calls which AFAICS do not
handle NULL; e.g. , at 872, we pass the returned
pointer to convertUft8ToPlatformString which passes it
on (on Windows) to MultiByteToWideChar, which does not
handle NULL input.<o:p></o:p></span></p>
<div>
<p class="MsoNormal"><span lang="DE"><o:p> </o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span lang="DE">So I wonder whether
a clear error message with an exit would be better
in this case. Otherwise we may get a crash just some
instructions later.<o:p></o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span lang="DE"><o:p> </o:p></span></p>
</div>
<div>
<p class="MsoNormal"><span lang="DE">Cheers, \
Thomas<o:p></o:p></span></p> </div>
<div>
<p class="MsoNormal"><span lang="DE"><o:p> </o:p></span></p>
</div>
</div>
<p class="MsoNormal"><span lang="DE"><o:p> </o:p></span></p>
<div>
<div>
<p class="MsoNormal"><span lang="DE">On Thu, Nov 28,
2019 at 5:21 PM Baesken, Matthias <<a
href="mailto:matthias.baesken@sap.com"
moz-do-not-send="true">matthias.baesken@sap.com</a>>
wrote:<o:p></o:p></span></p>
</div>
<blockquote style="border:none;border-left:solid #CCCCCC
1.0pt;padding:0cm 0cm 0cm
6.0pt;margin-left:4.8pt;margin-top:5.0pt;margin-right:0cm;margin-bottom:5.0pt">
<div>
<div>
<p class="MsoNormal"
\
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">Hello,
please review this small patch .<span \
lang="DE"><o:p></o:p></span></p> <p class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">It
adds return value checking for calloc at one place
where it is missing .<span lang="DE"><o:p></o:p></span></p>
<p class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> \
<span lang="DE"><o:p></o:p></span></p>
<p class="MsoNormal"
\
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">Thanks, Matthias<span \
lang="DE"><o:p></o:p></span></p> <p class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> \
<span lang="DE"><o:p></o:p></span></p>
<p class="MsoNormal"
\
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto">Bug/webrev :<span \
lang="DE"><o:p></o:p></span></p> <p class="MsoNormal"
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"> \
<span lang="DE"><o:p></o:p></span></p>
<p class="MsoNormal"
\
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span lang="DE"><a
href="https://bugs.openjdk.java.net/browse/JDK-8234968"
target="_blank" moz-do-not-send="true"><span
\
lang="EN-US">https://bugs.openjdk.java.net/browse/JDK-8234968</span></a><o:p></o:p></span></p>
<p class="MsoNormal"
\
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span lang="DE"> \
<o:p></o:p></span></p> <p class="MsoNormal"
\
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span lang="DE"><a
\
href="http://cr.openjdk.java.net/~mbaesken/webrevs/8234968.1/"
target="_blank" \
moz-do-not-send="true">http://cr.openjdk.java.net/~mbaesken/webrevs/8234968.1/</a><o:p></o:p></span></p>
<p class="MsoNormal"
\
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span lang="DE"> \
<o:p></o:p></span></p> <p class="MsoNormal"
\
style="mso-margin-top-alt:auto;mso-margin-bottom-alt:auto"><span lang="DE"> \
<o:p></o:p></span></p> </div>
</div>
</blockquote>
</div>
</div>
</div>
</div>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic