diff --git a/CHANGELOG.md b/CHANGELOG.md index f3ceb3699f..618f796100 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -85,6 +85,12 @@ All notable changes to this project will be documented in this file. Take a look * The deprecated `ReadiumAdapterGCDWebServer` and `ReadiumAdapterLCPSQLite` adapter packages have been removed. * The `ReadiumInternal` package has been removed. Its utilities were internal helpers and are now folded into `ReadiumShared` with `package` visibility. If you imported `ReadiumInternal` directly, remove the import. +### Fixed + +#### Shared + +* [#876](https://github.com/readium/swift-toolkit/issues/876) Fixed a crash (`String index is out of bounds`) in `ContentSearchService` when searching a publication containing characters merging with the surrounding text, such as combining diacritical marks. + diff --git a/Sources/Shared/Publication/Services/Search/ContentSearchService.swift b/Sources/Shared/Publication/Services/Search/ContentSearchService.swift index 9333a43bd4..d410453192 100644 --- a/Sources/Shared/Publication/Services/Search/ContentSearchService.swift +++ b/Sources/Shared/Publication/Services/Search/ContentSearchService.swift @@ -314,18 +314,24 @@ private actor Iterator: SearchIterator, Loggable { guard !entryText.isEmpty else { return } - let startOffset: Int - if windowText.isEmpty { - startOffset = 0 - } else { + if !windowText.isEmpty { // Space separator owned by this (following) entry. windowText.append(" ") - windowTextCount += 1 - startOffset = windowTextCount } - windowText.append(contentsOf: entryText) - windowTextCount += entryText.count + + // `windowTextCount` is recomputed from `windowText` instead of adding + // the appended counts: Swift regroups grapheme clusters across the + // junction (e.g. an entry starting with a combining mark merges with + // the separator space), so the sum could exceed the real character + // count and offset arithmetic would fall out of the window's bounds. + // + // For the same reason, `startOffset` is derived from the final count + // rather than from the separator's position: when the separator merged + // with the entry's first character, the merged cluster belongs to the + // entry. This keeps `startOffset + text.count == windowTextCount`. + windowTextCount = windowText.count + let startOffset = windowTextCount - entryText.count entries.append(ElementEntry( text: entryText, @@ -334,6 +340,26 @@ private actor Iterator: SearchIterator, Loggable { )) } + /// Returns the index in `windowText` at the given character offset. + /// + /// Offsets are derived from `windowTextCount`, which is recomputed from + /// `windowText` on every mutation of the window, so they are expected to + /// be in bounds. As a safety net against a crash in a user's library (see + /// issue #876), an out-of-bounds offset is logged and returns `nil` + /// instead of trapping. + private func windowIndex(at offset: Int) -> String.Index? { + guard + // `limitedBy` only bounds the forward walk: a negative offset would + // still trap. + offset >= 0, + let index = windowText.index(windowText.startIndex, offsetBy: offset, limitedBy: windowText.endIndex) + else { + log(.error, "Window offset \(offset) is out of bounds (window count: \(windowText.count))") + return nil + } + return index + } + /// Resets the window for a new resource. private func resetWindow() { entries = [] @@ -421,7 +447,10 @@ private actor Iterator: SearchIterator, Loggable { trimAmount = 0 } - guard trimAmount > 0 else { return } + guard + trimAmount > 0, + let trimIdx = windowIndex(at: trimAmount) + else { return } // Drop leading entries. entries.removeFirst(dropCount) @@ -434,9 +463,8 @@ private actor Iterator: SearchIterator, Loggable { searchCeiling -= trimAmount // Drop prefix from windowText. - let trimIdx = windowText.index(windowText.startIndex, offsetBy: trimAmount) windowText = String(windowText[trimIdx...]) - windowTextCount -= trimAmount + windowTextCount = windowText.count isAtResourceStart = false } @@ -462,8 +490,12 @@ private actor Iterator: SearchIterator, Loggable { private func search(dangerZoneCapacity: Int) async -> [Locator] { guard searchCeiling > searchFloor else { return [] } - let sliceStart = windowText.index(windowText.startIndex, offsetBy: searchFloor) - let sliceEnd = windowText.index(windowText.startIndex, offsetBy: searchCeiling) + guard + let sliceStart = windowIndex(at: searchFloor), + let sliceEnd = windowIndex(at: searchCeiling) + else { + return [] + } let searchSlice = String(windowText[sliceStart ..< sliceEnd]) let ranges = await searchAlgorithm.findRanges( @@ -507,8 +539,13 @@ private actor Iterator: SearchIterator, Loggable { return nil } - let highlightStart = windowText.index(windowText.startIndex, offsetBy: matchStart) - let highlightEnd = windowText.index(windowText.startIndex, offsetBy: matchEnd) + guard + let highlightStart = windowIndex(at: matchStart), + let highlightEnd = windowIndex(at: matchEnd) + else { + return nil + } + let highlight = String( windowText[highlightStart ..< highlightEnd] ) @@ -563,11 +600,14 @@ private actor Iterator: SearchIterator, Loggable { /// Returns `nil` if the match is at the very beginning of a resource and /// the resulting text is empty after trimming. private func extractSnippetBefore(matchStart: Int) -> String? { - guard matchStart > 0 else { + guard + matchStart > 0, + let matchStartIndex = windowIndex(at: matchStart) + else { return nil } - let available = windowText[windowText.startIndex ..< windowText.index(windowText.startIndex, offsetBy: matchStart)] + let available = windowText[windowText.startIndex ..< matchStartIndex] var chars: [Character] = [] var count = snippetLength @@ -600,11 +640,13 @@ private actor Iterator: SearchIterator, Loggable { /// Returns `nil` if the match is at the very end of a resource and the /// resulting text is empty after trimming. private func extractSnippetAfter(matchEnd: Int) -> String? { - guard matchEnd < windowTextCount else { + guard + matchEnd < windowTextCount, + let afterStart = windowIndex(at: matchEnd) + else { return nil } - let afterStart = windowText.index(windowText.startIndex, offsetBy: matchEnd) let available = windowText[afterStart...] var result = "" diff --git a/Tests/SharedTests/Publication/Services/Search/ContentSearchServiceTests.swift b/Tests/SharedTests/Publication/Services/Search/ContentSearchServiceTests.swift new file mode 100644 index 0000000000..0b5cf6a025 --- /dev/null +++ b/Tests/SharedTests/Publication/Services/Search/ContentSearchServiceTests.swift @@ -0,0 +1,190 @@ +// +// Copyright 2026 Readium Foundation. All rights reserved. +// Use of this source code is governed by the BSD-style license +// available in the top-level LICENSE file of the project. +// + +@testable import ReadiumShared +import Testing + +enum ContentSearchServiceTests { + /// Regression tests for https://github.com/readium/swift-toolkit/issues/876 + /// + /// When an element's text begins with a character that merges with the + /// preceding grapheme cluster (e.g. a combining mark merging with the + /// window's space separator), the sliding window's cached character count + /// used to run ahead of the real `windowText.count`, and offset arithmetic + /// trapped with "String index is out of bounds". + struct GraphemeClusterBoundaries { + /// A publication made of one `TextContentElement` per array of segment + /// texts, containing the query "夜灯" twice. + struct Fixture: CustomTestStringConvertible { + let name: String + let elements: [[String]] + + var testDescription: String { + name + } + } + + static let fixtures: [Fixture] = [ + Fixture( + name: "plain content (control)", + elements: [ + ["夜灯亮着，街角安静。"], + ["猫在窗台上打盹，夜灯映出影子。"], + ] + ), + Fixture( + name: "element starting with a combining mark", + elements: [ + ["夜灯亮着，街角安静。"], + ["\u{0301}猫在窗台上打盹，夜灯映出影子。"], + ] + ), + Fixture( + name: "element starting with a variation selector", + elements: [ + ["夜灯亮着，街角安静。"], + ["\u{FE0F}猫在窗台上打盹，夜灯映出影子。"], + ] + ), + Fixture( + name: "segment starting with a combining mark", + elements: [ + ["夜灯亮着，街角安静", "\u{0301}，猫在窗台上打盹，夜灯映出影子。"], + ] + ), + ] + + @Test(arguments: fixtures) + func findsAllMatches(fixture: Fixture) async throws { + let results = try await search(query: "夜灯", elements: fixture.elements) + + #expect(results.count == 2) + #expect(results.allSatisfy { $0.text.highlight == "夜灯" }) + } + + /// The merged cluster shifts every character of the entry one position + /// back in the window. The entry's start offset must account for it, + /// otherwise a match is attributed to the wrong segment. + @Test func matchResolvesToTheOwningSegment() async throws { + let results = try await search(query: "夜灯", elements: [ + ["第一段。"], + ["\u{0301}abc", "夜灯xyz"], + ]) + + #expect(results.count == 1) + let result = try #require(results.first) + #expect(result.text.highlight == "夜灯") + #expect(result.locations.fragments == ["e1s1"]) + } + + @Test func snippetsAroundAMergedCluster() async throws { + let results = try await search(query: "abc", elements: [ + ["第一段。"], + ["\u{0301}abc def"], + ]) + + let result = try #require(results.first) + #expect(result.text.highlight == "abc") + #expect(result.text.before == "第一段。 \u{0301}") + #expect(result.text.after == "def") + } + + /// A long resource forces the window to be front-trimmed repeatedly, + /// which rebases every entry's offset. Merged clusters must not make + /// the offsets drift across trims. + @Test(arguments: ["\u{0301}", "\u{FE0F}", "\u{200C}"]) + func matchesSurviveWindowTrimming(mergingCharacter: String) async throws { + let elementCount = 100 + let results = try await search(query: "夜灯", elements: (0 ..< elementCount).map { _ in + ["\(mergingCharacter)猫在窗台上打盹，", "\(mergingCharacter)夜灯映出影子。"] + }) + + #expect(results.count == elementCount) + #expect(results.allSatisfy { $0.text.highlight == "夜灯" }) + #expect(results.map(\.locations.fragments) == (0 ..< elementCount).map { ["e\($0)s1"] }) + } + } +} + +// MARK: - Helpers + +/// Runs a search over a publication whose content is made of one +/// `TextContentElement` per array of segment texts. +private func search(query: String, elements segmentTexts: [[String]]) async throws -> [Locator] { + let locator = Locator(href: "chap1", mediaType: .html) + let elements: [ContentElement] = segmentTexts.enumerated().map { elementIndex, texts in + TextContentElement( + locator: locator, + role: .body, + segments: texts.enumerated().map { segmentIndex, text in + TextContentElement.Segment( + locator: locator.copy(locations: { $0.fragments = ["e\(elementIndex)s\(segmentIndex)"] }), + text: text + ) + } + ) + } + + let publication = Publication( + manifest: Manifest( + metadata: Metadata(title: ""), + readingOrder: [Link(href: "chap1", mediaType: .html)] + ), + servicesBuilder: PublicationServicesBuilder( + content: { _ in StubContentService(elements: elements) }, + search: ContentSearchService.makeFactory() + ) + ) + + let iterator = try await publication.search(query: query).get() + + var locators: [Locator] = [] + while let batch = try await iterator.next().get() { + locators.append(contentsOf: batch.locators) + } + return locators +} + +private final class StubContentService: ContentService { + private let elements: [ContentElement] + + init(elements: [ContentElement]) { + self.elements = elements + } + + func content(from start: Locator?) -> Content? { + StubContent(elements: elements) + } + + private struct StubContent: Content { + let elements: [ContentElement] + + func iterator() -> ContentIterator { + StubContentIterator(elements: elements) + } + } + + private actor StubContentIterator: ContentIterator { + private let elements: [ContentElement] + private var index = 0 + + init(elements: [ContentElement]) { + self.elements = elements + } + + func next() async throws -> ContentElement? { + guard index < elements.count else { + return nil + } + defer { index += 1 } + return elements[index] + } + + func previous() async throws -> ContentElement? { + nil + } + } +}