Lucene - Core
  1. Lucene - Core
  2. LUCENE-6976

BytesTermAttributeImpl.copyTo NPEs when the BytesRef is null

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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
          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
          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
          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
          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
          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
          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
          Adrien Grand added a comment -

          OK, nevermind then +1 to the patch

          Show
          Adrien Grand added a comment - OK, nevermind then +1 to the patch
          Hide
          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
          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
          David Smiley added a comment -

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

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

          OK, +1

          Show
          Uwe Schindler added a comment - OK, +1
          Hide
          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
          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
          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
          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
          Michael McCandless added a comment -

          Reopen for backport to 5.4.2.

          Show
          Michael McCandless added a comment - Reopen for backport to 5.4.2.
          Hide
          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
          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:
              David Smiley
              Reporter:
              David Smiley
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development