Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-6976

BytesTermAttributeImpl.copyTo NPEs when the BytesRef is null

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.5, 5.4.2
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      The BytesTermAttributeImpl class, not used much I think, has a problem in its copyTo method in which it assumes "bytes" isn't null since it calls BytesRef.deepCopyOf on it. Perhaps deepCopyOf should support null? And also, toString(), equals() and hashCode() aren't implemented but we can do so.
      This was discovered in SOLR-8541; the spatial PrefixTreeStrategy uses this attribute and the CachingTokenFilter when used on the analysis chain will call clearAttributes() in it's end() method and then capture the state so it can be replayed later. BytesTermAttributeImpl.clear() nulls out the bytes reference.

      1. LUCENE_6976.patch
        5 kB
        David Smiley
      2. LUCENE_6976.patch
        6 kB
        David Smiley

        Issue Links

          Activity

          Hide
          dsmiley David Smiley added a comment -

          See patch with fix and 2 tests (one from SOLR-8541 which is basically this same issue but the cause was not known at the time the bug was reported).

          Anyone have an opinion if BytesRef.deepCopyOf should handle null?

          Show
          dsmiley David Smiley added a comment - See patch with fix and 2 tests (one from SOLR-8541 which is basically this same issue but the cause was not known at the time the bug was reported). Anyone have an opinion if BytesRef.deepCopyOf should handle null?
          Hide
          jpountz Adrien Grand added a comment -

          >Anyone have an opinion if BytesRef.deepCopyOf should handle null?

          I would rather like it to not special-case null as it could hide bugs?

          The patch looks good, even if it's a bit awkward that the second attribute is instantiated through reflection in the test?

          Show
          jpountz Adrien Grand added a comment - >Anyone have an opinion if BytesRef.deepCopyOf should handle null? I would rather like it to not special-case null as it could hide bugs? The patch looks good, even if it's a bit awkward that the second attribute is instantiated through reflection in the test?
          Hide
          dsmiley David Smiley added a comment -

          Thanks for the review Adrien.

          I'll leave BytesRef.deepCopyOf alone then.

          The patch looks good, even if it's a bit awkward that the second attribute is instantiated through reflection in the test?

          The assertCopyIsEqual() was copied verbatim from TestCharTermAttributeImpl. Seems prettty reasonable to me – tests that there is a no-carg constructor.

          Show
          dsmiley David Smiley added a comment - Thanks for the review Adrien. I'll leave BytesRef.deepCopyOf alone then. The patch looks good, even if it's a bit awkward that the second attribute is instantiated through reflection in the test? The assertCopyIsEqual() was copied verbatim from TestCharTermAttributeImpl. Seems prettty reasonable to me – tests that there is a no-carg constructor.
          Hide
          jpountz Adrien Grand added a comment -

          OK, nevermind then +1 to the patch

          Show
          jpountz Adrien Grand added a comment - OK, nevermind then +1 to the patch
          Hide
          thetaphi Uwe Schindler added a comment - - edited

          Please don't implement toString() in attributes!!! This was removed consequently in favor of attribute reflection (LUCENE-6651, LUCENE-2374). Only CharTermAttribute implements it, because it is required for CharSequence impl.

          So -1 to current patch (I take this seriously).

          Show
          thetaphi Uwe Schindler added a comment - - edited Please don't implement toString() in attributes!!! This was removed consequently in favor of attribute reflection ( LUCENE-6651 , LUCENE-2374 ). Only CharTermAttribute implements it, because it is required for CharSequence impl. So -1 to current patch (I take this seriously).
          Hide
          dsmiley David Smiley added a comment -

          Updated patch removing toString() impl and adjusted test to not call it.

          Show
          dsmiley David Smiley added a comment - Updated patch removing toString() impl and adjusted test to not call it.
          Hide
          thetaphi Uwe Schindler added a comment -

          OK, +1

          Show
          thetaphi Uwe Schindler added a comment - OK, +1
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1724874 from David Smiley in branch 'dev/trunk'
          [ https://svn.apache.org/r1724874 ]

          LUCENE-6976 SOLR-8541: BytesTermAttributeImpl.copyTo could NPE.
          Could be triggered by trying to highlight a spatial RPT field.

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1724874 from David Smiley in branch 'dev/trunk' [ https://svn.apache.org/r1724874 ] LUCENE-6976 SOLR-8541 : BytesTermAttributeImpl.copyTo could NPE. Could be triggered by trying to highlight a spatial RPT field.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1724877 from David Smiley in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1724877 ]

          LUCENE-6976 SOLR-8541: BytesTermAttributeImpl.copyTo could NPE.
          Could be triggered by trying to highlight a spatial RPT field.

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1724877 from David Smiley in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1724877 ] LUCENE-6976 SOLR-8541 : BytesTermAttributeImpl.copyTo could NPE. Could be triggered by trying to highlight a spatial RPT field.
          Hide
          mikemccand Michael McCandless added a comment -

          Reopen for backport to 5.4.2.

          Show
          mikemccand Michael McCandless added a comment - Reopen for backport to 5.4.2.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 7d52c2523c7a4ff70612742b76b934a12b493331 in lucene-solr's branch refs/heads/branch_5_4 from David Smiley
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7d52c25 ]

          LUCENE-6976 SOLR-8541: BytesTermAttributeImpl.copyTo could NPE.
          Could be triggered by trying to highlight a spatial RPT field.

          git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/branches/branch_5x@1724877 13f79535-47bb-0310-9956-ffa450edef68

          Conflicts:
          lucene/CHANGES.txt
          solr/CHANGES.txt

          Show
          jira-bot ASF subversion and git services added a comment - Commit 7d52c2523c7a4ff70612742b76b934a12b493331 in lucene-solr's branch refs/heads/branch_5_4 from David Smiley [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7d52c25 ] LUCENE-6976 SOLR-8541 : BytesTermAttributeImpl.copyTo could NPE. Could be triggered by trying to highlight a spatial RPT field. git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/branches/branch_5x@1724877 13f79535-47bb-0310-9956-ffa450edef68 Conflicts: lucene/CHANGES.txt solr/CHANGES.txt

            People

            • Assignee:
              dsmiley David Smiley
              Reporter:
              dsmiley David Smiley
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development