Merge pull request #1004 from openzim/protection-against-too-long-single-word-titles

Protection against too long single word titles

Author: kelson42

include/zim/error.h

@@ -52,9 +52,10 @@ namespace zim
       {}
   };
 
-  /* Exception thrown by the Creator in case of error.
+  /**
+   * Exception thrown by the Creator in case of error.
    *
-   * Most exceptions actually thrown are inheriting this exception.
+   * Most exceptions actually thrown are subclasses of this type.
    */
   class LIBZIM_API CreatorError : public std::runtime_error
   {
@@ -64,7 +65,9 @@ namespace zim
       {}
   };
 
-   /* Exception thrown when a entry cannot be added to the Creator.*/
+  /**
+   * Exception thrown when an entry cannot be added to the ZIM archive.
+   */
   class LIBZIM_API InvalidEntry : public CreatorError
   {
     public:
@@ -73,14 +76,16 @@ namespace zim
       {}
   };
 
-  /* Exception thrown if a incoherence in the user implementation has been detected.
+  /**
+   * Exception thrown if incoherence in the user implementation has been
+   * detected.
    *
    * Users need to implement interfaces such as:
-   * - ContentProvider
-   * - IndexData
-   * - Item
+   * - zim::writer::ContentProvider
+   * - zim::writer::IndexData
+   * - zim::writer::Item
    *
-   * If a incoherence has been detected in those implementations a
+   * If incoherence has been detected in those implementations an
    * `IncoherentImplementationError` will be thrown.
    */
   class LIBZIM_API IncoherentImplementationError : public CreatorError
@@ -91,7 +96,26 @@ namespace zim
       {}
   };
 
-  /* Exception thrown in the main thread when another exception has been
+  /**
+   * Exception thrown when there are problems indexing a title.
+   *
+   * In the current implementation, the only situation deliberately targeted by
+   * this type of error is when the title appears to contain a word that
+   * exceeds the limit on the longest indexable word (as defined by
+   * `MAX_INDEXABLE_TITLE_WORD_SIZE` in `src/constants.h`) but - due to hacky
+   * implementation - titles containing too much whitespace and/or punctuation
+   * may also trigger this error.
+   */
+  class LIBZIM_API TitleIndexingError : public CreatorError
+  {
+    public:
+      explicit TitleIndexingError(const std::string& message)
+       : CreatorError(message)
+      {}
+  };
+
+  /**
+   * Exception thrown in the main thread when another exception has been
    * thrown in another worker thread.
    *
    * Creator uses different worker threads to do background work.
@@ -144,10 +168,12 @@ namespace zim
       }
   };
 
-  /* Exception thrown when the creator is in error state.
+  /**
+   * Exception thrown when the creator is in error state.
    *
-   * If the creator is in error state (mostly because a AsyncError has already
-   * being thrown), any call to any method on it will thrown a `CreatorStateError`.
+   * If the creator is in error state (mostly because an AsyncError has already
+   * been thrown), any call to any method on it will throw a
+   * `CreatorStateError`.
    */
   class LIBZIM_API CreatorStateError : public CreatorError
   {

include/zim/writer/creator.h

@@ -43,24 +43,40 @@ namespace zim
      * Elements of the zim file can be added using the `add*` methods.
      * The final steps is to call `finishZimCreation`.
      *
-     * During the creation of the zim file (and before the call to `finishZimCreation`),
-     * some values must be set using the `set*` methods.
+     * During the creation of the zim file (and before the call to
+     * `finishZimCreation`), some values must be set using the `set*` methods.
+     *
+     * All `add*` methods and `finishZimCreation` can throw an exception.
+     * (most of the time - though not necessarily - a subclass of
+     * zim::CreatorError). It is up to the user to catch this exception and
+     * handle the error.
+     *
+     * The current (documented) conditions when an exception is thrown are:
+     *
+     * - When an entry cannot be added (mainly because an entry with the same
+     *   path has already been added) a `zim::InvalidEntry` will be thrown. The
+     *   creator will still be in a valid state and the creation can continue.
+     *
+     * - When there are problems indexing the title a `zim::TitleIndexingError`
+     *   is thrown. The creator will remain in a valid state, however it is not
+     *   clear whether continuing the creation will result in a perfectly valid
+     *   ZIM archive.
+     *
+     * - An exception has been thrown in a worker thread. This exception will
+     *   be caught and rethrown via a `zim::AsyncError`. The creator will
+     *   be put in an invalid state and creation cannot continue.
+     *
+     * - The creator is in error state.  A `zim::CreatorStateError` will be
+     *   thrown.
+     *
+     * - Any exception thrown by the user implementation itself. Note that if
+     *   this exception is thrown in a worker thread it will be exposed via an
+     *   AsyncError.
      *
-     * All `add*` methods and `finishZimCreation` can throw a exception.
-     * (most of the time zim::CreatorError child but not limited to)
-     * It is up to the user to catch this exception and handle the error.
-     * The current (documented) conditions when a exception is thrown are:
-     * - When a entry cannot be added (mainly because a entry with the same path has already been added)
-     *    A `zim::InvalidEntry` will be thrown. The creator will still be in a valid state and the creation can continue.
-     * - An exception has been thrown in a worker thread.
-     *    This exception will be catch and rethrown through a `zim::AsyncError`.
-     *    The creator will be set in a invalid state and creation cannot continue.
-     * - The creator is in error state.
-     *    A `zim::CreatorStateError` will be thrown.
-     * - Any exception thrown by user implementation itself.
-     *    Note that this exception may be thrown in a worker thread and so being "catch" by a AsyncError.
      * - Any other exception thrown for unknown reason.
-     * By default, creator status is not changed by thrown exception and creation should stop.
+     *
+     * By default, creator status is not changed by thrown exception and
+     * creation should stop.
      */
     class LIBZIM_API Creator
     {

src/constants.h

@@ -20,3 +20,11 @@
 #define ANCHOR_TERM "0posanchor "
 
 #define DEFAULT_CLUSTER_SIZE 2*1024*1024
+
+// The size in bytes of the longest word that is indexable in a title.
+// Xapian's default value is 64 while the hard limit is 245, however crashes
+// have been observed with values as low as 150 (demonstrated by the unit test
+// Suggestion.handlingOfTooLongWords in test/suggestion.cpp).
+// Note that a similar limit applies to full-text indexing but we don't
+// provide a way to control it (so it is at Xapian's default value of 64)
+#define MAX_INDEXABLE_TITLE_WORD_SIZE 64

src/writer/xapianIndexer.cpp

@@ -24,6 +24,7 @@
 #include "fs.h"
 #include "tools.h"
 #include "../constants.h"
+#include <zim/error.h>
 #include <sstream>
 #include <fstream>
 #include <stdexcept>
@@ -116,11 +117,30 @@ size_t getTermCount(const Xapian::Document& d)
   return std::distance(d.termlist_begin(), d.termlist_end());
 }
 
+size_t sizeOfIndexedText(const Xapian::Document& d)
+{
+  size_t n = 0;
+  for (auto termIt = d.termlist_begin(); termIt != d.termlist_end(); ++termIt) {
+    const std::string& term = *termIt;
+    if ( term[0] != 'Z' ) {
+      n += termIt.get_wdf() * term.size();
+    }
+  }
+  return n;
+}
+
 } // unnamed namespace
 
 /*
- * For title index, index the full path with namespace as data of the document.
- * The targetPath in valuesmap will store the path without namespace.
+ * For title index, index the title with the full path (including the
+ * namespace) as data of the document. The targetPath in valuesmap will store
+ * the path without namespace.
+ *
+ * An exception is thrown if the total size of non-indexable text in the title
+ * exceeds MAX_INDEXABLE_TITLE_WORD_SIZE (this is intended to detect omission
+ * of too long words during indexing but may be triggered by excessive
+ * whitespace and/or punctuation as well).
+ *
  * TODO:
  * Currently for title index we are storing path twice (redirectPath/path in
  * valuesmap and path in index data). In the future, we want to keep only one of
@@ -130,9 +150,12 @@ size_t getTermCount(const Xapian::Document& d)
 
 void XapianIndexer::indexTitle(const std::string& path, const std::string& title, const std::string& targetPath)
 {
+  const size_t MAX_WORD_LENGTH = MAX_INDEXABLE_TITLE_WORD_SIZE;
+
   assert(indexingMode == IndexingMode::TITLE);
   Xapian::Stem stemmer;
   Xapian::TermGenerator indexer;
+  indexer.set_max_word_length(MAX_WORD_LENGTH);
   indexer.set_flags(Xapian::TermGenerator::FLAG_CJK_NGRAM);
   try {
     stemmer = Xapian::Stem(stemmer_language);
@@ -158,11 +181,16 @@ void XapianIndexer::indexTitle(const std::string& path, const std::string& title
   if (!unaccentedTitle.empty()) {
     std::string anchoredTitle = ANCHOR_TERM + unaccentedTitle;
     indexer.index_text(anchoredTitle, 1);
+    if ( anchoredTitle.size() >= sizeOfIndexedText(currentDocument) + MAX_WORD_LENGTH ) {
+      throw zim::TitleIndexingError("Too much loss of data during title indexing");
+    }
     if ( getTermCount(currentDocument) == 1 ) {
       // only ANCHOR_TERM was added, hence unaccentedTitle is made solely of
       // non-word characters. Then add entire title as a single term.
       currentDocument.remove_term(*currentDocument.termlist_begin());
-      currentDocument.add_term(unaccentedTitle);
+      if ( unaccentedTitle.size() <= MAX_WORD_LENGTH ) {
+        currentDocument.add_term(unaccentedTitle);
+      }
     }
   }
 

test/suggestion.cpp

@@ -22,11 +22,13 @@
 #include <zim/archive.h>
 #include <zim/suggestion.h>
 #include <zim/item.h>
+#include <zim/error.h>
 
 #include <xapian.h>
 
 #include "tools.h"
 #include "../src/tools.h"
+#include "../src/constants.h"
 
 #include "gtest/gtest.h"
 
@@ -695,13 +697,72 @@ TEST(Suggestion, CJK) {
   );
 }
 
+std::string makeLongWord(size_t n) {
+  std::ostringstream oss;
+  oss << "awordthatis" << n << "characterslong";
+  const std::string s = oss.str();
+  if ( s.size() > n )
+    throw std::runtime_error("That is not a request for a long enough word!");
+
+  return s + std::string(n - s.size(), s.back());
+}
+
+void createASingleEntryZimArchive(const std::string& title)
+{
+  TempZimArchiveMadeOfEmptyHtmlArticles tza("en", {{ "path", title}});
+}
+
+const size_t MAX_WORD_LENGTH = MAX_INDEXABLE_TITLE_WORD_SIZE;
+
+TEST(Suggestion, handlingOfTooLongWords) {
+  const std::string shortOfBeingTooLong = makeLongWord(MAX_WORD_LENGTH);
+  const std::string tooLong = makeLongWord(MAX_WORD_LENGTH+1);
+
+  std::vector<std::string> titlesWithTooMuchDiscardableStuff{
+        tooLong,
+        "Is " + tooLong + " too long?",
+        ";-) " + tooLong,
+        "too much whitespace"  + std::string(MAX_WORD_LENGTH, ' '),
+        "too much punctuation" + std::string(MAX_WORD_LENGTH, '!'),
+  };
+
+  for ( const std::string& title : titlesWithTooMuchDiscardableStuff ) {
+    EXPECT_THROW(createASingleEntryZimArchive(title), zim::TitleIndexingError)
+      << "title: " << title;
+  }
+
+  TempZimArchiveMadeOfEmptyHtmlArticles tza("en", {
+     // { path                , title   }
+        { "path1", shortOfBeingTooLong                             },
+        { "path2", "Is " + shortOfBeingTooLong + " too long?"      },
+        { "path3", shortOfBeingTooLong + " " + shortOfBeingTooLong },
+  });
+
+  zim::Archive archive(tza.getPath());
+  EXPECT_SUGGESTED_TITLES(archive, "long",
+      "Is " + shortOfBeingTooLong + " too long?"
+  );
+
+  EXPECT_SUGGESTED_TITLES(archive, "awordthatis",
+      shortOfBeingTooLong + " " + shortOfBeingTooLong,
+      shortOfBeingTooLong,
+      "Is " + shortOfBeingTooLong + " too long?"
+  );
+}
+
 TEST(Suggestion, titleEdgeCases) {
   TempZimArchiveMadeOfEmptyHtmlArticles tza("en", {
      // { path     , title   }
 
         { "About"  , "About" }, // Title identical to path
         { "Trout"  , "trout" }, // Title differing from path in case only
         { "Without", ""      }, // No title
+                                //
+        // Handling of pseudo-words consisting exclusively of punctuation
+        { "winknsmilewithouttext",          ";-)" }, // A punctuation-only title
+        { "winknsmilebothways",             ";-) wink'n'smile" },
+        { "winknsmiletheotherwayaround",    "wink'n'smile ;-)" },
+        { "winknsmilewithothernonwords",    "~~ ;-) ~~"        },
 
         // Non edge cases
         { "Stout",   "About Rex Stout" },
@@ -727,6 +788,24 @@ TEST(Suggestion, titleEdgeCases) {
   EXPECT_SUGGESTED_TITLES(archive, "hang"
       /* nothing */
   );
+
+  EXPECT_SUGGESTED_TITLES(archive, ";-",
+      ";-)",
+      // The following results aren't included because ";-)" isn't treated as a
+      // term in the presence of anything else:
+      // - ";-) wink'n'smile"
+      // - "wink'n'smile ;-)"
+      // - "~~ ;-) ~~"
+  );
+
+  EXPECT_SUGGESTED_TITLES(archive, "win",
+      ";-) wink'n'smile",
+      "wink'n'smile ;-)"
+  );
+
+  EXPECT_SUGGESTED_TITLES(archive, "smile",
+      /* nothing */ // smile in "wink'n'smile" isn't a separate term
+  );
 }
 
 zim::Entry getTitleIndexEntry(const zim::Archive& a)