descriptor: enforce import visibility when linking a DescriptorPool - #428
Merged
Merged
Conversation
A type_name, extendee, or method input/output type must resolve to a definition in the referring file, a file in its dependency list, or a file those re-export through import public; anything else is PoolError::NotImported, matching protoc. Each message and enum records its defining file at registration, each new file's visible set is computed once (direct and weak deps, then the transitive public_dependency closure, across files already in the pool and files in the set being added), and every resolve site checks it. LinkOptions (via DescriptorPool::with_link_options) can switch enforcement off or additionally require every listed dependency to be present (PoolError::ImportNotFound); by default an absent, unreferenced import is tolerated so sets that strip option-only imports still load. Closes #423.
|
All contributors have signed the CLA ✍️ ✅ |
- LinkOptions follows DecodeOptions' shape: private fields, with_* setters, getters; DescriptorPool::decode_with_link_options is the one-call form. - Visible sets are built per file and per pass instead of tabulated for the whole batch (a public-import chain made the table quadratic in the file count on untrusted input). - Variants renamed TypeNotImported / DependencyNotFound; Display names the fix; filenames print bare like their neighbours. New DuplicateFileName for two same-named files in one set, which previously shadowed silently. - Missing weak dependencies are exempt from required_dependencies, as under protoc. - Docs: LinkOptions no longer steals DescriptorPool's summary line; add_file_descriptor_set states the cross-set rule; guide example; crate-root module list mentions pool/reflect; generated descriptor_pool() expect message says linkable. - Tests: enum-typed cross-file reference, reference into an absent import (UnresolvedTypeName), weak exemption, decode_with_link_options, duplicate file names.
The hand-built set referenced .alpha.v1.Alpha from beta.proto without a dependency entry, which the pool now rejects; the (cargo-spawning, ignored by default) shared_pool_compile tests failed in CI on it.
iainmcgin
marked this pull request as ready for review
September 6, 2026 17:22
azdagron
approved these changes
Sep 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DescriptorPoolresolved everytype_name,extendeeand method input/output type against one flat table of everything in the pool, sob.protocould use.a.Thingwithout importinga.protoand the set linked. protoc rejects that: a file sees itself, itsdependencylist, and whatever those re-export throughimport public, and nothing else.The pool now records each message's and enum's defining file at registration, computes each new file's visible set before linking (direct and weak dependencies, then the transitive
public_dependencyclosure, across files already in the pool and files in the set being added), and checks it at every resolve site. A reference outside the set is the newPoolError::TypeNotImported, whose message names the file to add todependency.A
dependencythat is not in the pool is still tolerated by default, so sets that strip option-only imports (google/api/annotations.proto) keep loading; only a reference into a missing or unimported file fails.LinkOptions, passed through the newDescriptorPool::decode_with_link_optionsorwith_link_options, turns enforcement off (with_import_visibility(false)) or additionally requires every listed non-weak dependency to be present (with_required_dependencies(true)→PoolError::DependencyNotFound). Two files with the same name in one set, which previously shadowed each other in the filename index, are nowPoolError::DuplicateFileName.Sets from
protoc --include_imports,buf build, and codegen's embeddedFILE_DESCRIPTOR_SET_BYTEScarry intact dependency lists and are unaffected. Breaking for hand-builtFileDescriptorProtos that reference another file without listing it, hence 0.10.0.Closes #423.