Review fixes

lib/Epub/Epub/css/CssParser.cpp

@@ -52,6 +52,29 @@ constexpr size_t MAX_SELECTOR_LENGTH = 256;
 // Check if character is CSS whitespace
 bool isCssWhitespace(const char c) { return c == ' ' || c == '\t' || c == '\n' || c == '\r' || c == '\f'; }
 
+std::string_view stripTrailingImportant(std::string_view value) {
+  constexpr std::string_view IMPORTANT = "!important";
+
+  while (!value.empty() && isCssWhitespace(value.back())) {
+    value.remove_suffix(1);
+  }
+
+  if (value.size() < IMPORTANT.size()) {
+    return value;
+  }
+
+  const size_t suffixPos = value.size() - IMPORTANT.size();
+  if (value.substr(suffixPos) != IMPORTANT) {
+    return value;
+  }
+
+  value.remove_suffix(IMPORTANT.size());
+  while (!value.empty() && isCssWhitespace(value.back())) {
+    value.remove_suffix(1);
+  }
+  return value;
+}
+
 }  // anonymous namespace
 
 // String utilities implementation
@@ -318,7 +341,8 @@ void CssParser::parseDeclarationIntoStyle(const std::string& decl, CssStyle& sty
       style.defined.imageWidth = 1;
     }
   } else if (propNameBuf == "display") {
-    style.display = (propValueBuf == "none") ? CssDisplay::None : CssDisplay::Block;
+    const std::string_view displayValue = stripTrailingImportant(propValueBuf);
+    style.display = (displayValue == "none") ? CssDisplay::None : CssDisplay::Block;
     style.defined.display = 1;
   }
 }
@@ -753,16 +777,44 @@ bool CssParser::loadFromCache() {
     return false;
   }
 
+  if (ruleCount > MAX_RULES) {
+    LOG_DBG("CSS", "Invalid cache rule count (%u > %zu)", ruleCount, MAX_RULES);
+    rulesBySelector_.clear();
+    file.close();
+    return false;
+  }
+
+  auto hasRemainingBytes = [&file](const size_t neededBytes) -> bool {
+    return static_cast<size_t>(file.available()) >= neededBytes;
+  };
+
+  constexpr size_t CSS_LENGTH_FIELD_COUNT = 11;
+  constexpr size_t CSS_LENGTH_BYTES = sizeof(float) + sizeof(uint8_t);
+  constexpr size_t CSS_FIXED_STYLE_BYTES =
+      4 * sizeof(uint8_t) + (CSS_LENGTH_FIELD_COUNT * CSS_LENGTH_BYTES) + sizeof(uint8_t) + sizeof(uint16_t);
+
   // Read each rule
   for (uint16_t i = 0; i < ruleCount; ++i) {
     // Read selector string
     uint16_t selectorLen = 0;
+    if (!hasRemainingBytes(sizeof(selectorLen))) {
+      rulesBySelector_.clear();
+      file.close();
+      return false;
+    }
     if (file.read(&selectorLen, sizeof(selectorLen)) != sizeof(selectorLen)) {
       rulesBySelector_.clear();
       file.close();
       return false;
     }
 
+    if (selectorLen == 0 || selectorLen > MAX_SELECTOR_LENGTH || !hasRemainingBytes(selectorLen)) {
+      LOG_DBG("CSS", "Invalid selector length in cache: %u", selectorLen);
+      rulesBySelector_.clear();
+      file.close();
+      return false;
+    }
+
     std::string selector;
     selector.resize(selectorLen);
     if (file.read(&selector[0], selectorLen) != selectorLen) {
@@ -771,6 +823,13 @@ bool CssParser::loadFromCache() {
       return false;
     }
 
+    if (!hasRemainingBytes(CSS_FIXED_STYLE_BYTES)) {
+      LOG_DBG("CSS", "Truncated CSS cache while reading style payload");
+      rulesBySelector_.clear();
+      file.close();
+      return false;
+    }
+
     // Read CssStyle fields
     CssStyle style;
     uint8_t enumVal;

lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp

@@ -182,6 +182,24 @@ void XMLCALL ChapterHtmlSlimParser::startElement(void* userData, const XML_Char*
   centeredBlockStyle.textAlignDefined = true;
   centeredBlockStyle.alignment = CssTextAlign::Center;
 
+  // Compute CSS style for this element early so display:none can short-circuit
+  // before tag-specific branches emit any content or metadata.
+  CssStyle cssStyle;
+  if (self->cssParser) {
+    cssStyle = self->cssParser->resolveStyle(name, classAttr);
+    if (!styleAttr.empty()) {
+      CssStyle inlineStyle = CssParser::parseInlineStyle(styleAttr);
+      cssStyle.applyOver(inlineStyle);
+    }
+  }
+
+  // Skip elements with display:none before all fast paths (tables, links, etc.).
+  if (cssStyle.hasDisplay() && cssStyle.display == CssDisplay::None) {
+    self->skipUntilDepth = self->depth;
+    self->depth += 1;
+    return;
+  }
+
   // Special handling for tables/cells: flatten into per-cell paragraphs with a prefixed header.
   if (strcmp(name, "table") == 0) {
     // skip nested tables
@@ -402,7 +420,8 @@ void XMLCALL ChapterHtmlSlimParser::startElement(void* userData, const XML_Char*
                   self->flushPartWordBuffer();
                 }
                 if (self->currentTextBlock && !self->currentTextBlock->isEmpty()) {
-                  self->startNewTextBlock(BlockStyle());
+                  const BlockStyle parentBlockStyle = self->currentTextBlock->getBlockStyle();
+                  self->startNewTextBlock(parentBlockStyle);
                 }
 
                 // Create page for image - only break if image won't fit remaining space
@@ -535,25 +554,6 @@ void XMLCALL ChapterHtmlSlimParser::startElement(void* userData, const XML_Char*
     }
   }
 
-  // Compute CSS style for this element
-  CssStyle cssStyle;
-  if (self->cssParser) {
-    // Get combined tag + class styles
-    cssStyle = self->cssParser->resolveStyle(name, classAttr);
-    // Merge inline style (highest priority)
-    if (!styleAttr.empty()) {
-      CssStyle inlineStyle = CssParser::parseInlineStyle(styleAttr);
-      cssStyle.applyOver(inlineStyle);
-    }
-  }
-
-  // Skip elements with display:none
-  if (cssStyle.hasDisplay() && cssStyle.display == CssDisplay::None) {
-    self->skipUntilDepth = self->depth;
-    self->depth += 1;
-    return;
-  }
-
   const float emSize = static_cast<float>(self->renderer.getFontAscenderSize(self->fontId));
   const auto userAlignmentBlockStyle = BlockStyle::fromCssStyle(
       cssStyle, emSize, static_cast<CssTextAlign>(self->paragraphAlignment), self->viewportWidth);