OPENNLP-1909: General verified installer for user-supplied third-party resources - #1211
Conversation
|
I've been testing this functionality heavily via the grpc service (there's a webpage on the FE that lets you download supported models). So far, so good. |
# Conflicts: # opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/DownloadUtil.java
bca31fc to
b1b3803
Compare
# Conflicts: # dev/README-mecab-dictionaries.md # opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/stemmer/hunspell/HunspellDictionaryDownload.java # opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/tokenize/lattice/MecabDictionaryInstaller.java # opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/DictionaryCatalog.java # opennlp-core/opennlp-runtime/src/main/java/opennlp/tools/util/ResourceInstaller.java # opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/stemmer/hunspell/HunspellDictionaryDownloadTest.java # opennlp-core/opennlp-runtime/src/test/java/opennlp/tools/tokenize/lattice/MecabDictionaryInstallerTest.java # opennlp-docs/src/docbkx/stemmer.xml # opennlp-docs/src/docbkx/tokenizer.xml
|
Please rebase and fix conflicts @krickert |
b1b3803 to
21caf14
Compare
| final List<Path> stagedFiles = new ArrayList<>(); | ||
| final List<Path> publishedFiles = new ArrayList<>(); | ||
| boolean published = false; | ||
| final Path unpacked = Files.createTempDirectory(TEMP_DIRECTORY_PREFIX); |
There was a problem hiding this comment.
Install is staged in the system temp directory, not on the target filesystem.
Files.createTempDirectory(TEMP_DIRECTORY_PREFIX) creates unpacked under java.io.tmpdir and then hands it to ResourceInstaller as the target, so both the download file and the staging tree land in the system temp directory. That defeats the stated design of ResourceInstaller.createDownloadFile ("placed on the target's filesystem, not in the system temporary directory, so a large download cannot exhaust the system temporary directory while the target has room"), and it is a regression from the code this PR removes, which the old tokenizer docs described as "Files are staged on the target filesystem and published after the archive passes validation."
Scenario: MecabDictionaryInstaller.install(uri, dictDir, sha) for a 900 MB UniDic archive writes ~900 MB of download plus up to maxExpandedBytes (4 GiB by default) of unpacked content into /tmp, even though dictDir has plenty of room. On a distro where /tmp is a tmpfs — the default on systemd systems — that is RAM, so a legitimate large dictionary install can push the machine to OOM. The same applies at line 152 in installFromCatalog.
| * @throws IOException Thrown if decompressing or unpacking fails or a limit is | ||
| * exceeded. | ||
| */ | ||
| private static void unpackGzip(InputStream raw, String name, Path staging, |
There was a problem hiding this comment.
The gzip expansion-ratio guard was dropped and nothing replaced it.
MecabDictionaryInstaller.MAX_GZIP_EXPANSION_RATIO = 100 is deleted in this PR, and unpackGzip now bounds decompression only by the absolute maxExpandedBytes ceiling — which this PR simultaneously raises from 2 GiB (ResourceLimits.MAX_ARCHIVE_TOTAL_BYTES) to 4 GiB.
Scenario: a 10 KB .tar.gz consisting of zero bytes decompresses to ~4 GiB. The old code aborted at ~1 MB (100x the compressed size); the new code streams and writes ~4 GiB to disk before Budget.spend throws. Combined with MecabDictionaryInstaller staging in java.io.tmpdir, that is 4 GiB into /tmp from a 10 KB input — on the install(URI, Path) overload that accepts an unverified file: archive with no checksum at all.
The size of downloaded is known before unpack runs, so a ratio check is cheap to reinstate here.
| for (final Path file : files) { | ||
| final Path destination = destination(target, staging.relativize(file)); | ||
| try { | ||
| Files.move(file, destination, StandardCopyOption.ATOMIC_MOVE); |
There was a problem hiding this comment.
ATOMIC_MOVE silently overwrites an existing destination on POSIX, so the "does not replace" guarantee is not enforced at the move.
Files.move(..., ATOMIC_MOVE) without REPLACE_EXISTING does not throw FileAlreadyExistsException on Unix: sun.nio.fs.UnixCopyFile.move takes the flags.atomicMove branch and calls rename(2) directly, which replaces the target. I confirmed this on a stock JDK — moving over an existing file with ATOMIC_MOVE replaced it, no exception. So the class-level promise "Promotion does not replace a file that already exists in the target" rests entirely on the ensureVacant pre-pass at line 826, with nothing backing it at the move itself.
Scenario: two installs run into the same target directory (two threads, or two JVMs). Both complete their ensureVacant pass while the destination is still free; the second promote then silently clobbers the file the first just published, instead of failing.
Note that the fallback at line 833 (plain Files.move) does fail loudly, so the two branches have different semantics for the same input. Using ATOMIC_MOVE only where absence is proven — or dropping it in favour of the non-atomic move, which enforces the invariant — would make the guarantee real rather than advisory.
| public static final String MAX_DOWNLOAD_BYTES_PROPERTY = "opennlp.download.max.bytes"; | ||
|
|
||
| /** The system property overriding the default expansion limit in bytes. */ | ||
| public static final String MAX_EXPANDED_BYTES_PROPERTY = |
There was a problem hiding this comment.
opennlp.install.max.total.bytes is now defined twice, with two different meanings and defaults.
This constant claims the property for ResourceInstaller.Limits with a 4 GiB default, while opennlp-api/src/main/java/opennlp/tools/util/ResourceLimits.java:70 still declares MAX_ARCHIVE_TOTAL_BYTES_PROPERTY = "opennlp.install.max.total.bytes" with a 2 GiB default and javadoc describing it as "Maximum total size extracted from one archive". After this PR nothing reads the ResourceLimits pair any more — MAX_ARCHIVE_ENTRY_BYTES / opennlp.install.max.entry.bytes and MAX_ARCHIVE_TOTAL_BYTES are dead public API — but their javadoc still documents the old semantics.
Scenario: a user reads ResourceLimits, sets -Dopennlp.install.max.total.bytes=3221225472 expecting a per-archive extraction cap of 3 GiB, and instead silently changes ResourceInstaller's expansion limit, while ResourceLimits's own constant (which nothing consumes) also shifts.
Either delete the two dead ResourceLimits constants, or point their javadoc at ResourceInstaller.Limits.
| } else if (hasMagic(magic, ZIP_MAGIC_FIRST, ZIP_MAGIC_SECOND, | ||
| ZIP_LOCAL_HEADER_THIRD, ZIP_LOCAL_HEADER_FOURTH)) { | ||
| unpackZip(raw, staging, budget, entryBudget); | ||
| validateZip(downloaded); |
There was a problem hiding this comment.
Zip content is extracted from the local file headers but validated only against the central directory.
unpackZip above reads entries through ZipInputStream, i.e. from the streamed local file headers, while validateZip here only checks that a well-formed central directory exists by constructing a ZipFile. The two entry sets are never cross-checked.
Scenario: an archive whose central directory lists a benign set of entries while its local headers carry a different set passes validateZip and installs the local-header content — the classic zip-ambiguity divergence. The mandatory checksum on http/https sources mitigates this, but install(URI, Path) accepts an unverified file: archive, so the divergence is reachable.
Extracting via ZipFile's central-directory entries, or comparing the two name sets before promotion, would close it.
| final String resourceName = validateSourceName( | ||
| name == null ? sourceName(source) : name); | ||
| Files.createDirectories(targetDirectory); | ||
| final Path downloaded = createDownloadFile(targetDirectory); |
There was a problem hiding this comment.
A killed JVM leaves partially unpacked content permanently in the caller's target directory, and nothing ever cleans it.
Files.createDirectories(targetDirectory) and createDownloadFile(targetDirectory) run before the source is contacted, and installStaged creates .opennlp-staging<random>/ inside the target as well. The finally at line 497 removes only the download file, and installStaged's cleanup only runs on a normal return or a caught IOException.
Scenario: the process is killed (or an Error unwinds) part-way through unpack. A .opennlp-staging<random>/ directory holding partially unpacked third-party content — plus a .opennlp-download<random>.part file — is left in the target directory. No later run inspects or removes them, so each retry adds another copy. A failed install also leaves a freshly created empty target directory behind.
| raw.mark(MAGIC_LENGTH); | ||
| final byte[] magic = raw.readNBytes(MAGIC_LENGTH); | ||
| raw.reset(); | ||
| if (name.endsWith(MODEL_SUFFIX)) { |
There was a problem hiding this comment.
The two name-driven rules are case-sensitive.
name.endsWith(MODEL_SUFFIX) here and name.endsWith(GZIP_SUFFIX) at line 1088 both compare exact case.
Scenario: a source URL or catalog filename ending in .BIN — routine on Windows-authored mirrors — is not recognised as an OpenNLP model, so byte detection sees the zip magic and unpackZip expands it, dropping manifest.properties and *.model into the target instead of storing the model packed. That is exactly the failure this rule exists to prevent, and the resulting directory will not load as a model.
Likewise a .GZ source keeps its suffix in the installed file name (corpus.txt.GZ holding decompressed content).
| } | ||
| if (expected != actual) { | ||
| throw new IOException("tar header checksum does not match"); | ||
| final Path unpacked = Files.createTempDirectory(TEMP_DIRECTORY_PREFIX); |
There was a problem hiding this comment.
Same as the comment on line 117: installFromCatalog also stages the whole install under java.io.tmpdir rather than on the target filesystem, so a catalog dictionary of a few hundred MB is downloaded and expanded into /tmp regardless of how much room targetDirectory has.
| <code>MecabDictionary</code>, and tokenize. | ||
| Install a dictionary archive with <code>MecabDictionaryInstaller</code>, load it | ||
| as a <code>MecabDictionary</code>, and tokenize. Remote archives require an | ||
| expected SHA-512 digest; an application-supplied opt-in catalog |
There was a problem hiding this comment.
"Remote archives require an expected SHA-512 digest" understates what is implemented: ResourceInstaller.validateChecksum accepts a 64-character SHA-256 digest as well as a 128-character SHA-512 one, and MecabDictionaryInstaller.install's own javadoc says "64 characters for SHA-256 or 128 for SHA-512".
Scenario: a reader following this sentence concludes a SHA-256 pin is not accepted and goes looking for a SHA-512 they may not have. The model-loading chapter added in this PR states it correctly ("Http and https sources require a SHA-256 or SHA-512 checksum"); this line should match.
|
Add bounded downloads, SHA-512 verification, staged publication, tar and zip extraction, catalog entries, and adapters for the Hunspell and MeCab features merged through #1190 and #1191. Red evidence recorded while developing and reviewing this branch covered unsafe redirects and schemes, archive traversal and expansion limits, incomplete tar data, checksum failures, replacement of existing files, malformed catalog values, and validation order.
Red evidence on the rebased branch: ZIP extraction on a non-default file system threw UnsupportedOperationException, catalog filenames accepted empty and path-like values, tar metadata bypassed maxEntries, and null timeout messages did not match the public validation contract.
Validate catalog filenames, support ZIP archives on non-default file systems, and count tar extension headers against the entry limit. Align timeout errors and update the installer documentation.
ResourceInstaller walked the directories below the target twice with the same link and file checks, once to prove a destination vacant and once to create the path for the move; one helper now does both. The lattice tests kept a second copy of the gzip tar builders that the installer tests had, so TarArchives owns them and both packages call it. The MeCab README names the catalog example as the test resource it is.
The per-feature download path that #1190 and #1191 added to DownloadUtil (the download(URI, Path, sha512) method, the opennlp.download.remote gate, the byte ceiling and its property, and configuredLimit) merged with those PRs and is deleted here, with its test. DictionaryCatalog owns the gate and installs through ResourceInstaller, and the byte ceilings live in ResourceInstaller.Limits, so nothing in the tree called the old path. Model downloads and the cached-model checksum verification are untouched.
…listings (failing tests) Red evidence on the rebased branch: an atomic move replaced an existing destination on POSIX, so the vacancy check was advisory; a 4 MiB gzip of zeros expanded in full because no ratio bounded it; *.BIN and *.GZ names did not match the lower-case rules; a download file and a staging directory left by a killed installation stayed in the target forever; a failed first installation left the target directory it created; and a zip whose local headers and central directory listed different files installed the local-header content. The MeCab installer also left a stale scratch directory in the target. The move is extracted into a helper so the promotion guarantee can be tested on its own.
…nd enforce vacancy at the move The MeCab installer unpacks into a hidden scratch directory beneath the target instead of the system temporary directory, so the download, the unpacked tree, and the installed files share one filesystem; stale scratch directories from a killed run are removed first. Gzip content is bounded to 100 times its compressed size, with a 1 MiB floor for small sources, before the absolute expansion limit applies. The promotion move is a plain move, since an atomic move renames over an existing file on POSIX and made the vacancy check advisory. Zip file names read from the local headers must match the central directory's file entries, on the default filesystem through ZipFile and elsewhere through the zip filesystem. The *.bin and *.gz name rules match in any letter case. Work files left in the target by a killed installation are removed at the next installation into it, and a failed installation removes the empty target directory it created; the class documents that concurrent installations into one target are not supported. The two ResourceLimits archive constants and their properties, which nothing read after this change, are deleted so the expansion property has one meaning. The tokenizer manual names both accepted digests, and the model loading chapter states the ratio rule.
362e4b8 to
4c299bb
Compare
The expansion-ratio guard applied only to gzip content, so a zip archive was bounded by the absolute expansion limit alone. Deflate reaches roughly 1000 to 1, so a few megabytes of crafted zip expanded to the full 4 GiB ceiling in the caller's target directory, and install(URI, Path) accepts a file: archive with no checksum at all. The ratio now applies to every compressed source: gzip and zip alike expand to at most 100 times the compressed size, with the 1 MiB floor for small sources.
a3bb247 to
d0f33e6
Compare
The ratio was a private constant, so 100 to 1 was a hard ceiling no caller could lift. Raising opennlp.install.max.total.bytes does not help, because the ratio and the absolute limit are enforced independently and the tighter one applies, so a resource that legitimately compresses better than 100 to 1 could not be installed at all. maxExpansionRatio joins the other six values on Limits, with the builder method and the opennlp.install.max.expansion.ratio property the three byte and entry ceilings already have, and must be positive like them. The manual states that the ratio and the expansion limit are separate, since raising the byte limit alone reads like the fix and is not.
Adds a general installer for user-supplied third-party resources: training corpora, dictionary archives, and lexicons that the project cannot bundle. The caller supplies the location and thereby accepts that resource's license; no locations are built in and no data ships with OpenNLP.
ResourceInstalleris a single hardened download-and-unpack path:http,https, andfilelocations are accepted, checked at the public boundary. Remote fetches use connection and read timeouts, follow a bounded number of redirects, and reject redirects that leave the http/https schemes or downgrade https to http.httpandhttpssources and optional forfilesources, which are trusted caller input; it is verified against the downloaded bytes before anything is unpacked: a 64-character hex digest selects SHA-256, a 128-character one SHA-512.Limitsvalue (download bytes, expanded bytes, entry count);Limits.DEFAULTapplies when none is given andLimits.builder()starts from it. The three default ceilings can be raised at JVM startup viaopennlp.download.max.bytes,opennlp.install.max.total.bytes, andopennlp.install.max.entries.*.binsources are stored packed, because OpenNLP model files are zip archives their consumers load packed.The tar reading is a small forward-only reader,
TarStream, not a dependency. It reads classic v7, POSIX ustar, GNU, and pax formats with a valid header checksum required in every case, honors the ustar name prefix field, reads paxx(path, size) and GNULlong-name extension headers, and reads GNU base-256 sizes for entries of 8 GiB or more. Sparse entries are rejected because their archived bytes are not the file content, and a pax global header withpathorsizeis rejected because it would rewrite each following entry. Metadata expansion (extension header sizes) is bounded like everything else.Every behavior above is pinned by tests, in failing-test-then-fix commit pairs.
Rebased on main after #1190 and #1191 merged, so the diff is the installer plus the convergence discussed on those PRs: the per-feature download code added to them during review is deleted.
DownloadUtilreturns to its model-only surface,DictionaryCatalogowns theopennlp.download.remoteopt-in gate and installs throughResourceInstaller, andMecabDictionaryInstallerkeeps only its payload selection while delegating fetch, verification, and unpacking (gaining pax and GNU long-name support).