jdk icon indicating copy to clipboard operation
jdk copied to clipboard
verified publisher icon openjdk

8291792: DefaultStyledDocument.setCharacterAttribute accepts negative length

Open TejeshR13 opened this issue 3 years ago • 3 comments

The Document for DefaultStyledDocument.setCharacterAttribute states that the range of length accepted is length - the length >= 0, whereas in code the length check is done only for length==0. Meaning the control just returns if the length is 0. Since length is int type and there is a possibility of negative length being set through the method, the code doesn't actually handles the negative length case. Hence to handle negative length, negative length check has been added along with 0 check. Its safe to check and return the control for negative length input. Test case to check the same is added.

Progress

  • [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • [ ] Change must not contain extraneous whitespace
  • [x] Commit message must refer to an issue

Issue

  • JDK-8291792: DefaultStyledDocument.setCharacterAttribute accepts negative length

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9830/head:pull/9830
$ git checkout pull/9830

Update a local copy of the PR:
$ git checkout pull/9830
$ git pull https://git.openjdk.org/jdk pull/9830/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9830

View PR using the GUI difftool:
$ git pr show -t 9830

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9830.diff

TejeshR13 avatar Aug 11 '22 05:08 TejeshR13

:wave: Welcome back tr! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

bridgekeeper[bot] avatar Aug 11 '22 05:08 bridgekeeper[bot]

⚠️ @TejeshR13 This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

openjdk[bot] avatar Aug 11 '22 05:08 openjdk[bot]

@TejeshR13 The following label will be automatically applied to this pull request:

  • client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

openjdk[bot] avatar Aug 11 '22 05:08 openjdk[bot]

Webrevs

mlbridge[bot] avatar Aug 17 '22 06:08 mlbridge[bot]

Although I've reviewed the CSR I'd prefer it not be finalized yet. I think it needs more changes. The spec is SILENT about what then happens if length is negative and SILENT about an out of range offset too. I think we should address these issues as well.

Also everywhere - in the CSR and the bug description and summary the text said the method is called setCharacterAttribute whereas it is setCharacterAttributes.

prrace avatar Aug 23 '22 22:08 prrace

Although I've reviewed the CSR I'd prefer it not be finalized yet. I think it needs more changes. The spec is SILENT about what then happens if length is negative and SILENT about an out of range offset too. I think we should address these issues as well.

Also everywhere - in the CSR and the bug description and summary the text said the method is called setCharacterAttribute whereas it is setCharacterAttributes.

For negative length, now it just returns the control without processing it whereas before it used to raise an exception in arrayCopy method. Upper bound for length should be the text length(which is to be modified/added) I feel, still even if it crosses overall text size that case is handled by limiting to text length. These are not mentioned in spec, so should we modify the spec by adding the range bounds for length......?

TejeshR13 avatar Aug 24 '22 05:08 TejeshR13

Although I've reviewed the CSR I'd prefer it not be finalized yet. I think it needs more changes. The spec is SILENT about what then happens if length is negative and SILENT about an out of range offset too. I think we should address these issues as well. Also everywhere - in the CSR and the bug description and summary the text said the method is called setCharacterAttribute whereas it is setCharacterAttributes.

For negative length, now it just returns the control without processing it whereas before it used to raise an exception in arrayCopy method. Upper bound for length should be the text length(which is to be modified/added) I feel, still even if it crosses overall text size that case is handled by limiting to text length. These are not mentioned in spec, so should we modify the spec by adding the range bounds for length......?

Yes I am saying we should mention all of this

prrace avatar Aug 24 '22 17:08 prrace

Although I've reviewed the CSR I'd prefer it not be finalized yet. I think it needs more changes. The spec is SILENT about what then happens if length is negative and SILENT about an out of range offset too. I think we should address these issues as well. Also everywhere - in the CSR and the bug description and summary the text said the method is called setCharacterAttribute whereas it is setCharacterAttributes.

For negative length, now it just returns the control without processing it whereas before it used to raise an exception in arrayCopy method. Upper bound for length should be the text length(which is to be modified/added) I feel, still even if it crosses overall text size that case is handled by limiting to text length. These are not mentioned in spec, so should we modify the spec by adding the range bounds for length......?

Yes I am saying we should mention all of this

Will this addition to spec be fine -

      * A write lock is held by this operation while changes
      * are being made, and a DocumentEvent is sent to the listeners
      * after the change has been successfully completed.
+     *
+     * <p>
+     * The expected {@Code length} range is the length of the text
+     * in which the attributes are set. If the length is &lt;= 0, then no
+     * attributes are set, the control returns. If the length is &gt; the
+     * length of text in which the attributes are to be set then the
+     * extra length is trimmed.
+     * </p>
+     *
      * <p>
      * This method is thread safe, although most Swing methods
      * are not. Please see

TejeshR13 avatar Aug 24 '22 18:08 TejeshR13

Although I've reviewed the CSR I'd prefer it not be finalized yet. I think it needs more changes. The spec is SILENT about what then happens if length is negative and SILENT about an out of range offset too. I think we should address these issues as well. Also everywhere - in the CSR and the bug description and summary the text said the method is called setCharacterAttribute whereas it is setCharacterAttributes.

For negative length, now it just returns the control without processing it whereas before it used to raise an exception in arrayCopy method. Upper bound for length should be the text length(which is to be modified/added) I feel, still even if it crosses overall text size that case is handled by limiting to text length. These are not mentioned in spec, so should we modify the spec by adding the range bounds for length......?

Yes I am saying we should mention all of this

Will this addition to spec be fine -

      * A write lock is held by this operation while changes
      * are being made, and a DocumentEvent is sent to the listeners
      * after the change has been successfully completed.
+     *
+     * <p>
+     * The expected {@Code length} range is the length of the text
+     * in which the attributes are set. If the length is &lt;= 0, then no
+     * attributes are set, the control returns. If the length is &gt; the
+     * length of text in which the attributes are to be set then the
+     * extra length is trimmed.
+     * </p>
+     *
      * <p>
      * This method is thread safe, although most Swing methods
      * are not. Please see

should be {@code .. } And the control doesn't return, the method does. I think you were trying to say control returns to the caller but that doesn't work here and the interaction with offset is cryptic .. assuming that's the reference to "trimmed". Something that makes clear that the replace arg isn't used in such a case might help too.

I expect something like

 * {@code offset} and {@code length} define the range of the text over which the attributes are set.
 * If the length is &lt;= 0, then no action is taken  and the method just returns.
 * If the offset is < 0 or >= the length of the text then no action is taken, and the method just returns
 *  Otherwise if {@code offset + length} will exceed the length of the  text then the affected range is truncated to that length
 *

But YOU need to verify this is all actually true ..

prrace avatar Aug 24 '22 23:08 prrace

Although I've reviewed the CSR I'd prefer it not be finalized yet. I think it needs more changes. The spec is SILENT about what then happens if length is negative and SILENT about an out of range offset too. I think we should address these issues as well. Also everywhere - in the CSR and the bug description and summary the text said the method is called setCharacterAttribute whereas it is setCharacterAttributes.

For negative length, now it just returns the control without processing it whereas before it used to raise an exception in arrayCopy method. Upper bound for length should be the text length(which is to be modified/added) I feel, still even if it crosses overall text size that case is handled by limiting to text length. These are not mentioned in spec, so should we modify the spec by adding the range bounds for length......?

Yes I am saying we should mention all of this

Will this addition to spec be fine -

      * A write lock is held by this operation while changes
      * are being made, and a DocumentEvent is sent to the listeners
      * after the change has been successfully completed.
+     *
+     * <p>
+     * The expected {@Code length} range is the length of the text
+     * in which the attributes are set. If the length is &lt;= 0, then no
+     * attributes are set, the control returns. If the length is &gt; the
+     * length of text in which the attributes are to be set then the
+     * extra length is trimmed.
+     * </p>
+     *
      * <p>
      * This method is thread safe, although most Swing methods
      * are not. Please see

should be {@code .. } And the control doesn't return, the method does. I think you were trying to say control returns to the caller but that doesn't work here and the interaction with offset is cryptic .. assuming that's the reference to "trimmed". Something that makes clear that the replace arg isn't used in such a case might help too.

I expect something like

 * {@code offset} and {@code length} define the range of the text over which the attributes are set.
 * If the length is &lt;= 0, then no action is taken  and the method just returns.
 * If the offset is < 0 or >= the length of the text then no action is taken, and the method just returns
 *  Otherwise if {@code offset + length} will exceed the length of the  text then the affected range is truncated to that length
 *

But YOU need to verify this is all actually true ..

Yes @prrace , I have verified this and I will update the spec. Except offset is <= 0 or > the length everything is right.

TejeshR13 avatar Aug 25 '22 03:08 TejeshR13

OK but you need to change the title of this PR to match JBS.

Sure.....

TejeshR13 avatar Aug 29 '22 04:08 TejeshR13

/integrate auto

TejeshR13 avatar Aug 30 '22 06:08 TejeshR13

@TejeshR13 This pull request will be automatically integrated when it is ready

openjdk[bot] avatar Aug 30 '22 06:08 openjdk[bot]

@TejeshR13 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8291792: DefaultStyledDocument.setCharacterAttributes accepts negative length

Reviewed-by: psadhukhan, prr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 231 new commits pushed to the master branch:

  • bc6ac6f7af7a41a7f045c9f2e443e6d204197051: 8292968: java.net.ContentHandler's javadoc has a broken reference
  • e016363b54f1624e9ff4470803c6000d8fe91a7f: 8293007: riscv: failed to build after JDK-8290025
  • 9424d6d487db4ad0f6f671a8c33b8f169794fe25: 8293012: ConstantPool::print_on can crash if _cache is NULL
  • 40b0ed565720fa75fc23e1a8f26e7cfe5feebf8f: 8292891: ifdef-out some CDS-only functions
  • adb3d4f14af1974e7fc9461eb59f98131f0d33f7: 8292694: x86_64 c2i/i2c adapters may use more stack space than necessary
  • 30def49c7286e2a6c2585bc85084b003eec4543a: 8292769: [JVMCI] OutOfMemoryError thrown when attaching the libgraal isolate causes HotSpot to crash.
  • a88a9e344f66bca21f3a01dbbcea19b52af14865: 8291466: C2: assert(false) failed: infinite loop in PhaseIterGVN::transform_old with -XX:+StressIGVN
  • d5167a91a9d35afba1a2f246f9d320f1cbb998b2: 7189422: [macosx] Submenu's arrow have a wrong position
  • 512fee1d1eee4aa4bb7362cc9cb48d63e129a525: 8292972: Initialize fields if CodeBlobIterator shortcuts without heaps
  • a476ec5c81968e9a8cf7fb02f61cc4a5c8c2d520: 8292983: ModuleReferenceImpl.computeHash should record algorithm for cache checks
  • ... and 221 more: https://git.openjdk.org/jdk/compare/77398430b5e13768cddd5f63e8fe9e53735bbea8...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@prsadhuk, @prrace) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk[bot] avatar Aug 30 '22 06:08 openjdk[bot]

/integrate

openjdk[bot] avatar Aug 30 '22 06:08 openjdk[bot]

@openjdk[bot] Your change (at version adde88d11e49efcd12182fadf6c5b9589ff906fa) is now ready to be sponsored by a Committer.

openjdk[bot] avatar Aug 30 '22 06:08 openjdk[bot]

/sponsor

prsadhuk avatar Aug 30 '22 08:08 prsadhuk

Going to push as commit 4a28f3798d25b64d44aad557f94d1045c253cdfb. Since your change was applied there have been 233 commits pushed to the master branch:

  • f766d92755276a40f0cdc087db32c285548572fe: 8290344: Start/stop displaysync affects performance in metal rendering pipeline
  • afa5d4ced38b7f3752932c28af96d8fc600d1df7: 8290451: Incorrect result when switching to C2 OSR compilation from C1
  • bc6ac6f7af7a41a7f045c9f2e443e6d204197051: 8292968: java.net.ContentHandler's javadoc has a broken reference
  • e016363b54f1624e9ff4470803c6000d8fe91a7f: 8293007: riscv: failed to build after JDK-8290025
  • 9424d6d487db4ad0f6f671a8c33b8f169794fe25: 8293012: ConstantPool::print_on can crash if _cache is NULL
  • 40b0ed565720fa75fc23e1a8f26e7cfe5feebf8f: 8292891: ifdef-out some CDS-only functions
  • adb3d4f14af1974e7fc9461eb59f98131f0d33f7: 8292694: x86_64 c2i/i2c adapters may use more stack space than necessary
  • 30def49c7286e2a6c2585bc85084b003eec4543a: 8292769: [JVMCI] OutOfMemoryError thrown when attaching the libgraal isolate causes HotSpot to crash.
  • a88a9e344f66bca21f3a01dbbcea19b52af14865: 8291466: C2: assert(false) failed: infinite loop in PhaseIterGVN::transform_old with -XX:+StressIGVN
  • d5167a91a9d35afba1a2f246f9d320f1cbb998b2: 7189422: [macosx] Submenu's arrow have a wrong position
  • ... and 223 more: https://git.openjdk.org/jdk/compare/77398430b5e13768cddd5f63e8fe9e53735bbea8...master

Your commit was automatically rebased without conflicts.

openjdk[bot] avatar Aug 30 '22 08:08 openjdk[bot]

@prsadhuk @TejeshR13 Pushed as commit 4a28f3798d25b64d44aad557f94d1045c253cdfb.

:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk[bot] avatar Aug 30 '22 08:08 openjdk[bot]