Derby
  1. Derby
  2. DERBY-3705

In Net Client mode, negative values for stream length are accepted without an exception for PreparedStatement.setAsciiStream()

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.4.1.3, 10.5.1.1
    • Fix Version/s: 10.5.1.1
    • Component/s: Network Client
    • Labels:
      None
    • Environment:
      Windows XP SP2, Derby trunk SVN checkout
    • Urgency:
      Normal

      Description

      This is related to Cloudscape bug 4250.

      Pass negative length as the stream length for various setXXXStream methods should throw an exception. But in Net Client mode, passing a negative value as stream length to PreparedStatement.setAsciiStream() doesn't throw an exception.

      e.g. //from store/StreamingColumn
      PreparedStatement ps = prepareStatement("insert into "
      + "testLongVarCharInvalidStreamLength11 values(?, ?, ?)");
      ps.setInt(1, 100);
      try

      { println("===> testing using setAsciiStream with -2 as length"); ps.setAsciiStream(2, fileIn, -2); // should throw exception here but doesn't. }

      This issue has been fixed for embedded mode, but not for client mode.

      1. derby-3705-2b.diff
        4 kB
        Kristian Waagan
      2. derby-3705-2.diff
        4 kB
        Suran Jayathilaka
      3. derby-3705.diff
        2 kB
        Suran Jayathilaka

        Issue Links

          Activity

          Hide
          Suran Jayathilaka added a comment -

          The same bug applies to client's PreparedStatement.setBinaryStream() and PreparedStatement.setCharacterStream() methods.

          Show
          Suran Jayathilaka added a comment - The same bug applies to client's PreparedStatement.setBinaryStream() and PreparedStatement.setCharacterStream() methods.
          Hide
          Suran Jayathilaka added a comment -

          Adds a less than zero check to setXXXStream methods in PreparedStatement, which throws an exception with NEGATIVE_STREAM_LENGTH state code if true.

          Show
          Suran Jayathilaka added a comment - Adds a less than zero check to setXXXStream methods in PreparedStatement, which throws an exception with NEGATIVE_STREAM_LENGTH state code if true.
          Hide
          Kristian Waagan added a comment -

          Good catch Suran
          Nice work on converting the big test.

          I''m changing the priority from critical to minor (press the yellow question mark next to the priority field to see descriptions of the various levels). There is a simple workaround available; don't specify negative stream lengths.
          It would be interesting to know what happens if you do specify a negative length though, and if that causes data corruption the priority would have to be raised again.

          I'll happily review your patch tomorrow.

          Show
          Kristian Waagan added a comment - Good catch Suran Nice work on converting the big test. I''m changing the priority from critical to minor (press the yellow question mark next to the priority field to see descriptions of the various levels). There is a simple workaround available; don't specify negative stream lengths. It would be interesting to know what happens if you do specify a negative length though, and if that causes data corruption the priority would have to be raised again. I'll happily review your patch tomorrow.
          Hide
          Kristian Waagan added a comment -

          The patch looks good to me, and is basically ready for commit.
          We do try to keep lines under 80 characters long, so you might want to reformat the lines that are longer than that.

          I also assume the regression test for this issue will be found in the test you are working on converting?

          As an extra step later on, one could also refactor the client code and create a method that checks the maximum and minimum length. Currently the code is duplicated in a number of methods.
          I also notice that there is a try-catch for SqlException, but that 'throw new SqlException(...).getSQLException()' is used within. It would be good to either get rid of the try-catch blocks or not use the getSQLException method.

          I have started the regression tests and will report back when they are done.

          Show
          Kristian Waagan added a comment - The patch looks good to me, and is basically ready for commit. We do try to keep lines under 80 characters long, so you might want to reformat the lines that are longer than that. I also assume the regression test for this issue will be found in the test you are working on converting? As an extra step later on, one could also refactor the client code and create a method that checks the maximum and minimum length. Currently the code is duplicated in a number of methods. I also notice that there is a try-catch for SqlException, but that 'throw new SqlException(...).getSQLException()' is used within. It would be good to either get rid of the try-catch blocks or not use the getSQLException method. I have started the regression tests and will report back when they are done.
          Hide
          Kristian Waagan added a comment -

          suites.All and derbyall passed.

          Show
          Kristian Waagan added a comment - suites.All and derbyall passed.
          Hide
          Suran Jayathilaka added a comment -

          This patch refactors length checking (for max value and negativity) code to a new method.

          Show
          Suran Jayathilaka added a comment - This patch refactors length checking (for max value and negativity) code to a new method.
          Hide
          Kristian Waagan added a comment - - edited

          Thank you Suran.
          I'm running the tests again and intend to commit the patch as soon as they have completed.

          I took the liberty to replace a few tabs in the patch, and changed some formatting in the new method to stay below 80 characters per line. Attached modified patch as 'derby-3705-2b.diff'.

          Show
          Kristian Waagan added a comment - - edited Thank you Suran. I'm running the tests again and intend to commit the patch as soon as they have completed. I took the liberty to replace a few tabs in the patch, and changed some formatting in the new method to stay below 80 characters per line. Attached modified patch as 'derby-3705-2b.diff'.
          Hide
          Kristian Waagan added a comment -

          Regression tests passed.
          Committed 'derby-3705-2b.diff' to trunk with revision 666174.

          Thanks for the contribution

          Show
          Kristian Waagan added a comment - Regression tests passed. Committed 'derby-3705-2b.diff' to trunk with revision 666174. Thanks for the contribution
          Hide
          Suran Jayathilaka added a comment -

          Patch committed.

          Show
          Suran Jayathilaka added a comment - Patch committed.
          Hide
          Kristian Waagan added a comment -

          Closing issue.

          Show
          Kristian Waagan added a comment - Closing issue.

            People

            • Assignee:
              Suran Jayathilaka
              Reporter:
              Suran Jayathilaka
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development