<html>
  <head>
    <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    Hello Andrew, <br>
    <br>
        you are right, the fix may be less relaxing but still
    conforming: <br>
        if the height == 1, but the minY - sampleModelTranslateY > 0
    the data buffer should still contain at least one scanline. <br>
        Below is the updated fix. I've also added explicit check for
    implicit (minY - sampleModelTranslateY >= 0) constraint.<br>
    <br>
        <a
      href="http://cr.openjdk.java.net/%7Eanashaty/8038000/webrev.02/">http://cr.openjdk.java.net/%7Eanashaty/8038000/webrev.02/</a><br>
    <br>
    Thank you!<br>
    Anton.<br>
    <br>
    <div class="moz-cite-prefix">On 11.04.2014 15:06, Andrew Brygin
      wrote:<br>
    </div>
    <blockquote cite="mid:5347CCA4.4090907@oracle.com" type="cite">
      <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
      <div class="moz-cite-prefix">Hello Anton,<br>
        <br>
         we probably should use '(minY + height) > 1' as a condition
        for the <br>
         scanstride test, should not we?<br>
         Otherwise, we are not taking into account the case when minY is<br>
         greater than 0, and too big scanstride may be potentially
        dangerous.<br>
        <br>
        Thanks,<br>
        Andrew <br>
        <br>
        <br>
        On 4/10/2014 11:26 PM, anton nashatyrev wrote:<br>
      </div>
      <blockquote cite="mid:5346F064.9060600@oracle.com" type="cite">
        <meta content="text/html; charset=UTF-8"
          http-equiv="Content-Type">
        Hello,<br>
        <br>
            could you please review a slightly update fix version (the
        regression test upgraded) <br>
        fix: <a moz-do-not-send="true"
          href="http://cr.openjdk.java.net/%7Eanashaty/8038000/webrev.01/">http://cr.openjdk.java.net/%7Eanashaty/8038000/webrev.01/</a><br>
        bug: <a moz-do-not-send="true"
          href="https://bugs.openjdk.java.net/browse/JDK-8038000">https://bugs.openjdk.java.net/browse/JDK-8038000


        </a><br>
        <br>
        Jim, thanks for your in-depth analysis, the validation indeed
        doesn't look ideal. However my fix addresses the concrete
        regression which was introduced by these validations, so I'm
        leaving the fix as is. <br>
        <br>
        Thank you!<br>
        Anton.<br>
        <br>
        <div class="moz-cite-prefix">On 01.04.2014 19:39, anton
          nashatyrev wrote:<br>
        </div>
        <blockquote cite="mid:533ADD9F.50501@oracle.com" type="cite">Hello

          Jim, <br>
          <br>
          On 28.03.2014 3:25, Jim Graham wrote: <br>
          <blockquote type="cite">Hi Anton, <br>
            <br>
            A lot of those tests seem out of whack in that they test
            related conditions, but not the exact condition itself. 
            What we really want is for every index of the form: <br>
            <br>
            offset + y * scanlineStride + x + {0 -> numcomponents-1}
            => [0, buf.length-1] <br>
            <br>
            to be in the array for all valid x,y.  There are a lot of
            tests there that are sufficient to prove this, but are
            overly restrictive in that they reject a bunch of cases. 
            The fix you propose only fixes this for a case where h=1,
            but what about: <br>
            <br>
                w = 10 <br>
                h = 2 <br>
                numcomponents = 1 <br>
                scanlineStride = 1000 <br>
                buffer.length = 1010 <br>
            <br>
            The buffer contains the data for all possible pixel fetches,
            but since it isn't 2000 in length, we reject it. <br>
          </blockquote>
          <br>
          My fix actually relaxes the condition, and the case above is
          not rejected: <br>
          (height > 1 && scanlineStride > data.length) is
          FALSE here and hence the exception is not thrown <br>
          <br>
          <blockquote type="cite"> <br>
            Also, we restrict a bunch of parameters to "MAX_INT / some
            other factor" because we blindly multiply things out and
            don't want to deal with overflow, but a better "pixel array"
            test would use division (I think we do this in our native
            code): <br>
            <br>
                buf.length / w <= h <br>
            <br>
            except that you need to deal with offset and scanlineStride
            for h-1 lines and then w for the last one, so this gets
            complicate, but you have: <br>
            <br>
                ((buf.length - offset - w) / (h-1)) < scanlineStride
            <br>
            <br>
            but then you have to special case h=1 to avoid the divide by
            0 so you get: <br>
            <br>
                if (w <= 0 || h <= 0 || scanlineStride < 0 ||
            offset < 0) exception; <br>
                if (offset >= buf.length) exception; <br>
                int len = buf.length - offset; // known positive <br>
                if (len < w) exception; <br>
                if (h > 1) { <br>
                     if (((len - w) / (h - 1)) < scanlineStride)
            exception; <br>
                } <br>
            <br>
            Note that the test for (len < w) is done in all cases
            because it prevents the calculation in the "h>1" case
            from having a negative numerator, which would make the test
            invalid.  These tests are also valid for scan=0 for the rare
            case where someone wants every row of an image to contain
            the same data (we may use a case like that in the GIF
            loading code that needs to replicate incoming data for
            interlaced GIFs).  It doesn't go so far as to allow scan=-1
            or similar cases where the user wants to play games with
            aliasing parts of rows in a backwards cascade. <br>
          </blockquote>
          <br>
          There are a lot of checks in the *Raster.verify() methods. I'm
          not 100% confident but they look pretty equivalent to the
          algorithm you have described (BTW the 0 scanline is also
          acceptable). Anyways that was a security fix some time ago
          when some of those validations have been added and I'm not
          sure we would like to perform some major refactorings here
          unless any incompatibilities are found. <br>
          <br>
          Thank you! <br>
          Anton. <br>
          <br>
          <blockquote type="cite"> <br>
                        ...jim <br>
            <br>
            On 3/26/14 10:35 AM, anton nashatyrev wrote: <br>
            <blockquote type="cite">Hello, <br>
                   could you please review the following fix: <br>
              <br>
              fix: <a moz-do-not-send="true"
                class="moz-txt-link-freetext"
                href="http://cr.openjdk.java.net/%7Eanashaty/8038000/webrev.00/">http://cr.openjdk.java.net/~anashaty/8038000/webrev.00/</a>
              <br>
              <a moz-do-not-send="true" class="moz-txt-link-rfc2396E"
                href="http://cr.openjdk.java.net/%7Eanashaty/8038000/webrev.00/"><http://cr.openjdk.java.net/%7Eanashaty/8038000/webrev.00/></a>
              <br>
              bug: <a moz-do-not-send="true"
                class="moz-txt-link-freetext"
                href="https://bugs.openjdk.java.net/browse/JDK-8038000">https://bugs.openjdk.java.net/browse/JDK-8038000</a>
              <br>
              <br>
              The last row in the Raster shouldn't be necessary of the
              scanlineStride <br>
              length and thus the Raster with height 1 might have a
              buffer smaller <br>
              than a scanlineStride. <br>
              <br>
              Thanks! <br>
              Anton. <br>
            </blockquote>
          </blockquote>
          <br>
        </blockquote>
        <br>
      </blockquote>
      <br>
    </blockquote>
    <br>
  </body>
</html>