Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8291792: DefaultStyledDocument.setCharacterAttributes accepts negative length #9830

Closed
wants to merge 18 commits into from

Conversation

TejeshR13
Copy link
Contributor

@TejeshR13 TejeshR13 commented Aug 11, 2022

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
  • Commit message must refer to an issue
  • Change requires a CSR request to be approved

Issues

  • JDK-8291792: DefaultStyledDocument.setCharacterAttributes accepts negative length
  • JDK-8292557: DefaultStyledDocument.setCharacterAttributes accepts negative length (CSR)

Reviewers

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

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 11, 2022

👋 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.

@openjdk
Copy link

openjdk bot commented Aug 11, 2022

⚠️ @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
Copy link

openjdk bot commented Aug 11, 2022

@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 openjdk bot added the client client-libs-dev@openjdk.org label Aug 11, 2022
@openjdk openjdk bot added the rfr Pull request is ready for review label Aug 17, 2022
@mlbridge
Copy link

mlbridge bot commented Aug 17, 2022

Webrevs

@@ -0,0 +1,85 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guess it can be moved to test/jdk/javax/swing/text/DefaultStyledDocument/
Also, rename it something like TestDocNegLenCharAttr or something similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@@ -497,7 +497,7 @@ public Style getLogicalStyle(int p) {
* before setting the new attributes
*/
public void setCharacterAttributes(int offset, int length, AttributeSet s, boolean replace) {
if (length == 0) {
if (length <= 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@param length the length >= 0 javadoc should it not be then > 0 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, its should be > 0 in java doc. Will update it accordingly. Then should I raise a CSR......?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably yes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

* @run main DocNegLenCharAttrTest
*/
public class DocNegLenCharAttrTest {
private static DefaultStyledDocument doc;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guess it can be local variable in test()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Aug 17, 2022
@prrace
Copy link
Contributor

prrace commented Aug 23, 2022

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.

@TejeshR13
Copy link
Contributor Author

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......?

@prrace
Copy link
Contributor

prrace commented Aug 24, 2022

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

@TejeshR13
Copy link
Contributor Author

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

@prrace
Copy link
Contributor

prrace commented Aug 24, 2022

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 ..

@TejeshR13
Copy link
Contributor Author

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.

Copy link
Contributor

@prrace prrace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@TejeshR13 TejeshR13 changed the title 8291792: DefaultStyledDocument.setCharacterAttribute accepts negative length 8291792: DefaultStyledDocument.setCharacterAttributes accepts negative length Aug 29, 2022
@TejeshR13
Copy link
Contributor Author

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

Sure.....

@TejeshR13
Copy link
Contributor Author

/integrate auto

@openjdk openjdk bot added the auto label Aug 30, 2022
@openjdk
Copy link

openjdk bot commented Aug 30, 2022

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

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Aug 30, 2022
@openjdk
Copy link

openjdk bot commented Aug 30, 2022

@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:

  • bc6ac6f: 8292968: java.net.ContentHandler's javadoc has a broken reference
  • e016363: 8293007: riscv: failed to build after JDK-8290025
  • 9424d6d: 8293012: ConstantPool::print_on can crash if _cache is NULL
  • 40b0ed5: 8292891: ifdef-out some CDS-only functions
  • adb3d4f: 8292694: x86_64 c2i/i2c adapters may use more stack space than necessary
  • 30def49: 8292769: [JVMCI] OutOfMemoryError thrown when attaching the libgraal isolate causes HotSpot to crash.
  • a88a9e3: 8291466: C2: assert(false) failed: infinite loop in PhaseIterGVN::transform_old with -XX:+StressIGVN
  • d5167a9: 7189422: [macosx] Submenu's arrow have a wrong position
  • 512fee1: 8292972: Initialize fields if CodeBlobIterator shortcuts without heaps
  • a476ec5: 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 openjdk bot added the ready Pull request is ready to be integrated label Aug 30, 2022
@openjdk
Copy link

openjdk bot commented Aug 30, 2022

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Aug 30, 2022
@openjdk
Copy link

openjdk bot commented Aug 30, 2022

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

@prsadhuk
Copy link
Contributor

/sponsor

@openjdk
Copy link

openjdk bot commented Aug 30, 2022

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

  • f766d92: 8290344: Start/stop displaysync affects performance in metal rendering pipeline
  • afa5d4c: 8290451: Incorrect result when switching to C2 OSR compilation from C1
  • bc6ac6f: 8292968: java.net.ContentHandler's javadoc has a broken reference
  • e016363: 8293007: riscv: failed to build after JDK-8290025
  • 9424d6d: 8293012: ConstantPool::print_on can crash if _cache is NULL
  • 40b0ed5: 8292891: ifdef-out some CDS-only functions
  • adb3d4f: 8292694: x86_64 c2i/i2c adapters may use more stack space than necessary
  • 30def49: 8292769: [JVMCI] OutOfMemoryError thrown when attaching the libgraal isolate causes HotSpot to crash.
  • a88a9e3: 8291466: C2: assert(false) failed: infinite loop in PhaseIterGVN::transform_old with -XX:+StressIGVN
  • d5167a9: 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 openjdk bot added the integrated Pull request has been integrated label Aug 30, 2022
@openjdk openjdk bot closed this Aug 30, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Aug 30, 2022
@openjdk
Copy link

openjdk bot commented Aug 30, 2022

@prsadhuk @TejeshR13 Pushed as commit 4a28f37.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto client client-libs-dev@openjdk.org integrated Pull request has been integrated
3 participants