[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] RFR 8078654: CloseTTFontFileFunc callback should be removed
From:       Phil Race <philip.race () oracle ! com>
Date:       2015-04-30 20:55:37
Message-ID: 554296C9.50102 () oracle ! com
[Download RAW message or body]

I don't read it the way you do.
The code is getting a reference to the java "Font2D" - almost
certainly an instance of the "TrueTypeFont" subclass
That is NULL which is the source of the crash.
But if it does retrieve it then it wants the value of the String 
'platName' field.
For TrueTypeFont that always
  holds the name of the file that contains the font.
It then asks for the name of the file in platform encoding ..

    const char *name = JNU_GetStringPlatformChars(env, platName, NULL);

  but  promptly releases it again without doing anything ...

  JNU_ReleaseStringPlatformChars(env, platName, name);

These two calls are a matching pair, the latter isn't freeing
some 'platform allocated' resource simply referenced by the VM.
The first JNU call allocated that resource.

It is as if there was an intention to do something file system
related with that name between the Get and the Release
but you can't close a file descriptor based on the file name,
so it looks like an imcomplete piece of work to me.

-phil.

On 04/30/2015 01:33 AM, Andrew Dinn wrote:
> On 29/04/15 23:23, Phil Race wrote:
>> I think this fix should be fine. There should be no reason
>> we need to do anything here as the file should already be
>> closed by the Java side which has demonstrably gone away
>> so should have already closed the file before then.
> That's true and I'm glad to hear this fix will go into jdk9 (and
> hopefully 7 and 8). Please let me know if the webrev is ok or you need
> any further input.
>
> However, I'll note in passing that the closefn was never there in order
> to ensure the JVM closes the file (at least it has been as is for as far
> back as I can check the source). The callback looks for some String
> memory to free, specifically a char buffer associated with a particular
> type of Windows character set -- it trips up because the reference to
> the object holding the String returns null. When you investigate the
> provenance of the String it turns out that it is always just a normal
> String with a normal char buffer -- and therefore the attempt to free
> this char buffer is at best misguided. That's the real reason why this
> callback is redundant.
>
> regards,
>
>
> Andrew Dinn
> -----------
>


[Attachment #3 (text/html)]

<html>
  <head>
    <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <div class="moz-cite-prefix">I don't read it the way you do.<br>
      The code is getting a reference to the java "Font2D" - almost<br>
      certainly an instance of the "TrueTypeFont" subclass<br>
      That is NULL which is the source of the crash.<br>
      But if it does retrieve it then it wants the value of the String
      'platName' field.<br>
      For TrueTypeFont that always<br>
        holds the name of the file that contains the font.<br>
      It then asks for the name of the file in platform encoding ..<br>
      <pre class="sourcelines wrap"><span id="l1.17" class="minusline">   const char \
*name = JNU_GetStringPlatformChars(env, platName, NULL);</span><span id="l1.18" \
class="minusline">

</span></pre>
      <pre class="sourcelines wrap"><small><span id="l1.18" class="minusline"> \
<big><big>but</big></big></span></small> promptly releases it again without doing \
anything ... <span id="l1.18" class="minusline"></span></pre>
      <span id="l1.18" class="minusline">
          JNU_ReleaseStringPlatformChars(env, platName, name);</span><br>
      <br>
      These two calls are a matching pair, the latter isn't freeing<br>
      some 'platform allocated' resource simply referenced by the VM.<br>
      The first JNU call allocated that resource. <br>
      <br>
      It is as if there was an intention to do something file system<br>
      related with that name between the Get and the Release<br>
      but you can't close a file descriptor based on the file name,<br>
      so it looks like an imcomplete piece of work to me.<br>
      <br>
      -phil.<br>
      <br>
      On 04/30/2015 01:33 AM, Andrew Dinn wrote:<br>
    </div>
    <blockquote cite="mid:5541E8DD.2020008@redhat.com" type="cite">
      <pre wrap="">On 29/04/15 23:23, Phil Race wrote:
</pre>
      <blockquote type="cite">
        <pre wrap="">I think this fix should be fine. There should be no reason
we need to do anything here as the file should already be
closed by the Java side which has demonstrably gone away
so should have already closed the file before then.
</pre>
      </blockquote>
      <pre wrap="">
That's true and I'm glad to hear this fix will go into jdk9 (and
hopefully 7 and 8). Please let me know if the webrev is ok or you need
any further input.

However, I'll note in passing that the closefn was never there in order
to ensure the JVM closes the file (at least it has been as is for as far
back as I can check the source). The callback looks for some String
memory to free, specifically a char buffer associated with a particular
type of Windows character set -- it trips up because the reference to
the object holding the String returns null. When you investigate the
provenance of the String it turns out that it is always just a normal
String with a normal char buffer -- and therefore the attempt to free
this char buffer is at best misguided. That's the real reason why this
callback is redundant.

regards,


Andrew Dinn
-----------

</pre>
    </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