From 0a0803de73f2e97f0bad865af91cf736eec821d7 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Fri, 28 Aug 2026 10:35:27 +0900 Subject: [PATCH] fix(server): classify projector flags and confine request media b10621's multimodal support is a separate GGUF artifact loaded next to the language model; mlxcel loads an integrated MLX VLM checkpoint whose vision tower, audio tower and projector are tensors in the same snapshot. Before this change `--mmproj` parsed into an ignored field, the other seven projector and media options did not parse at all, a text-only checkpoint answered an image request from the prompt alone, and any `file://` path or bare existing path named by a request was opened straight off the wire. New `src/cli/multimodal_compat_args.rs` accepts all nine b10621 options on both server binaries and classifies each before the model reference resolves. `--mmproj` and `--mmproj-url` are refused with a diagnostic naming an MLX checkpoint to use instead; `--no-mmproj-offload` and every `--mmproj-device` value are refused because mlxcel has no host projector path; `--mtmd-batch-max-tokens` is inert only at upstream's own default of 1024. The projector family stays hidden from `--help`, so the operator surface never implies a GGUF projector can be attached; `--media-path` is visible under a new Multimodal Options heading. `-mm`, `-mmu` and `-mmdev` join the argv pre-pass in `src/cli/llama_short_flags.rs`. `--mmproj-auto` / `--no-mmproj` is honored: `src/server/state.rs`'s `ModelMediaSupport` gains `image` and `audio` alongside `video`, `detect_model_media_support` fills them from the same config.json-only model-type probe, and the chat, responses and Anthropic-messages routes now refuse a media part the checkpoint cannot consume with b10621's ` input is not supported` clause at HTTP 501 and its `not_supported_error` type. `prepare_request_vlm_embeddings` refuses the same case rather than silently dropping the image. `--image-min-tokens` / `--image-max-tokens` reach the real encoder through `src/vision/image_token_overrides.rs`, which reproduces upstream's `set_limit_image_tokens` arithmetic (`pixels = tokens * patch_size^2 * merge^2`) against the dynamic-resolution processors that express the same bounds. A checkpoint whose preprocessor has no such bound refuses to serve after load rather than ignoring the flag, using the applications-counter mechanism `models::rope_overrides` established so no architecture list can go stale. `src/server/media_root.rs` implements `--media-path` with a confined root: upstream's concatenation semantics (a leading separator is stripped so an absolute-looking path cannot replace the root), its whole `fs_validate_filename` rule set, refusal of percent-encoded traversal, canonicalize-and-contain against symlink escape, `O_NOFOLLOW` on the open, and regular files only. `src/server/media_net.rs` adds a network-address policy to remote media fetches: scheme and credential checks, IP-literal and resolved-address checks before the request, a redirect policy that refuses a hop leaving the public address space, and a post-connect peer re-check that closes the DNS-rebinding window. `MLXCEL_ALLOW_PRIVATE_MEDIA_URLS=1` is the operator opt-in. Validated against real checkpoints on M1 Ultra. qwen2.5-vl-3b-4bit with `--media-path` describes a relative `file://` image and a bare relative path; ten escape attempts (`../`, absolute, symlink out of the root, `%2e%2e`, `..%2f`, loopback, `169.254.169.254`, `[::1]`, `/dev/zero`) are all refused with a specific server-side diagnostic. `--image-max-tokens 16` moves prompt_tokens from 86 to 38, exactly 64 image tokens down to 16. gemma-3-4b-it-4bit with `--image-max-tokens 64` refuses to serve. qwen3-0.6b-4bit answers an image request with 501 `not_supported_error` on both chat and messages. `--no-mmproj` on the VLM refuses media and still serves text. Malformed, truncated, 80 MiB and decompression-bomb inputs all fail within bounded memory. Nine manifest entries in `compat/llama-server/b10621/multimodal-and-audio.json` leave `deferred`: `--mmproj`, `--mmproj-url`, `--mmproj-offload`, `--mmproj-device` and `--mtmd-batch-max-tokens` become `not_applicable`; `--mmproj-auto`, `--image-min-tokens`, `--image-max-tokens` and `--media-path` become `aliased`, each recording the observable divergence that forbids `supported`. Closes #1451 --- .../b10621/multimodal-and-audio.json | 175 ++++-- docs/llama-server-compat.md | 69 +++ src/bin/mlx_server.rs | 53 +- src/cli/llama_short_flags.rs | 3 + src/cli/mod.rs | 1 + src/cli/multimodal_compat_args.rs | 542 ++++++++++++++++++ src/cli/multimodal_compat_args_tests.rs | 345 +++++++++++ src/commands/serve.rs | 37 ++ src/commands/serve_tests.rs | 2 +- src/main.rs | 16 +- src/server/media.rs | 203 ++++++- src/server/media_net.rs | 377 ++++++++++++ src/server/media_net_tests.rs | 183 ++++++ src/server/media_root.rs | 305 ++++++++++ src/server/media_root_tests.rs | 314 ++++++++++ src/server/media_tests.rs | 206 ++++++- src/server/mod.rs | 6 +- src/server/model_worker.rs | 35 ++ src/server/routes/anthropic.rs | 18 + src/server/routes/chat.rs | 31 +- src/server/routes/responses.rs | 11 + src/server/startup.rs | 22 +- src/server/state.rs | 32 ++ src/vision/image_token_overrides.rs | 270 +++++++++ src/vision/image_token_overrides_tests.rs | 121 ++++ src/vision/mod.rs | 1 + src/vision/processors/qwen2_vl.rs | 36 +- 27 files changed, 3301 insertions(+), 113 deletions(-) create mode 100644 src/cli/multimodal_compat_args.rs create mode 100644 src/cli/multimodal_compat_args_tests.rs create mode 100644 src/server/media_net.rs create mode 100644 src/server/media_net_tests.rs create mode 100644 src/server/media_root.rs create mode 100644 src/server/media_root_tests.rs create mode 100644 src/vision/image_token_overrides.rs create mode 100644 src/vision/image_token_overrides_tests.rs diff --git a/compat/llama-server/b10621/multimodal-and-audio.json b/compat/llama-server/b10621/multimodal-and-audio.json index a5cf7d7db..4379a7644 100644 --- a/compat/llama-server/b10621/multimodal-and-audio.json +++ b/compat/llama-server/b10621/multimodal-and-audio.json @@ -17,12 +17,21 @@ "default": "read from model", "description": "maximum number of tokens each image can take, only used by vision models with dynamic resolution (default: read from model) (env: LLAMA_ARG_IMAGE_MAX_TOKENS)", "discovery": "help-entry-head", - "state": "deferred", + "state": "aliased", "issue": 1451, - "test": null, - "notes": null, - "divergence": [], - "mlxcel": null + "test": "src/cli/multimodal_compat_args_tests.rs::a_maximum_below_the_minimum_is_refused_as_upstream_throws", + "notes": "Honored, because the translation is the identity rather than an approximation. b10621 stores the value on `clip_hparams::custom_image_min_tokens` / `custom_image_max_tokens` and converts it in `set_limit_image_tokens` as `pixels = tokens * patch_size^2 * n_merge^2`; mlxcel's dynamic-resolution processors express the same two bounds as `min_pixels` / `max_pixels`, and `src/vision/image_token_overrides.rs` applies the same multiplication against the same patch area. Upstream's value domain is reproduced too: only a positive value is a custom bound, `0` and a negative number mean `read it from the model`, and a maximum below the minimum is refused as upstream throws on `image_max_pixels < image_min_pixels`. An architecture with no such bound does not silently ignore the flag: every bound-consuming processor increments an applications counter, and the model worker refuses to serve when a budget was requested and the counter is still zero after the checkpoint loads, naming the checkpoint and the flag. That is the same no-stale-list mechanism `src/models/rope_overrides.rs` uses. Accepted as a hidden compatibility argument; the operator-facing image controls stay `--max-image-payload-size` and `--max-images`.", + "divergence": [ + "Applies to the dynamic-resolution preprocessing path only (every family that resizes through `vision::processors::qwen2_vl`); a checkpoint whose preprocessor has no per-image pixel bound refuses to serve with a diagnostic instead of encoding at its own geometry, where b10621 accepts the flag against a wider set of architectures." + ], + "mlxcel": { + "accepted_spellings": [ + "--image-max-tokens" + ], + "env": "LLAMA_ARG_IMAGE_MAX_TOKENS", + "env_binding": "clap", + "hidden": true + } }, { "kind": "option", @@ -39,12 +48,21 @@ "default": "read from model", "description": "minimum number of tokens each image can take, only used by vision models with dynamic resolution (default: read from model) (env: LLAMA_ARG_IMAGE_MIN_TOKENS)", "discovery": "help-entry-head", - "state": "deferred", + "state": "aliased", "issue": 1451, - "test": null, - "notes": null, - "divergence": [], - "mlxcel": null + "test": "src/cli/multimodal_compat_args_tests.rs::image_token_bounds_parse_upstreams_value_domain", + "notes": "Honored, because the translation is the identity rather than an approximation. b10621 stores the value on `clip_hparams::custom_image_min_tokens` / `custom_image_max_tokens` and converts it in `set_limit_image_tokens` as `pixels = tokens * patch_size^2 * n_merge^2`; mlxcel's dynamic-resolution processors express the same two bounds as `min_pixels` / `max_pixels`, and `src/vision/image_token_overrides.rs` applies the same multiplication against the same patch area. Upstream's value domain is reproduced too: only a positive value is a custom bound, `0` and a negative number mean `read it from the model`, and a maximum below the minimum is refused as upstream throws on `image_max_pixels < image_min_pixels`. An architecture with no such bound does not silently ignore the flag: every bound-consuming processor increments an applications counter, and the model worker refuses to serve when a budget was requested and the counter is still zero after the checkpoint loads, naming the checkpoint and the flag. That is the same no-stale-list mechanism `src/models/rope_overrides.rs` uses. Accepted as a hidden compatibility argument; the operator-facing image controls stay `--max-image-payload-size` and `--max-images`.", + "divergence": [ + "Applies to the dynamic-resolution preprocessing path only (every family that resizes through `vision::processors::qwen2_vl`); a checkpoint whose preprocessor has no per-image pixel bound refuses to serve with a diagnostic instead of encoding at its own geometry, where b10621 accepts the flag against a wider set of architectures." + ], + "mlxcel": { + "accepted_spellings": [ + "--image-min-tokens" + ], + "env": "LLAMA_ARG_IMAGE_MIN_TOKENS", + "env_binding": "clap", + "hidden": true + } }, { "kind": "option", @@ -61,12 +79,20 @@ "default": "disabled", "description": "directory for loading local media files; files can be accessed via file:// URLs using relative paths (default: disabled)", "discovery": "help-entry-head", - "state": "deferred", + "state": "aliased", "issue": 1451, - "test": null, - "notes": null, - "divergence": [], - "mlxcel": null + "test": "src/server/media_root_tests.rs::a_symlink_pointing_outside_the_root_is_refused", + "notes": "Implemented, with a confined root. b10621 keeps the value on `common_params::media_path`, appends the platform separator at parse time, and resolves a request's `file://` URL by string concatenation in `handle_media`: no root means `file:// URLs are not allowed unless --media-path is specified`, and otherwise `fs_validate_filename(path, allow_subdirs=true)` runs before `media_path + file_path` is opened. mlxcel reproduces the wording, the concatenation (a leading separator is stripped so `file:///etc/passwd` lands under the root rather than replacing it, which a Rust `Path::join` would have done) and the whole `fs_validate_filename` rule set, then adds what a pure string check cannot do: the joined path is canonicalized and must stay inside the canonical root, and the file is opened with `O_NOFOLLOW` so the resolve-to-open window cannot be won by a symlink swap. Non-regular files are refused too, so a FIFO in the media root cannot block a request task. The same root governs a bare relative path, which is what an operator who configured `--media-path` naturally writes; before #1451 mlxcel had no root at all and opened any `file://` path, or any bare string naming an existing file, straight off the request.", + "divergence": [ + "A path that resolves outside the root through a symlink is refused; upstream's check is a pure string test and reads the symlink target.", + "A path carrying a percent escape for `.`, `/`, `\\` or NUL is refused outright rather than being treated as a literal filename, so an encoding-based traversal cannot depend on a future decoder being added.", + "A resolved path that is not a regular file is refused; upstream opens whatever the path names." + ], + "mlxcel": { + "accepted_spellings": [ + "--media-path" + ] + } }, { "kind": "option", @@ -84,15 +110,19 @@ "default": null, "description": "path to a multimodal projector file. see tools/mtmd/README.md note: if -hf is used, this argument can be omitted (env: LLAMA_ARG_MMPROJ)", "discovery": "help-entry-head", - "state": "deferred", + "state": "not_applicable", "issue": 1451, - "test": "tests/llama_compat_manifest.rs", - "notes": "Accepted and ignored (hidden): mlxcel loads the vision projector automatically from the checkpoint. #1451 owns projector flag classification.", - "divergence": [], + "test": "src/cli/multimodal_compat_args_tests.rs::mmproj_file_is_refused_with_an_mlx_replacement", + "notes": "b10621's multimodal support is a separate GGUF artifact: `libmtmd` loads a projector file alongside the language model. mlxcel loads an integrated MLX VLM checkpoint whose vision tower, audio tower and multimodal projector are tensors inside the same SafeTensors snapshot as the language model, resolved by the same `-m` reference, so there is nothing for a separate projector artifact to be attached to. Before #1451 this parsed into an ignored field, so a command line that asked mlxcel to attach a GGUF projector started a server that silently was not using it; it now stops startup, before the model reference resolves, with a diagnostic naming the value, the reason, and the MLX checkpoint to point `-m` at instead. Hidden from `--help` on purpose: rendering it would imply that a GGUF `mmproj` file can be attached to an MLX checkpoint, which is exactly what the epic asks the help surface not to say. llama.cpp writes the short spelling with a single dash and several letters, which clap reads as a cluster of one-letter shorts; the argv pre-pass in `src/cli/llama_short_flags.rs` rewrites the exact upstream tokens to the long spelling before clap sees them, so the flag-surface dump the manifest gate reads records only the long form.", + "divergence": [ + "Rejected at startup instead of loading a separate multimodal projector; an MLX VLM checkpoint carries its projector inside its own snapshot, so there is no GGUF artifact to attach." + ], "mlxcel": { "accepted_spellings": [ "--mmproj" ], + "env": "LLAMA_ARG_MMPROJ", + "env_binding": "clap", "hidden": true } }, @@ -115,12 +145,28 @@ "default": "enabled", "description": "whether to use multimodal projector file (if available), useful when using -hf (default: enabled) (env: LLAMA_ARG_MMPROJ_AUTO)", "discovery": "help-entry-head", - "state": "deferred", + "state": "aliased", "issue": 1451, - "test": null, - "notes": null, - "divergence": [], - "mlxcel": null + "test": "src/cli/multimodal_compat_args_tests.rs::no_mmproj_disables_media_admission_on_both_spellings", + "notes": "The one projector flag whose observable effect has a translation. Upstream's `--no-mmproj` leaves `mctx` null, and every image, audio or video part in a request is then refused with ` input is not supported`. mlxcel cannot leave the projector unloaded, because it is inside the checkpoint, but it declines exactly those requests at the HTTP boundary with the same leading clause, so a deployment script that disables multimodal input keeps its meaning. The hint half of upstream's sentence is replaced, because telling an operator to provide an `mmproj` would be false here. Upstream's default (`--mmproj-auto`, enabled) is what mlxcel already does and is inert. All three spellings parse; the pair is resolved from `LLAMA_ARG_MMPROJ_AUTO` at runtime rather than by clap, because b10621 reads it through `parse_bool_value` plus a `LLAMA_ARG_NO_*` alias meaning false, and clap's boolish parser accepts a wider vocabulary and errors outside it.", + "divergence": [ + "The checkpoint's vision and audio weights are still loaded and resident under `--no-mmproj`; upstream never loads the projector, so the two processes differ in memory footprint even though both refuse the same requests.", + "The refusal keeps b10621's ` input is not supported` clause but replaces its `you may need to provide the mmproj` hint, which has no meaning for an integrated MLX checkpoint." + ], + "mlxcel": { + "accepted_spellings": [ + "--mmproj-auto", + "--no-mmproj", + "--no-mmproj-auto" + ], + "defaults": [ + "false" + ], + "env": "LLAMA_ARG_MMPROJ_AUTO", + "env_binding": "runtime", + "env_test": "src/cli/multimodal_compat_args_tests.rs", + "hidden": true + } }, { "kind": "option", @@ -138,12 +184,21 @@ "default": null, "description": "device to use for multimodal projector (none = don't offload, default: auto) use --list-devices to see a list of available devices (env: MTMD_BACKEND_DEVICE)", "discovery": "help-entry-head", - "state": "deferred", + "state": "not_applicable", "issue": 1451, - "test": null, - "notes": null, - "divergence": [], - "mlxcel": null + "test": "src/cli/multimodal_compat_args_tests.rs::mmproj_device_none_and_a_named_device_are_both_refused", + "notes": "The flag describes where the projector runs. mlxcel evaluates the whole checkpoint, vision and audio towers included, on the one MLX device selected by `MLXCEL_DEVICE`, and has no host projector path to move it to, so the value that asks for nothing is inert and every other value stops startup with a diagnostic naming the option, the value, the limitation, and the mlxcel alternative where one exists. Accepted as a hidden compatibility argument and classified before the model reference resolves, like the GGML group. Upstream's `none` means `do not offload the projector`, the same request as `--no-mmproj-offload`, and any other value names a GGML device from `--list-devices`. mlxcel has neither, so both are rejected; a named device points at `MLXCEL_DEVICE`, which selects the MLX device for the whole checkpoint. The environment binding is `MTMD_BACKEND_DEVICE`, not a `LLAMA_ARG_*` variable, exactly as b10621 binds it, and it takes a value so clap carries it through unchanged.", + "divergence": [ + "Every value is rejected at startup; mlxcel has no GGML device list and places the whole checkpoint on one MLX device." + ], + "mlxcel": { + "accepted_spellings": [ + "--mmproj-device" + ], + "env": "MTMD_BACKEND_DEVICE", + "env_binding": "clap", + "hidden": true + } }, { "kind": "option", @@ -162,12 +217,26 @@ "default": "enabled", "description": "whether to enable GPU offloading for multimodal projector (default: enabled) (env: LLAMA_ARG_MMPROJ_OFFLOAD)", "discovery": "help-entry-head", - "state": "deferred", + "state": "not_applicable", "issue": 1451, - "test": null, - "notes": null, - "divergence": [], - "mlxcel": null + "test": "src/cli/multimodal_compat_args_tests.rs::projector_placement_accepts_only_the_inert_half", + "notes": "The flag describes where the projector runs. mlxcel evaluates the whole checkpoint, vision and audio towers included, on the one MLX device selected by `MLXCEL_DEVICE`, and has no host projector path to move it to, so the value that asks for nothing is inert and every other value stops startup with a diagnostic naming the option, the value, the limitation, and the mlxcel alternative where one exists. Accepted as a hidden compatibility argument and classified before the model reference resolves, like the GGML group. The positive half is b10621's own default and describes what mlxcel already does, so it is accepted silently; `--no-mmproj-offload` asks for a host-resident projector and is rejected. The pair is resolved from `LLAMA_ARG_MMPROJ_OFFLOAD` at runtime rather than by clap, for the same `parse_bool_value` reason as the rest of the b10621 boolean pairs.", + "divergence": [ + "`--no-mmproj-offload` is rejected; mlxcel cannot run the vision or audio tower on the host." + ], + "mlxcel": { + "accepted_spellings": [ + "--mmproj-offload", + "--no-mmproj-offload" + ], + "defaults": [ + "false" + ], + "env": "LLAMA_ARG_MMPROJ_OFFLOAD", + "env_binding": "runtime", + "env_test": "src/cli/multimodal_compat_args_tests.rs", + "hidden": true + } }, { "kind": "option", @@ -185,12 +254,21 @@ "default": null, "description": "URL to a multimodal projector file. see tools/mtmd/README.md (env: LLAMA_ARG_MMPROJ_URL)", "discovery": "help-entry-head", - "state": "deferred", + "state": "not_applicable", "issue": 1451, - "test": null, - "notes": null, - "divergence": [], - "mlxcel": null + "test": "src/cli/multimodal_compat_args_tests.rs::mmproj_url_is_refused_for_the_same_reason", + "notes": "b10621's multimodal support is a separate GGUF artifact: `libmtmd` loads a projector file alongside the language model. mlxcel loads an integrated MLX VLM checkpoint whose vision tower, audio tower and multimodal projector are tensors inside the same SafeTensors snapshot as the language model, resolved by the same `-m` reference, so there is nothing for a separate projector artifact to be attached to. Before #1451 this parsed into an ignored field, so a command line that asked mlxcel to attach a GGUF projector started a server that silently was not using it; it now stops startup, before the model reference resolves, with a diagnostic naming the value, the reason, and the MLX checkpoint to point `-m` at instead. Hidden from `--help` on purpose: rendering it would imply that a GGUF `mmproj` file can be attached to an MLX checkpoint, which is exactly what the epic asks the help surface not to say. llama.cpp writes the short spelling with a single dash and several letters, which clap reads as a cluster of one-letter shorts; the argv pre-pass in `src/cli/llama_short_flags.rs` rewrites the exact upstream tokens to the long spelling before clap sees them, so the flag-surface dump the manifest gate reads records only the long form.", + "divergence": [ + "Rejected at startup instead of downloading a separate multimodal projector; mlxcel resolves one MLX snapshot by repository identifier and has no second artifact to fetch." + ], + "mlxcel": { + "accepted_spellings": [ + "--mmproj-url" + ], + "env": "LLAMA_ARG_MMPROJ_URL", + "env_binding": "clap", + "hidden": true + } }, { "kind": "option", @@ -207,12 +285,21 @@ "default": "1024", "description": "maximum number of image tokens per batch when encoding images (default: 1024) (env: LLAMA_ARG_MTMD_BATCH_MAX_TOKENS)", "discovery": "help-entry-head", - "state": "deferred", + "state": "not_applicable", "issue": 1451, - "test": null, - "notes": null, - "divergence": [], - "mlxcel": null + "test": "src/cli/multimodal_compat_args_tests.rs::mtmd_batch_max_tokens_is_inert_at_the_upstream_default_only", + "notes": "Names an encode batch width, not a per-image budget: upstream splits an image's tokens across several `mtmd` encode batches of at most this many tokens. mlxcel encodes each image in a single vision-tower forward and has no image-token batch to bound, so b10621's own default of 1024 is inert and accepted, and any other value stops startup with a diagnostic pointing at `--max-image-payload-size` and `--max-images`, which are the image-work bounds mlxcel does have. Accepted as a hidden compatibility argument and classified before the model reference resolves.", + "divergence": [ + "Only b10621's own default of 1024 is accepted; mlxcel has no image-token encode batch for another value to size." + ], + "mlxcel": { + "accepted_spellings": [ + "--mtmd-batch-max-tokens" + ], + "env": "LLAMA_ARG_MTMD_BATCH_MAX_TOKENS", + "env_binding": "clap", + "hidden": true + } }, { "kind": "route", diff --git a/docs/llama-server-compat.md b/docs/llama-server-compat.md index 962840523..e7e099891 100644 --- a/docs/llama-server-compat.md +++ b/docs/llama-server-compat.md @@ -229,6 +229,75 @@ b10621 accepts either Jinja template text or one of 54 built-in identifiers (`ch Four entries are `deferred` against #1470 rather than claimed. `--reasoning-format` is the one whose behavior is otherwise complete; its streamed delimiter loss and the unverified `reasoning_in_content` question keep it there. `--prefill-assistant` is b10621's default and mlxcel diverges from it *with no flag passed*: a trailing assistant message is answered with a fresh turn here and continued upstream. `--reasoning-budget-message` parses and warns at startup but is not yet injected before the end-of-thinking tag. The native `echo` field is a plain completion feature mlxcel lacks. Every other native field in this shard is `not_applicable`: mlxcel's `POST /completion` is a raw-prompt endpoint with no chat template and no chat parsing for them to configure. +## Multimodal projectors and request media + +b10621's multimodal support is a separate artifact. `libmtmd` loads a projector file next to the language model (`--mmproj`, `--mmproj-url`), places it on a device (`--mmproj-device`, `--mmproj-offload`), and can be told not to load one at all (`--no-mmproj`). mlxcel loads an integrated MLX VLM checkpoint: the vision tower, the audio tower and the multimodal projector are tensors inside the same SafeTensors snapshot as the language model, resolved by the same `-m` reference. That difference decides the whole `multimodal-and-audio.json` shard (#1451). + +| Flag | Environment | State | Behavior | +|---|---|---|---| +| `--mmproj` / `-mm` | `LLAMA_ARG_MMPROJ` | `not_applicable` | Refused at startup. There is no separate GGUF projector to attach to an MLX checkpoint; the diagnostic names an MLX VLM checkpoint to point `-m` at instead. | +| `--mmproj-url` / `-mmu` | `LLAMA_ARG_MMPROJ_URL` | `not_applicable` | Refused for the same reason. | +| `--mmproj-auto` / `--no-mmproj` / `--no-mmproj-auto` | `LLAMA_ARG_MMPROJ_AUTO` | `aliased` | Honored. The default admits media; `--no-mmproj` refuses every image, audio and video part with b10621's own ` input is not supported` clause. | +| `--mmproj-offload` / `--no-mmproj-offload` | `LLAMA_ARG_MMPROJ_OFFLOAD` | `not_applicable` | The default is inert; `--no-mmproj-offload` is refused, because mlxcel has no host projector path. | +| `--mmproj-device` / `-mmdev` | `MTMD_BACKEND_DEVICE` | `not_applicable` | Refused, pointing at `MLXCEL_DEVICE`, which selects the MLX device for the whole checkpoint. | +| `--image-min-tokens` | `LLAMA_ARG_IMAGE_MIN_TOKENS` | `aliased` | Honored on the dynamic-resolution preprocessing path; see below. | +| `--image-max-tokens` | `LLAMA_ARG_IMAGE_MAX_TOKENS` | `aliased` | Honored on the same path. | +| `--mtmd-batch-max-tokens` | `LLAMA_ARG_MTMD_BATCH_MAX_TOKENS` | `not_applicable` | Inert at b10621's own default of 1024; any other value is refused, because mlxcel encodes each image in one vision-tower forward and has no image-token batch to bound. | +| `--media-path` | - | `aliased` | Implemented, with a confined root; see below. | + +Everything except `--media-path` is hidden from `--help`. Rendering the projector family would imply that a GGUF `mmproj` file can be attached to an MLX checkpoint, which is precisely what the classification denies. `--media-path` is a real mlxcel feature and is visible, under a `Multimodal Options` heading. + +### Media parts a checkpoint cannot consume are refused + +A text-only checkpoint used to accept an `image_url` content block at the HTTP boundary, drop it inside `prepare_request_vlm_embeddings`, and answer from the prompt alone. The reply was fluent, and a caller could not tell an ignored picture from a described one. The chat-completions, responses and Anthropic-messages routes now refuse the request before any referenced URL or file is read, with b10621's own leading clause and its `not_supported_error` type at HTTP 501: + +```text +image input is not supported - hint: the loaded checkpoint 'qwen3-0.6b-4bit' has no image tower; ... +``` + +Capability comes from the same `config.json`-only model-type probe that already decided video support, so a newly ported VLM family is admitted with no list to update. The worker keeps its own copy of the refusal, so a path that reaches it without passing a route gate fails rather than degrading to text. The hint half of upstream's sentence is replaced on purpose: telling an operator to provide an `mmproj` has no meaning for an integrated checkpoint. + +### `--image-min-tokens` / `--image-max-tokens` move real pixel bounds + +Upstream converts a token budget into pixel bounds in `clip_hparams::set_limit_image_tokens`: + +```text +patch_area = patch_size^2 * n_merge^2 +image_min_pixels = image_min_tokens * patch_area +image_max_pixels = image_max_tokens * patch_area +``` + +mlxcel's dynamic-resolution processors express the same two bounds as `min_pixels` / `max_pixels`, so the translation is the identity: the same multiplication against the same patch area, applied in `src/vision/image_token_overrides.rs` and consumed by `vision::processors::qwen2_vl::smart_resize`. Upstream's value domain carries over too. Only a positive value is a custom bound, `0` and a negative number mean "read it from the model", and a maximum below the minimum is refused exactly as upstream throws on `image_max_pixels < image_min_pixels`. + +A checkpoint whose preprocessor resizes to a fixed geometry has no such bound to move. Rather than a hand-written list of honoring architectures, every bound-consuming processor increments an applications counter and the model worker refuses to serve when a budget was requested and the counter is still zero after the checkpoint loads, naming the checkpoint and the flag. That is the mechanism [`rope_overrides`](../src/models/rope_overrides.rs) already uses for `--rope-freq-base`, and for the same reason: a list goes stale the moment a family is ported. + +### `--media-path` confines local files to one root + +Without `--media-path`, a `file://` media URL in a request is refused with b10621's own sentence, `file:// URLs are not allowed unless --media-path is specified`. With it, the request's path is resolved against the configured directory and may not leave it. + +mlxcel reproduces upstream's rules and then adds what a pure string check cannot do: + +- **Concatenation, not join.** b10621 evaluates `media_path + file_path`, so `file:///etc/passwd` lands under the root instead of replacing it. mlxcel strips the leading separators before joining, because a Rust `Path::join` with an absolute component would discard the root and turn a compatibility feature into an arbitrary-file read. +- **b10621's whole name validation.** `fs_validate_filename(path, allow_subdirs=true)`: no `..` anywhere, no control characters, none of `: * ? " < > |`, no leading or trailing space, no trailing `.`, at most 255 bytes, and the Unicode separator look-alikes (`U+FF0E`, `U+2215`, `U+2216`, `U+FFFD`, `U+FEFF`) refused. +- **No percent decoding, and no percent-encoded traversal.** Upstream never decodes, so `%2e%2e` is a literal filename there. mlxcel does not decode either, and additionally refuses a path carrying a percent escape for `.`, `/`, `\` or NUL, so the property is checked rather than inherited from an absent call. +- **Canonicalize and contain.** The joined path is canonicalized and must stay inside the canonical root. A symlink whose target leaves the root is refused; one that stays inside still resolves, so organising the media root with links keeps working. +- **`O_NOFOLLOW` on the open.** The resolve-to-open window cannot be won by swapping the last component for a symlink; the open fails with `ELOOP` instead. This is the same primitive the video resolver already used. +- **Regular files only.** A directory, FIFO, socket or device node under the root is refused, so a FIFO cannot block a request task waiting for a writer. + +The last three are stricter than b10621, whose check is a pure string test that reads whatever the path names, and are recorded as divergences on the entry. A bare relative path is governed by the same root, because that is what an operator who configured `--media-path` naturally writes. + +### Remote media URLs have a network-address policy + +A request may name an `http(s)` image, audio or video URL and the server fetches it, which makes the server a fetch proxy an unauthenticated caller steers. b10621 fetches with `common_remote_get_content` and applies no address policy at all. mlxcel refuses non-public addresses in three places, which is recorded as a divergence: + +1. **Before the request.** The scheme must be `http` or `https`, the URL may not carry credentials, an IP-literal host is checked directly, and a host written as a name is resolved with every resulting address checked. +2. **On every redirect.** The redirect policy sees the next URL before it is followed, so a public origin cannot bounce the fetch onto `http://169.254.169.254/`. The chain stays bounded at five hops. +3. **After the connection.** The peer address the request actually reached is re-checked before a single body byte is read, which closes the DNS-rebinding window the first two leave open. + +Refused: loopback, the unspecified address, multicast and broadcast, the IPv4 private and link-local ranges (which is what puts the cloud metadata service at `169.254.169.254` out of reach), carrier-grade NAT, the IETF protocol-assignment, benchmarking and documentation prefixes, `240.0.0.0/4`, IPv6 unique-local, link-local and documentation, and any IPv4-mapped or IPv4-compatible IPv6 spelling of a refused IPv4 address. A deployment that genuinely serves its media from an internal object store sets `MLXCEL_ALLOW_PRIVATE_MEDIA_URLS=1`, which turns every address check into a pass. It is off by default because a fetch proxy that reaches the private network has to be an explicit operator decision. + +The existing byte and decode limits are unchanged and still apply: `--max-image-payload-size`, `--max-images`, the decoder's width, height and allocation caps, the audio per-clip and per-request ceilings, and the 10-second fetch timeout. + ## Regeneration ```bash diff --git a/src/bin/mlx_server.rs b/src/bin/mlx_server.rs index e6efd93d6..1dcb53fb5 100644 --- a/src/bin/mlx_server.rs +++ b/src/bin/mlx_server.rs @@ -20,6 +20,7 @@ use mlxcel::cli::batch_quant_args::BatchKvQuantArgs; use mlxcel::cli::cache_args::CacheCompatArgs; use mlxcel::cli::chat_compat_args::ChatCompatArgs; use mlxcel::cli::ggml_compat_args::{GgmlCompatArgs, read_model_layer_count}; +use mlxcel::cli::multimodal_compat_args::MultimodalCompatArgs; use mlxcel::cli::rope_args::RopeOverrideArgs; use mlxcel::cli::speculative_args::{ SpeculativeArgs, env_fallback_draft_block_size, env_fallback_draft_kind, @@ -1214,10 +1215,6 @@ struct ServerArgs { #[arg(long, hide = true)] _no_webui: bool, - /// Accepted for llama-server CLI compatibility (ignored: vision projector loaded automatically) - #[arg(long, hide = true)] - _mmproj: Option, - /// Maximum number of cached post-projection image features per loaded VLM. /// /// Multi-turn conversations that revisit the same image reuse cached @@ -1361,6 +1358,17 @@ struct ServerArgs { #[command(flatten)] chat_compat: ChatCompatArgs, + /// llama-server b10621 multimodal projector and media options + /// (`--mmproj`, `--mmproj-url`, `--mmproj-auto`, `--mmproj-offload`, + /// `--mmproj-device`, `--image-min-tokens`, `--image-max-tokens`, + /// `--mtmd-batch-max-tokens`, `--media-path`). Every projector flag is a + /// hidden compatibility surface classified at startup like the GGML group; + /// `--media-path` is a real mlxcel feature and is visible. Defined once in + /// `mlxcel::cli::multimodal_compat_args` so both server binaries accept + /// exactly the same set (issue #1451). + #[command(flatten)] + multimodal_compat: MultimodalCompatArgs, + /// Continuous-batching KV quantization flag group /// (`--kv-bits`, `--kv-group-size`, `--kv-quant-scheme`, /// `--kv-skip-last-layer`). Defined once in @@ -1861,6 +1869,43 @@ fn build_startup_input(mut args: ServerArgs) -> anyhow::Result, +//! + +use std::path::{Path, PathBuf}; + +use clap::Args; + +use super::ggml_compat_args::{env_bool_pair, env_flag}; + +/// b10621's own `--mtmd-batch-max-tokens` default. +/// +/// The only value mlxcel accepts: it names an image-token batch width, and +/// mlxcel encodes every image in a single vision-tower forward. +pub const B10621_MTMD_BATCH_MAX_TOKENS: i64 = 1024; + +/// Whether the server admits multimodal content parts at the request boundary. +/// +/// This is the mlxcel translation of b10621's `--mmproj-auto` / +/// `--no-mmproj`. Upstream decides it by whether a projector was loaded: +/// `--no-mmproj` leaves `mctx` null and every image, audio or video part in a +/// request is refused with `... input is not supported - hint: if this is +/// unexpected, you may need to provide the mmproj`. mlxcel cannot leave the +/// projector unloaded (it is inside the checkpoint), so it declines the same +/// requests with the same wording instead. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub enum MediaAdmission { + /// b10621's default: media parts are admitted when the checkpoint can + /// consume them. + #[default] + Auto, + /// `--no-mmproj` / `--no-mmproj-auto`: every media part is refused. + Disabled, +} + +impl MediaAdmission { + /// True when the operator asked for media to be refused outright. + #[must_use] + pub const fn is_disabled(self) -> bool { + matches!(self, Self::Disabled) + } +} + +/// llama-server b10621 multimodal projector / media options. +/// +/// Flattened into both server binaries so the two surfaces cannot drift; see +/// `tests/cli_help_consistency.rs`. +#[derive(Args, Debug, Clone, Default)] +pub struct MultimodalCompatArgs { + // ── projector artifact ────────────────────────────────────────────── + /// b10621 `-mm` / `--mmproj`: path to a multimodal projector file. + #[arg( + long = "mmproj", + env = "LLAMA_ARG_MMPROJ", + value_name = "FILE", + hide = true + )] + pub mmproj: Option, + + /// b10621 `-mmu` / `--mmproj-url`: URL to a multimodal projector file. + #[arg( + long = "mmproj-url", + env = "LLAMA_ARG_MMPROJ_URL", + value_name = "URL", + hide = true + )] + pub mmproj_url: Option, + + /// b10621 `--mmproj-auto` (positive half of the pair, upstream's default). + #[arg(long = "mmproj-auto", overrides_with_all = ["no_mmproj", "no_mmproj_auto"], hide = true)] + pub mmproj_auto: bool, + + /// b10621 `--no-mmproj` (negative half of the pair). + #[arg(long = "no-mmproj", overrides_with = "mmproj_auto", hide = true)] + pub no_mmproj: bool, + + /// b10621 `--no-mmproj-auto` (second spelling of the negative half). + #[arg(long = "no-mmproj-auto", overrides_with = "mmproj_auto", hide = true)] + pub no_mmproj_auto: bool, + + // ── projector placement ───────────────────────────────────────────── + /// b10621 `--mmproj-offload` (positive half, upstream's default). + #[arg( + long = "mmproj-offload", + overrides_with = "no_mmproj_offload", + hide = true + )] + pub mmproj_offload: bool, + + /// b10621 `--no-mmproj-offload` (negative half). + #[arg( + long = "no-mmproj-offload", + overrides_with = "mmproj_offload", + hide = true + )] + pub no_mmproj_offload: bool, + + /// b10621 `-mmdev` / `--mmproj-device`: device for the projector. + /// + /// Bound to `MTMD_BACKEND_DEVICE`, not a `LLAMA_ARG_*` variable, exactly as + /// upstream binds it. + #[arg( + long = "mmproj-device", + env = "MTMD_BACKEND_DEVICE", + value_name = "DEVICE", + hide = true + )] + pub mmproj_device: Option, + + // ── image token budget ────────────────────────────────────────────── + /// b10621 `--image-min-tokens`: minimum tokens one image may take. + #[arg( + long = "image-min-tokens", + env = "LLAMA_ARG_IMAGE_MIN_TOKENS", + value_name = "N", + hide = true + )] + pub image_min_tokens: Option, + + /// b10621 `--image-max-tokens`: maximum tokens one image may take. + #[arg( + long = "image-max-tokens", + env = "LLAMA_ARG_IMAGE_MAX_TOKENS", + value_name = "N", + hide = true + )] + pub image_max_tokens: Option, + + /// b10621 `--mtmd-batch-max-tokens`: image tokens per encode batch. + #[arg( + long = "mtmd-batch-max-tokens", + env = "LLAMA_ARG_MTMD_BATCH_MAX_TOKENS", + value_name = "N", + hide = true + )] + pub mtmd_batch_max_tokens: Option, + + // ── local media root ──────────────────────────────────────────────── + /// Directory that local `file://` media URLs are resolved against. + /// + /// Disabled by default: without it, a `file://` media URL in a request is + /// refused. The value is canonicalized once at startup, and every resolved + /// file must stay inside it, symlink targets included. llama-server spells + /// this `--media-path` too. + #[arg( + long = "media-path", + value_name = "PATH", + help_heading = "Multimodal Options" + )] + pub media_path: Option, +} + +/// One rejected multimodal option, ready to render. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct MultimodalCompatRejection { + /// The option as the operator wrote it, for example `--mmproj`. + pub option: &'static str, + /// The requested value, or the flag name again for a value-less flag. + pub value: String, + /// Why mlxcel cannot reproduce b10621's behavior for that value. + pub limitation: &'static str, + /// What to use instead, or `None` when nothing corresponds. + pub alternative: Option<&'static str>, +} + +impl std::fmt::Display for MultimodalCompatRejection { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!( + f, + "{} {} is not supported: {}", + self.option, self.value, self.limitation + )?; + match self.alternative { + Some(alternative) => write!(f, "\nUse instead: {alternative}"), + None => write!( + f, + "\nThere is no mlxcel equivalent; drop the flag to start the server." + ), + } + } +} + +/// The operator-requested image token budget, in b10621's units. +/// +/// Both halves are token counts, converted to the pixel bounds a dynamic +/// resolution processor expresses by +/// [`crate::vision::image_token_overrides`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub struct ImageTokenBounds { + /// `--image-min-tokens`, when set to a positive value. + pub min_tokens: Option, + /// `--image-max-tokens`, when set to a positive value. + pub max_tokens: Option, +} + +impl ImageTokenBounds { + /// True when neither half was requested. + #[must_use] + pub const fn is_empty(self) -> bool { + self.min_tokens.is_none() && self.max_tokens.is_none() + } +} + +const PROJECTOR_LIMITATION: &str = "mlxcel loads an integrated MLX VLM checkpoint whose vision tower, audio tower and \ + multimodal projector are tensors inside the same SafeTensors snapshot as the language \ + model, so there is no separate GGUF projector artifact to attach"; + +const PROJECTOR_ALTERNATIVE: &str = "point `-m` / `--model` at an MLX VLM checkpoint (for example \ + mlx-community/Qwen2.5-VL-7B-Instruct-4bit); its projector loads with it and needs no flag"; + +const PLACEMENT_LIMITATION: &str = "mlxcel evaluates the whole checkpoint, vision and audio towers included, on the one MLX \ + device, and has no host projector path to move it to"; + +impl MultimodalCompatArgs { + /// Reject every value whose b10621 meaning mlxcel cannot reproduce. + /// + /// Runs before the model reference resolves, so `--mmproj proj.gguf` is + /// reported immediately rather than after a multi-gigabyte download. Every + /// option in this group is model-independent: what a projector artifact is, + /// where it runs, and how wide an encode batch is are all decided without + /// reading `config.json`. Whether the *loaded* checkpoint honors + /// `--image-min-tokens` is a separate, post-load check owned by + /// [`crate::vision::image_token_overrides::verify_applied`]. + /// + /// # Errors + /// + /// Returns the first rejection in a fixed order, so a command line carrying + /// several unsupported values always reports the same one. + pub fn ensure_inert(&self) -> Result<(), MultimodalCompatRejection> { + self.rejection().map_or(Ok(()), Err) + } + + /// The first unsupported request in this argument set, if any. + #[must_use] + pub fn rejection(&self) -> Option { + if let Some(value) = present(self.mmproj.as_deref()) { + return Some(reject( + "--mmproj", + value, + PROJECTOR_LIMITATION, + Some(PROJECTOR_ALTERNATIVE), + )); + } + if let Some(value) = present(self.mmproj_url.as_deref()) { + return Some(reject( + "--mmproj-url", + value, + PROJECTOR_LIMITATION, + Some(PROJECTOR_ALTERNATIVE), + )); + } + if self.no_mmproj_offload { + return Some(reject( + "--no-mmproj-offload", + "--no-mmproj-offload", + PLACEMENT_LIMITATION, + None, + )); + } + if let Some(value) = present(self.mmproj_device.as_deref()) { + // Upstream's `none` means "do not offload the projector", the same + // request as `--no-mmproj-offload`; anything else names a GGML + // device, which mlxcel selects with its own environment variable. + let alternative = if value.eq_ignore_ascii_case("none") { + None + } else { + Some("`MLXCEL_DEVICE` selects the MLX device for the whole checkpoint") + }; + return Some(reject( + "--mmproj-device", + value, + PLACEMENT_LIMITATION, + alternative, + )); + } + if let Some(value) = present(self.mtmd_batch_max_tokens.as_deref()) + && !numeric_equals(value, B10621_MTMD_BATCH_MAX_TOKENS) + { + return Some(reject( + "--mtmd-batch-max-tokens", + value, + "mlxcel encodes each image in a single vision-tower forward and has no \ + image-token batch to bound, so only b10621's own default of 1024 is inert", + Some("`--max-image-payload-size` and `--max-images` bound image work on mlxcel"), + )); + } + self.image_token_bounds().err() + } + + /// The requested image token budget, or the rejection its values earn. + /// + /// Upstream reads both with `std::stoi` and only treats a **positive** + /// value as a custom bound (`custom_image_min_tokens > 0`), so `0` and a + /// negative number mean "use the model's own value" rather than being an + /// error. It then throws when the resulting `image_max_pixels` is below + /// `image_min_pixels`, which for a single checkpoint reduces to + /// `max_tokens < min_tokens`. + /// + /// # Errors + /// + /// Returns a rejection for a non-integer value or for a maximum below the + /// minimum. + pub fn image_token_bounds(&self) -> Result { + let min_tokens = + parse_image_tokens("--image-min-tokens", self.image_min_tokens.as_deref())?; + let max_tokens = + parse_image_tokens("--image-max-tokens", self.image_max_tokens.as_deref())?; + if let (Some(min), Some(max)) = (min_tokens, max_tokens) + && max < min + { + return Err(reject_owned( + "--image-max-tokens", + &max.to_string(), + "b10621 refuses a maximum image-token budget below the minimum \ + (`image_max_pixels` below `image_min_pixels`)", + Some("raise `--image-max-tokens` above `--image-min-tokens`"), + )); + } + Ok(ImageTokenBounds { + min_tokens, + max_tokens, + }) + } + + /// Whether media parts are admitted, per `--mmproj-auto` / `--no-mmproj`. + #[must_use] + pub fn media_admission(&self) -> MediaAdmission { + if self.no_mmproj || self.no_mmproj_auto { + MediaAdmission::Disabled + } else { + MediaAdmission::Auto + } + } + + /// Resolve `--media-path` into a canonical confined root. + /// + /// b10621 requires the value to be an existing directory and appends the + /// platform separator so its own `media_path + file_path` concatenation + /// lands inside it. mlxcel canonicalizes instead, because the canonical + /// root is what every later containment check compares against; a root that + /// is itself a symlink would otherwise make every contained path look like + /// an escape. + /// + /// # Errors + /// + /// Returns the same class of startup error upstream throws: the value is + /// not a directory, or it cannot be canonicalized. + pub fn resolve_media_root(&self) -> Result, String> { + let Some(raw) = self.media_path.as_deref() else { + return Ok(None); + }; + resolve_media_root_path(raw).map(Some) + } + + /// Resolve the b10621 environment bindings this group cannot leave to clap. + /// + /// The two `--x` / `--no-x` pairs bind `LLAMA_ARG_MMPROJ_AUTO` and + /// `LLAMA_ARG_MMPROJ_OFFLOAD`, which upstream reads through + /// `parse_bool_value` plus a `LLAMA_ARG_NO_*` alias meaning false. clap's + /// own boolish parser accepts a wider vocabulary and errors outside it, so + /// the shared [`env_bool_pair`] reproduces b10621's rules instead. Every + /// value-taking option in this group is bound by clap directly. + /// + /// # Errors + /// + /// Returns the variable name and its value when b10621's `parse_bool_value` + /// would throw on it, which is what b10621 does. + pub fn apply_env_bindings(&mut self) -> Result<(), (&'static str, String)> { + for (var, positive, negative) in [ + ( + "LLAMA_ARG_MMPROJ_AUTO", + &mut self.mmproj_auto, + &mut self.no_mmproj, + ), + ( + "LLAMA_ARG_MMPROJ_OFFLOAD", + &mut self.mmproj_offload, + &mut self.no_mmproj_offload, + ), + ] { + // An explicit command-line occurrence of either half wins. + if *positive || *negative { + continue; + } + match env_bool_pair(var) { + None => {} + Some(Ok(true)) => *positive = true, + Some(Ok(false)) => *negative = true, + Some(Err(raw)) => return Err((var, raw)), + } + } + // `--no-mmproj-auto` has no environment binding of its own upstream; + // `LLAMA_ARG_NO_MMPROJ_AUTO` is the alias `env_bool_pair` already + // resolves into the negative half above. The value-less positive-only + // read is kept for symmetry with the GGML group, where a bare + // `LLAMA_ARG_MMPROJ_AUTO=on` must fire the flag. + if env_flag("LLAMA_ARG_MMPROJ_AUTO") { + self.mmproj_auto = true; + } + Ok(()) + } +} + +/// Canonicalize a `--media-path` value into a confined root. +/// +/// Separated from [`MultimodalCompatArgs::resolve_media_root`] so tests and the +/// server can reach the same rule with a bare path. +/// +/// # Errors +/// +/// Returns a startup diagnostic when the path does not exist, is not a +/// directory, or cannot be canonicalized. +pub fn resolve_media_root_path(raw: &Path) -> Result { + let canonical = std::fs::canonicalize(raw) + .map_err(|err| format!("--media-path {}: {err}", raw.display()))?; + if !canonical.is_dir() { + return Err(format!("--media-path not a directory: {}", raw.display())); + } + Ok(canonical) +} + +/// Parse one image-token bound the way upstream's `std::stoi` handler does. +fn parse_image_tokens( + option: &'static str, + value: Option<&str>, +) -> Result, MultimodalCompatRejection> { + let Some(value) = present(value) else { + return Ok(None); + }; + let Ok(parsed) = value.parse::() else { + return Err(reject( + option, + value, + "b10621 reads this option as an integer number of tokens", + Some("pass a positive token count, or drop the flag to use the checkpoint's own bound"), + )); + }; + // Upstream only treats a positive value as a custom bound; `0` and negative + // numbers leave `custom_image_*_tokens` at its `-1` sentinel. + Ok(u32::try_from(parsed).ok().filter(|n| *n > 0)) +} + +/// `Some(trimmed)` when a value is present and not whitespace-only. +/// +/// An environment-bound option routinely arrives as an empty string from a +/// shell variable that was never set, and refusing to start over one would make +/// the compatibility surface worse than ignoring the flag. +fn present(value: Option<&str>) -> Option<&str> { + value.map(str::trim).filter(|v| !v.is_empty()) +} + +/// True when `value` parses as an integer equal to `inert`. +fn numeric_equals(value: &str, inert: i64) -> bool { + value.trim().parse::().is_ok_and(|n| n == inert) +} + +fn reject( + option: &'static str, + value: &str, + limitation: &'static str, + alternative: Option<&'static str>, +) -> MultimodalCompatRejection { + reject_owned(option, value, limitation, alternative) +} + +fn reject_owned( + option: &'static str, + value: &str, + limitation: &'static str, + alternative: Option<&'static str>, +) -> MultimodalCompatRejection { + MultimodalCompatRejection { + option, + value: value.to_owned(), + limitation, + alternative, + } +} + +#[cfg(test)] +#[path = "multimodal_compat_args_tests.rs"] +mod tests; diff --git a/src/cli/multimodal_compat_args_tests.rs b/src/cli/multimodal_compat_args_tests.rs new file mode 100644 index 000000000..a7a367edf --- /dev/null +++ b/src/cli/multimodal_compat_args_tests.rs @@ -0,0 +1,345 @@ +// Copyright 2025-2026 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Classification tests for the b10621 multimodal projector / media group +//! (issue #1451). + +use super::*; + +fn args() -> MultimodalCompatArgs { + MultimodalCompatArgs::default() +} + +#[test] +fn a_bare_argument_set_is_inert() { + assert_eq!(args().rejection(), None); + assert_eq!(args().media_admission(), MediaAdmission::Auto); + assert!(args().image_token_bounds().expect("no bounds").is_empty()); +} + +#[test] +fn mmproj_file_is_refused_with_an_mlx_replacement() { + let mut a = args(); + a.mmproj = Some("/models/qwen2-vl-mmproj-f16.gguf".into()); + let rejection = a.rejection().expect("a projector file is refused"); + assert_eq!(rejection.option, "--mmproj"); + let rendered = rejection.to_string(); + assert!(rendered.contains("qwen2-vl-mmproj-f16.gguf"), "{rendered}"); + assert!( + rendered.contains("integrated MLX VLM checkpoint"), + "{rendered}" + ); + // The diagnostic has to name a concrete replacement, not just say no. + assert!(rendered.contains("--model"), "{rendered}"); +} + +#[test] +fn mmproj_url_is_refused_for_the_same_reason() { + let mut a = args(); + a.mmproj_url = Some("https://example.invalid/mmproj.gguf".into()); + let rejection = a.rejection().expect("a projector URL is refused"); + assert_eq!(rejection.option, "--mmproj-url"); +} + +#[test] +fn an_empty_projector_value_from_an_unset_variable_is_ignored() { + // `LLAMA_ARG_MMPROJ=` arrives as an empty string; refusing to start over one + // would make the compatibility surface worse than ignoring it. + let mut a = args(); + a.mmproj = Some(" ".into()); + a.mmproj_url = Some(String::new()); + assert_eq!(a.rejection(), None); +} + +#[test] +fn projector_placement_accepts_only_the_inert_half() { + let mut on = args(); + on.mmproj_offload = true; + assert_eq!(on.rejection(), None, "the default half asks for nothing"); + + let mut off = args(); + off.no_mmproj_offload = true; + let rejection = off.rejection().expect("host offload is refused"); + assert_eq!(rejection.option, "--no-mmproj-offload"); + assert!(rejection.to_string().contains("one MLX device")); +} + +#[test] +fn mmproj_device_none_and_a_named_device_are_both_refused() { + let mut none = args(); + none.mmproj_device = Some("none".into()); + let rejection = none.rejection().expect("`none` is refused"); + assert_eq!(rejection.option, "--mmproj-device"); + assert!( + rejection.alternative.is_none(), + "`none` asks for a host projector, for which there is no alternative" + ); + + let mut named = args(); + named.mmproj_device = Some("CUDA0".into()); + let rejection = named.rejection().expect("a device name is refused"); + assert!(rejection.to_string().contains("MLXCEL_DEVICE")); +} + +#[test] +fn mtmd_batch_max_tokens_is_inert_at_the_upstream_default_only() { + for inert in ["1024", " 1024 ", "+1024"] { + let mut a = args(); + a.mtmd_batch_max_tokens = Some(inert.into()); + assert_eq!(a.rejection(), None, "{inert} is b10621's own default"); + } + let mut a = args(); + a.mtmd_batch_max_tokens = Some("512".into()); + let rejection = a.rejection().expect("a narrower batch is refused"); + assert_eq!(rejection.option, "--mtmd-batch-max-tokens"); + assert!( + rejection + .to_string() + .contains("single vision-tower forward") + ); +} + +#[test] +fn image_token_bounds_parse_upstreams_value_domain() { + let mut a = args(); + a.image_min_tokens = Some("64".into()); + a.image_max_tokens = Some("1280".into()); + let bounds = a.image_token_bounds().expect("both halves parse"); + assert_eq!(bounds.min_tokens, Some(64)); + assert_eq!(bounds.max_tokens, Some(1280)); + assert!(!bounds.is_empty()); +} + +#[test] +fn a_non_positive_image_token_bound_means_use_the_models_own() { + // Upstream only treats `custom_image_*_tokens > 0` as a custom bound, so + // `0` and a negative value are the sentinel for "read it from the model" + // rather than an error. + for sentinel in ["0", "-1"] { + let mut a = args(); + a.image_min_tokens = Some(sentinel.into()); + let bounds = a.image_token_bounds().expect("a sentinel is accepted"); + assert_eq!(bounds.min_tokens, None, "{sentinel}"); + } +} + +#[test] +fn a_non_integer_image_token_bound_is_refused() { + let mut a = args(); + a.image_max_tokens = Some("lots".into()); + let rejection = a.rejection().expect("a non-integer is refused"); + assert_eq!(rejection.option, "--image-max-tokens"); +} + +#[test] +fn a_maximum_below_the_minimum_is_refused_as_upstream_throws() { + let mut a = args(); + a.image_min_tokens = Some("512".into()); + a.image_max_tokens = Some("64".into()); + let rejection = a.rejection().expect("an inverted budget is refused"); + assert_eq!(rejection.option, "--image-max-tokens"); + assert!(rejection.to_string().contains("below the minimum")); +} + +#[test] +fn no_mmproj_disables_media_admission_on_both_spellings() { + for mutate in [ + (|a: &mut MultimodalCompatArgs| a.no_mmproj = true) as fn(&mut MultimodalCompatArgs), + |a: &mut MultimodalCompatArgs| a.no_mmproj_auto = true, + ] { + let mut a = args(); + mutate(&mut a); + assert_eq!(a.media_admission(), MediaAdmission::Disabled); + assert!(a.media_admission().is_disabled()); + // Disabling admission is honored, not refused. + assert_eq!(a.rejection(), None); + } + let mut auto = args(); + auto.mmproj_auto = true; + assert_eq!(auto.media_admission(), MediaAdmission::Auto); +} + +#[test] +fn media_root_resolution_requires_an_existing_directory() { + let dir = tempfile::tempdir().expect("tempdir"); + let mut a = args(); + a.media_path = Some(dir.path().to_path_buf()); + let resolved = a + .resolve_media_root() + .expect("an existing directory resolves") + .expect("some root"); + assert_eq!( + resolved, + std::fs::canonicalize(dir.path()).expect("canonical tempdir"), + "the stored root must be canonical so containment compares like with like" + ); + + let mut missing = args(); + missing.media_path = Some(dir.path().join("nope")); + assert!(missing.resolve_media_root().is_err()); + + let file = dir.path().join("a.png"); + std::fs::write(&file, b"x").expect("write"); + let mut not_a_dir = args(); + not_a_dir.media_path = Some(file); + let err = not_a_dir + .resolve_media_root() + .expect_err("a file is not a media root"); + assert!(err.contains("not a directory"), "{err}"); +} + +#[test] +fn no_media_path_leaves_local_files_disabled() { + assert_eq!(args().resolve_media_root().expect("no root"), None); +} + +#[test] +fn every_projector_flag_reports_in_a_fixed_order() { + // A command line carrying several unsupported values must always name the + // same one, so a deployment script's failure message does not depend on + // clap's field order changing. + let mut a = args(); + a.mmproj = Some("p.gguf".into()); + a.mmproj_url = Some("https://example.invalid/p.gguf".into()); + a.no_mmproj_offload = true; + a.mmproj_device = Some("none".into()); + a.mtmd_batch_max_tokens = Some("1".into()); + assert_eq!(a.rejection().expect("refused").option, "--mmproj"); +} + +// ── environment vocabulary matches b10621 ─────────────────────────────────── + +/// Set `pairs` for the duration of `body`, restoring the previous values. +/// +/// Serialized against every other environment-mutating test in this binary; +/// `cargo test` runs them all in one process and Rust 2024 makes an unguarded +/// `set_var` next to a concurrent read undefined behavior. +fn with_env(pairs: &[(&str, Option<&str>)], body: impl FnOnce() -> T) -> T { + let _guard = crate::test_support::env_lock::env_lock(); + let saved: Vec<(String, Option)> = pairs + .iter() + .map(|(k, _)| ((*k).to_owned(), std::env::var(k).ok())) + .collect(); + unsafe { + for (key, value) in pairs { + match value { + Some(v) => std::env::set_var(key, v), + None => std::env::remove_var(key), + } + } + } + let out = body(); + unsafe { + for (key, value) in &saved { + match value { + Some(v) => std::env::set_var(key, v), + None => std::env::remove_var(key), + } + } + } + out +} + +#[test] +fn llama_arg_mmproj_auto_resolves_through_b10621s_own_vocabulary() { + // clap's boolish parser accepts a wider vocabulary than b10621's + // `parse_bool_value` and errors outside it, so the pair is resolved at + // runtime instead of being bound by clap. + for (value, expected) in [ + ("off", MediaAdmission::Disabled), + ("false", MediaAdmission::Disabled), + ("0", MediaAdmission::Disabled), + ("on", MediaAdmission::Auto), + ("1", MediaAdmission::Auto), + ] { + let admission = with_env( + &[ + ("LLAMA_ARG_MMPROJ_AUTO", Some(value)), + ("LLAMA_ARG_NO_MMPROJ_AUTO", None), + ], + || { + let mut a = args(); + a.apply_env_bindings().expect("a recognized value"); + a.media_admission() + }, + ); + assert_eq!(admission, expected, "LLAMA_ARG_MMPROJ_AUTO={value}"); + } +} + +#[test] +fn the_no_alias_variable_means_false_as_upstream_defines_it() { + let admission = with_env( + &[ + ("LLAMA_ARG_MMPROJ_AUTO", Some("on")), + ("LLAMA_ARG_NO_MMPROJ_AUTO", Some("whatever")), + ], + || { + let mut a = args(); + a.apply_env_bindings().expect("the alias wins"); + a.media_admission() + }, + ); + assert_eq!(admission, MediaAdmission::Disabled); +} + +#[test] +fn an_unrecognized_boolean_value_stops_startup_as_upstream_throws() { + let outcome = with_env( + &[ + ("LLAMA_ARG_MMPROJ_OFFLOAD", Some("sometimes")), + ("LLAMA_ARG_NO_MMPROJ_OFFLOAD", None), + ], + || args().apply_env_bindings(), + ); + let (var, raw) = outcome.expect_err("an unrecognized value is an error"); + assert_eq!(var, "LLAMA_ARG_MMPROJ_OFFLOAD"); + assert_eq!(raw, "sometimes"); +} + +#[test] +fn llama_arg_mmproj_offload_off_reaches_the_rejection() { + let rejection = with_env( + &[ + ("LLAMA_ARG_MMPROJ_OFFLOAD", Some("off")), + ("LLAMA_ARG_NO_MMPROJ_OFFLOAD", None), + ], + || { + let mut a = args(); + a.apply_env_bindings().expect("a recognized value"); + a.rejection() + }, + ); + assert_eq!( + rejection.expect("host offload is refused").option, + "--no-mmproj-offload" + ); +} + +#[test] +fn an_explicit_command_line_half_wins_over_the_environment() { + let admission = with_env( + &[ + ("LLAMA_ARG_MMPROJ_AUTO", Some("off")), + ("LLAMA_ARG_NO_MMPROJ_AUTO", None), + ], + || { + let mut a = args(); + a.mmproj_auto = true; + a.apply_env_bindings().expect("no parse needed"); + a.media_admission() + }, + ); + assert_eq!(admission, MediaAdmission::Auto); +} diff --git a/src/commands/serve.rs b/src/commands/serve.rs index 9cb20d4ea..fb413f206 100644 --- a/src/commands/serve.rs +++ b/src/commands/serve.rs @@ -86,6 +86,43 @@ async fn run_serve_async(mut args: crate::ServeArgs) -> anyhow::Result<()> { .ensure_inert_before_model() .map_err(|rejection| anyhow::anyhow!("{rejection}"))?; + // b10621 multimodal projector / media options (issue #1451): classified + // before the model reference resolves, like the GGML and chat-template + // groups, so `--mmproj projector.gguf` is reported immediately rather than + // after a multi-gigabyte download. The requested image-token budget and the + // `--media-path` root are installed process-wide here, before the first + // load and before the server accepts a request that could name a local + // file. + args.multimodal_compat + .apply_env_bindings() + .map_err(|(var, raw)| anyhow::anyhow!("{var} has an invalid boolean value {raw:?}"))?; + args.multimodal_compat + .ensure_inert() + .map_err(|rejection| anyhow::anyhow!("{rejection}"))?; + let image_token_bounds = args + .multimodal_compat + .image_token_bounds() + .map_err(|rejection| anyhow::anyhow!("{rejection}"))?; + mlxcel::vision::image_token_overrides::install( + mlxcel::vision::image_token_overrides::ImageTokenOverride::from_bounds( + image_token_bounds.min_tokens, + image_token_bounds.max_tokens, + ), + ) + .map_err(|message| anyhow::anyhow!("{message}"))?; + mlxcel::server::media_root::install_media_root( + args.multimodal_compat + .resolve_media_root() + .map_err(|message| anyhow::anyhow!("{message}"))?, + ) + .map_err(|message| anyhow::anyhow!("{message}"))?; + mlxcel::server::configure_media_admission( + args.multimodal_compat.media_admission().is_disabled(), + ); + mlxcel::server::configure_private_media_urls( + mlxcel::server::private_media_urls_allowed_from_env(), + ); + // b10621 chat-template / reasoning / parsing options (issue #1447): // classified before the model reference resolves, like the GGML group, so // `--reasoning-format legacy` is reported immediately. diff --git a/src/commands/serve_tests.rs b/src/commands/serve_tests.rs index 612f7be24..e71f873d3 100644 --- a/src/commands/serve_tests.rs +++ b/src/commands/serve_tests.rs @@ -125,7 +125,7 @@ fn sample_args() -> crate::ServeArgs { _no_webui: false, ggml_compat: Default::default(), chat_compat: Default::default(), - _mmproj: None, + multimodal_compat: Default::default(), cache_compat: mlxcel::cli::cache_args::CacheCompatArgs::default(), infill: mlxcel::cli::infill_args::InfillArgs::default(), estimate_memory: false, diff --git a/src/main.rs b/src/main.rs index 948534ced..2b9b08504 100644 --- a/src/main.rs +++ b/src/main.rs @@ -21,6 +21,7 @@ use mlxcel::cli::batch_quant_args::BatchKvQuantArgs; use mlxcel::cli::cache_args::CacheCompatArgs; use mlxcel::cli::chat_compat_args::ChatCompatArgs; use mlxcel::cli::ggml_compat_args::GgmlCompatArgs; +use mlxcel::cli::multimodal_compat_args::MultimodalCompatArgs; use mlxcel::cli::rope_args::RopeOverrideArgs; use mlxcel::cli::speculative_args::SpeculativeArgs; use mlxcel::cli::turbo_args::TurboKvCacheArgs; @@ -1855,10 +1856,6 @@ pub(crate) struct ServeArgs { #[arg(long, hide = true)] _no_webui: bool, - /// Accepted for llama-server CLI compatibility (ignored, vision projector loaded automatically) - #[arg(long, hide = true)] - _mmproj: Option, - /// Decode storage backend for continuous batching. /// /// Accepted values: `auto`, `dense`, `paged`. When omitted, the server @@ -2013,6 +2010,17 @@ pub(crate) struct ServeArgs { #[command(flatten)] chat_compat: ChatCompatArgs, + /// llama-server b10621 multimodal projector and media options + /// (`--mmproj`, `--mmproj-url`, `--mmproj-auto`, `--mmproj-offload`, + /// `--mmproj-device`, `--image-min-tokens`, `--image-max-tokens`, + /// `--mtmd-batch-max-tokens`, `--media-path`). Every projector flag is a + /// hidden compatibility surface classified at startup like the GGML group; + /// `--media-path` is a real mlxcel feature and is visible. Defined once in + /// `mlxcel::cli::multimodal_compat_args` so both server binaries accept + /// exactly the same set (issue #1451). + #[command(flatten)] + multimodal_compat: MultimodalCompatArgs, + /// Continuous-batching KV quantization flag group /// (`--kv-bits`, `--kv-group-size`, `--kv-quant-scheme`, /// `--kv-skip-last-layer`). Defined once in diff --git a/src/server/media.rs b/src/server/media.rs index 7fca31823..ef27da6fb 100644 --- a/src/server/media.rs +++ b/src/server/media.rs @@ -29,6 +29,8 @@ use std::{ use crate::multimodal::video::{TempFile, VideoSource, is_video_file}; +use super::media_net; +use super::media_root; use super::types::ChatCompletionRequest; use super::types::request::{InputAudio, VideoUrl}; @@ -307,6 +309,82 @@ impl ImageInputLimits { } } +/// Set by b10621's `--no-mmproj` / `--no-mmproj-auto` (issue #1451). +/// +/// Upstream's flag leaves the multimodal projector unloaded and every media +/// part in a request is then refused. mlxcel cannot leave the projector +/// unloaded, because it is inside the checkpoint, so it declines the same +/// requests instead. Process-wide, like the image limits below, because it is +/// an operator setting that predates any request. +static MEDIA_ADMISSION_DISABLED: std::sync::atomic::AtomicBool = + std::sync::atomic::AtomicBool::new(false); + +/// Record whether the operator disabled multimodal admission. +pub fn configure_media_admission(disabled: bool) { + MEDIA_ADMISSION_DISABLED.store(disabled, Ordering::Relaxed); +} + +/// True when `--no-mmproj` / `--no-mmproj-auto` was passed. +#[must_use] +pub fn media_admission_disabled() -> bool { + MEDIA_ADMISSION_DISABLED.load(Ordering::Relaxed) +} + +/// The b10621 refusal for a media part the loaded server will not consume. +/// +/// Upstream's wording is `" input is not supported - hint: if this is +/// unexpected, you may need to provide the mmproj"`. The leading clause is kept +/// verbatim, because that is what a llama-server client matches on; the hint is +/// replaced, because mlxcel has no separate projector file to provide and +/// telling an operator to attach a GGUF `mmproj` to an MLX checkpoint would be +/// false (issue #1451). +#[must_use] +pub(crate) fn media_kind_refusal(kind: &str, model_id: &str) -> String { + // Assembled by concatenation rather than as one long literal with escaped + // line continuations: rustfmt rewrites those continuations into literal + // runs of spaces, which then appear in the message a client reads. + let mut message = format!("{kind} input is not supported - hint: "); + if media_admission_disabled() { + message.push_str("--no-mmproj disabled multimodal input on this server"); + return message; + } + message.push_str(&format!( + "the loaded checkpoint '{model_id}' has no {kind} tower; " + )); + message.push_str("mlxcel loads an integrated MLX VLM checkpoint, so serve one to send "); + message.push_str(kind); + message.push_str(" input"); + message +} + +/// Refuse a request whose media parts the loaded checkpoint cannot consume. +/// +/// Runs at the HTTP boundary, before any byte of a referenced image or audio +/// file is fetched, so a text-only checkpoint never becomes a fetch proxy +/// either. Returns the first offending modality in a fixed order. +#[must_use] +pub(crate) fn media_capability_rejection( + request: &ChatCompletionRequest, + support: crate::server::state::ModelMediaSupport, + model_id: &str, +) -> Option { + let refuse = |kind: &str| { + Some(crate::server::types::ErrorResponse::not_supported( + media_kind_refusal(kind, model_id), + )) + }; + if !support.image && !request.image_urls().is_empty() { + return refuse("image"); + } + if !support.audio && !request.audio_inputs().is_empty() { + return refuse("audio"); + } + if !support.video && !request.video_urls().is_empty() { + return refuse("video"); + } + None +} + static MAX_IMAGE_PAYLOAD_BYTES: AtomicUsize = AtomicUsize::new(DEFAULT_MAX_IMAGE_PAYLOAD_SIZE); static MAX_IMAGES_PER_REQUEST: AtomicUsize = AtomicUsize::new(DEFAULT_MAX_IMAGES_PER_REQUEST); static MAX_IMAGE_WIDTH: AtomicU32 = AtomicU32::new(DEFAULT_MAX_IMAGE_WIDTH); @@ -768,13 +846,28 @@ async fn fetch_remote_video(url: &str) -> Option<(PathBuf, TempFile)> { return None; } }; - let response = match client.get(url).send().await { + // Same address policy as the image/audio fetch path (issue #1451). + let parsed = match media_net::ensure_media_url_allowed(url).await { + Ok(parsed) => parsed, + Err(err) => { + tracing::warn!("Video URL rejected {}: {}", url, err); + return None; + } + }; + let host = parsed.host_str().unwrap_or_default().to_owned(); + let response = match client.get(parsed).send().await { Ok(r) => r, Err(err) => { tracing::warn!("Failed to fetch video URL {}: {}", url, err); return None; } }; + if let Some(peer) = response.remote_addr() + && let Err(err) = media_net::ensure_socket_addr_allowed(&host, peer) + { + tracing::warn!("Video URL rejected {}: {}", url, err); + return None; + } let response = match response.error_for_status() { Ok(r) => r, Err(err) => { @@ -1022,10 +1115,10 @@ async fn read_audio_input( return validate_audio_size(bytes, clip_index, max_payload_bytes); } - // file:// prefix - if let Some(path) = data.strip_prefix("file://") { - let bytes = read_local_bytes_with_limit_cancel( - Path::new(path), + // file:// prefix, confined to the `--media-path` root (issue #1451). + if data.starts_with("file://") { + let bytes = read_confined_bytes_with_limit_cancel( + data, "audio", max_payload_bytes, Some(cancelled), @@ -1044,10 +1137,12 @@ async fn read_audio_input( return validate_audio_size(bytes, clip_index, max_payload_bytes); } - // Bare local path - if Path::new(data).is_file() { - let bytes = read_local_bytes_with_limit_cancel( - Path::new(data), + // Bare relative path under the configured `--media-path` root. Without a + // root, the value falls through to the raw-base64 branch below, which is + // what b10621 does with a string that is neither a URL nor a data URI. + if !data.contains("://") && media_root::media_root().is_some() { + let bytes = read_confined_bytes_with_limit_cancel( + data, "audio", max_payload_bytes, Some(cancelled), @@ -1189,8 +1284,10 @@ pub(crate) async fn try_read_image_url_with_limits( return decode_data_uri_with_limit(url, "image", limits.max_payload_bytes).map(Some); } - if let Some(path) = url.strip_prefix("file://") { - return read_local_bytes_with_limit(Path::new(path), "image", limits.max_payload_bytes) + if url.starts_with("file://") { + // Confined to the `--media-path` root (issue #1451). Before that, this + // branch opened any absolute path the request named. + return read_confined_bytes_with_limit(url, "image", limits.max_payload_bytes) .await .map(Some); } @@ -1201,8 +1298,11 @@ pub(crate) async fn try_read_image_url_with_limits( .map(Some); } - if Path::new(url).is_file() { - return read_local_bytes_with_limit(Path::new(url), "image", limits.max_payload_bytes) + if !url.contains("://") && media_root::media_root().is_some() { + // A bare relative path is what an operator who configured + // `--media-path` writes; it resolves against the same confined root as + // a `file://` URL and may not leave it. + return read_confined_bytes_with_limit(url, "image", limits.max_payload_bytes) .await .map(Some); } @@ -1254,10 +1354,23 @@ async fn fetch_remote_bytes_with_limit_cancel( cancelled: Option<&dyn AudioAcquisitionCancellation>, ) -> Result> { check_media_cancel(cancelled, 0)?; - let response = match http_image_client().await?.get(url).send().await { + // Address policy before the socket opens: scheme, credentials, IP literal, + // and every address the host name resolves to (issue #1451). + let parsed = media_net::ensure_media_url_allowed(url) + .await + .map_err(|err| anyhow!("{kind} URL rejected: {err}"))?; + let host = parsed.host_str().unwrap_or_default().to_owned(); + let response = match http_image_client().await?.get(parsed).send().await { Ok(response) => response, Err(err) => bail!("Failed to fetch {kind} URL {url}: {err}"), }; + // Post-connect re-check: the peer the request actually reached. This is + // what closes the DNS-rebinding window the pre-flight resolution leaves, + // and it runs before any body byte is read. + if let Some(peer) = response.remote_addr() { + media_net::ensure_socket_addr_allowed(&host, peer) + .map_err(|err| anyhow!("{kind} URL rejected: {err}"))?; + } let response = match response.error_for_status() { Ok(response) => response, @@ -1291,35 +1404,65 @@ async fn fetch_remote_bytes_with_limit_cancel( Ok(bytes) } -async fn read_local_bytes_with_limit(path: &Path, kind: &str, max_size: usize) -> Result> { - read_local_bytes_with_limit_cancel(path, kind, max_size, None).await +/// Read a local media reference confined to the `--media-path` root. +/// +/// Every local read on the request path goes through here (issue #1451): +/// [`media_root::resolve_media_file`] applies b10621's own name validation plus +/// the canonicalize-and-contain rules, and [`media_root::open_confined`] takes +/// the resulting file with `O_NOFOLLOW` so the resolve-to-open window cannot be +/// won by a symlink swap. +async fn read_confined_bytes_with_limit( + reference: &str, + kind: &str, + max_size: usize, +) -> Result> { + read_confined_bytes_with_limit_cancel(reference, kind, max_size, None).await } -async fn read_local_bytes_with_limit_cancel( - path: &Path, +async fn read_confined_bytes_with_limit_cancel( + reference: &str, kind: &str, max_size: usize, cancelled: Option<&dyn AudioAcquisitionCancellation>, ) -> Result> { - use tokio::io::AsyncReadExt; - check_media_cancel(cancelled, 0)?; - match tokio::fs::metadata(path).await { + let canonical = media_root::resolve_media_file(reference) + .await + .map_err(|err| anyhow!("{kind} file rejected: {err}"))?; + // The size preflight runs against the resolved path, so an oversized file + // is refused before a single byte is copied into the process. + match tokio::fs::metadata(&canonical).await { Ok(meta) if meta.len() > max_size as u64 => { bail!( "{kind} payload too large: file {} is {} bytes, maximum is {} bytes", - path.display(), + canonical.display(), meta.len(), max_size ); } Ok(_) => {} - Err(err) => bail!("Failed to stat {kind} file {}: {err}", path.display()), + Err(err) => bail!("Failed to stat {kind} file {}: {err}", canonical.display()), } - - let mut file = tokio::fs::File::open(path) + let file = media_root::open_confined(&canonical) .await - .with_context(|| format!("Failed to open {kind} file {}", path.display()))?; + .map_err(|err| anyhow!("{kind} file rejected: {err}"))?; + read_open_file_with_limit(file, &canonical, kind, max_size, cancelled).await +} + +/// Stream an already-opened file into memory under a byte cap. +/// +/// Split out of the former `read_local_bytes_with_limit_cancel` so the confined +/// resolver can hand over the file descriptor it opened with `O_NOFOLLOW` +/// rather than re-opening the path. +async fn read_open_file_with_limit( + mut file: tokio::fs::File, + label: &Path, + kind: &str, + max_size: usize, + cancelled: Option<&dyn AudioAcquisitionCancellation>, +) -> Result> { + use tokio::io::AsyncReadExt; + let mut bytes = Vec::new(); let mut chunk = [0u8; 64 * 1024]; loop { @@ -1327,7 +1470,7 @@ async fn read_local_bytes_with_limit_cancel( let read = file .read(&mut chunk) .await - .with_context(|| format!("Failed to read {kind} file {}", path.display()))?; + .with_context(|| format!("Failed to read {kind} file {}", label.display()))?; if read == 0 { break; } @@ -1396,8 +1539,10 @@ async fn http_image_client() -> Result<&'static reqwest::Client> { // than necessary. .connect_timeout(Duration::from_secs(5)) // Bound redirect chains so a malicious origin cannot - // bounce the client through unbounded hops. - .redirect(reqwest::redirect::Policy::limited(5)); + // bounce the client through unbounded hops, and refuse a hop + // whose target leaves the public address space or the + // http(s) schemes (issue #1451). + .redirect(media_net::media_redirect_policy()); // See `downloader::load_extra_ca_certificates` — this client // fetches remote media URLs over the same rustls-tls backend, // so it needs the same MLXCEL_EXTRA_CA_CERTS trust anchors to diff --git a/src/server/media_net.rs b/src/server/media_net.rs new file mode 100644 index 000000000..e0cea5bc8 --- /dev/null +++ b/src/server/media_net.rs @@ -0,0 +1,377 @@ +// Copyright 2025-2026 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Network-address policy for remote media URLs (issue #1451). +//! +//! A request may carry `{"type": "image_url", "image_url": {"url": "http://..."}}` +//! and the server fetches it. That makes the server a fetch proxy an +//! unauthenticated caller steers, which is the classic server-side request +//! forgery shape: `http://127.0.0.1:9090/admin`, +//! `http://169.254.169.254/latest/meta-data/iam/security-credentials/` on EC2, +//! `http://metadata.google.internal/` on GCE, or any address inside the +//! deployment's own network. b10621 fetches with `common_remote_get_content` +//! and applies no address policy at all; mlxcel refuses these addresses, which +//! is recorded as a divergence on the media entries it applies to. +//! +//! # The three checks +//! +//! 1. **Before the request.** The scheme must be `http` or `https`, the URL may +//! not carry credentials, and a host written as an IP literal is checked +//! directly. A host written as a name is resolved and every address it +//! resolves to must be allowed, so a name that maps to `127.0.0.1` is +//! refused before a socket is opened. +//! 2. **On every redirect.** The redirect policy is consulted with the *next* +//! URL before it is followed, so a public origin cannot bounce the fetch to +//! an internal one. Hop count stays bounded at five, as before. +//! 3. **After the connection.** [`reqwest::Response::remote_addr`] reports the +//! peer the request actually reached. Re-checking it before the body is read +//! closes the DNS-rebinding window that checks 1 and 2 leave open: a name +//! that resolves publicly at check time and privately at connect time is +//! caught here, after connecting but before any bytes are consumed. +//! +//! # The escape hatch +//! +//! A deployment that genuinely serves its media from an internal object store +//! sets `MLXCEL_ALLOW_PRIVATE_MEDIA_URLS=1`, which turns every address check +//! into a pass. It is off by default because a fetch proxy that reaches the +//! private network has to be an explicit operator decision. +//! +//! Used by: chat / responses / anthropic image and audio parts, embeddings and +//! rerank image inputs, request-path video fetches. + +use std::net::{IpAddr, SocketAddr}; + +/// Environment variable that disables the private-address refusal. +pub(crate) const ALLOW_PRIVATE_MEDIA_URLS_ENV: &str = "MLXCEL_ALLOW_PRIVATE_MEDIA_URLS"; + +/// Why a remote media URL was refused before, during, or after the fetch. +#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)] +pub(crate) enum MediaUrlError { + #[error("media URL is not a valid absolute URL: {url}")] + Malformed { url: String }, + #[error("media URL scheme {scheme:?} is not supported; only http and https are fetched")] + Scheme { scheme: String }, + #[error("media URL must not carry credentials")] + Credentials, + #[error("media URL has no host")] + NoHost, + #[error( + "media URL host {host} resolves to {address}, which is a loopback, link-local, private \ + or otherwise non-public address; set {ALLOW_PRIVATE_MEDIA_URLS_ENV}=1 to allow fetching \ + media from inside the deployment's own network" + )] + BlockedAddress { host: String, address: IpAddr }, + #[error("media URL host {host} does not resolve")] + Unresolvable { host: String }, + #[error("media URL redirect chain exceeded 5 hops at {url}")] + TooManyRedirects { url: String }, +} + +/// Whether private addresses are reachable, resolved once at startup. +/// +/// Read from an atomic rather than from the environment on every fetch: the +/// setting is an operator decision that predates any request, and a per-request +/// `std::env::var` in the hot path would also be an unsynchronized environment +/// read next to whatever else a process is doing with `set_var`. +static ALLOW_PRIVATE_ADDRESSES: std::sync::atomic::AtomicBool = + std::sync::atomic::AtomicBool::new(false); + +/// Record whether media fetches may reach private addresses. +pub fn configure_private_media_urls(allowed: bool) { + ALLOW_PRIVATE_ADDRESSES.store(allowed, std::sync::atomic::Ordering::Relaxed); +} + +/// Read the opt-in from the environment, for startup to hand to +/// [`configure_private_media_urls`]. +#[must_use] +pub fn private_media_urls_allowed_from_env() -> bool { + std::env::var(ALLOW_PRIVATE_MEDIA_URLS_ENV) + .is_ok_and(|value| matches!(value.trim(), "1" | "true" | "on" | "yes")) +} + +/// True when the operator opted into fetching media from private addresses. +fn private_addresses_allowed() -> bool { + ALLOW_PRIVATE_ADDRESSES.load(std::sync::atomic::Ordering::Relaxed) +} + +/// True when `addr` is a public unicast address a media fetch may reach. +/// +/// Everything else is refused: loopback, the unspecified address, multicast and +/// broadcast, IPv4 private and link-local ranges (which is what makes +/// `169.254.169.254` unreachable), carrier-grade NAT, the documentation and +/// benchmark ranges, IPv6 unique-local and link-local, and any IPv4-mapped or +/// IPv4-compatible IPv6 form of a refused IPv4 address. +#[must_use] +pub(crate) fn is_public_address(addr: IpAddr) -> bool { + match addr { + IpAddr::V4(v4) => { + { + let [a, b, c, _] = v4.octets(); + // 100.64.0.0/10 carrier-grade NAT, 192.0.0.0/24 IETF protocol + // assignments, 198.18.0.0/15 benchmarking, 192.0.2.0/24 / + // 198.51.100.0/24 / 203.0.113.0/24 documentation, 240.0.0.0/4 + // reserved. `Ipv4Addr::is_global` and `is_documentation` cover + // these on nightly only, so they are spelled out here, each at + // its exact prefix so no ordinary public address is refused. + let reserved = (a == 100 && (64..128).contains(&b)) + || (a == 192 && b == 0 && c == 0) + || (a == 192 && b == 0 && c == 2) + || (a == 198 && (18..20).contains(&b)) + || (a == 198 && b == 51 && c == 100) + || (a == 203 && b == 0 && c == 113) + || a >= 240; + if reserved { + return false; + } + } + !(v4.is_loopback() + || v4.is_private() + || v4.is_link_local() + || v4.is_unspecified() + || v4.is_multicast() + || v4.is_broadcast()) + } + IpAddr::V6(v6) => { + // The IPv6-native rules come first. `::1` and `::` both live inside + // the deprecated IPv4-compatible `::/96` block, so converting to + // IPv4 before testing them would turn `::1` into `0.0.0.1` and let + // loopback through. + if v6.is_loopback() || v6.is_unspecified() || v6.is_multicast() { + return false; + } + // An IPv4 address written in IPv4-mapped form must be judged by its + // IPv4 rules, or `::ffff:127.0.0.1` walks straight past them. + if let Some(mapped) = v6.to_ipv4_mapped() { + return is_public_address(IpAddr::V4(mapped)); + } + let segments = v6.segments(); + // The rest of `::/96` is the deprecated IPv4-compatible form, which + // no reachable public host uses; refuse the whole block rather than + // reasoning about the embedded address. + if segments[..6] == [0, 0, 0, 0, 0, 0] { + return false; + } + // fc00::/7 unique local, fe80::/10 link local, 2001:db8::/32 + // documentation. + let unique_local = segments[0] & 0xfe00 == 0xfc00; + let link_local = segments[0] & 0xffc0 == 0xfe80; + let documentation = segments[0] == 0x2001 && segments[1] == 0x0db8; + !(unique_local || link_local || documentation) + } + } +} + +/// The IP address a host component names, when it is written as a literal. +/// +/// `Url::host_str` renders an IPv6 literal in its bracketed form, so the +/// brackets are stripped before parsing. +#[must_use] +fn host_ip_literal(host: &str) -> Option { + let unbracketed = host + .strip_prefix('[') + .and_then(|rest| rest.strip_suffix(']')) + .unwrap_or(host); + unbracketed.parse::().ok() +} + +/// Validate the shape of a media URL and its host when written as an IP literal. +/// +/// Synchronous: this is the half that needs no name resolution, so it can run +/// inside a redirect policy callback. +/// +/// # Errors +/// +/// Returns the first violation among scheme, credentials, missing host, and a +/// blocked IP literal. +pub(crate) fn ensure_media_url_shape(url: &reqwest::Url) -> Result<(), MediaUrlError> { + ensure_media_url_shape_with(url, private_addresses_allowed()) +} + +/// [`ensure_media_url_shape`] against an explicit opt-in value. +/// +/// The policy tests call this rather than the global-reading wrapper, so a test +/// that flips the process-wide opt-in for its own loopback origin cannot make +/// them pass vacuously. +pub(crate) fn ensure_media_url_shape_with( + url: &reqwest::Url, + allow_private: bool, +) -> Result<(), MediaUrlError> { + match url.scheme() { + "http" | "https" => {} + scheme => { + return Err(MediaUrlError::Scheme { + scheme: scheme.to_owned(), + }); + } + } + if !url.username().is_empty() || url.password().is_some() { + return Err(MediaUrlError::Credentials); + } + let Some(host) = url.host_str() else { + return Err(MediaUrlError::NoHost); + }; + if allow_private { + return Ok(()); + } + if let Some(address) = host_ip_literal(host) + && !is_public_address(address) + { + return Err(MediaUrlError::BlockedAddress { + host: host.to_owned(), + address, + }); + } + Ok(()) +} + +/// Validate a media URL's shape and every address its host resolves to. +/// +/// # Errors +/// +/// Returns the shape violations of [`ensure_media_url_shape`], plus a blocked +/// resolved address or an unresolvable host. +pub(crate) async fn ensure_media_url_allowed(raw: &str) -> Result { + let url = reqwest::Url::parse(raw).map_err(|_| MediaUrlError::Malformed { + url: raw.to_owned(), + })?; + ensure_media_url_shape(&url)?; + if private_addresses_allowed() { + return Ok(url); + } + let Some(host) = url.host_str() else { + return Err(MediaUrlError::NoHost); + }; + if host_ip_literal(host).is_some() { + // Already checked as a literal; there is nothing to resolve. + return Ok(url); + } + let port = url.port_or_known_default().unwrap_or(80); + let resolved = tokio::net::lookup_host((host.to_owned(), port)) + .await + .map_err(|_| MediaUrlError::Unresolvable { + host: host.to_owned(), + })?; + let mut any = false; + for addr in resolved { + any = true; + ensure_socket_addr_allowed(host, addr)?; + } + if !any { + return Err(MediaUrlError::Unresolvable { + host: host.to_owned(), + }); + } + Ok(url) +} + +/// Validate one concrete peer address, used both before and after connecting. +/// +/// # Errors +/// +/// Returns [`MediaUrlError::BlockedAddress`] for a non-public address. +pub(crate) fn ensure_socket_addr_allowed( + host: &str, + addr: SocketAddr, +) -> Result<(), MediaUrlError> { + ensure_socket_addr_allowed_with(host, addr, private_addresses_allowed()) +} + +/// [`ensure_socket_addr_allowed`] against an explicit opt-in value. +pub(crate) fn ensure_socket_addr_allowed_with( + host: &str, + addr: SocketAddr, + allow_private: bool, +) -> Result<(), MediaUrlError> { + if allow_private || is_public_address(addr.ip()) { + return Ok(()); + } + Err(MediaUrlError::BlockedAddress { + host: host.to_owned(), + address: addr.ip(), + }) +} + +/// Allow private media addresses for the duration of a test, restoring the +/// previous setting on drop. +/// +/// Serialized against every other holder, so a test that stands up a loopback +/// origin cannot overlap one that asserts the refusal. +#[cfg(test)] +#[must_use] +pub(crate) fn allow_private_media_urls_in_tests() -> PrivateAddressTestGuard { + private_media_urls_in_tests(true) +} + +/// Refuse private media addresses for the duration of a test. +/// +/// A test that asserts the refusal has to hold the same lock as one that opts +/// in, or the two overlap and the refusal never fires. +#[cfg(test)] +#[must_use] +pub(crate) fn deny_private_media_urls_in_tests() -> PrivateAddressTestGuard { + private_media_urls_in_tests(false) +} + +#[cfg(test)] +fn private_media_urls_in_tests(allowed: bool) -> PrivateAddressTestGuard { + static LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + let guard = LOCK + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + let previous = private_addresses_allowed(); + configure_private_media_urls(allowed); + PrivateAddressTestGuard { + _lock: guard, + previous, + } +} + +/// Restores the process-wide opt-in when dropped. +#[cfg(test)] +pub(crate) struct PrivateAddressTestGuard { + _lock: std::sync::MutexGuard<'static, ()>, + previous: bool, +} + +#[cfg(test)] +impl Drop for PrivateAddressTestGuard { + fn drop(&mut self) { + configure_private_media_urls(self.previous); + } +} + +/// Redirect policy for the shared media client. +/// +/// Bounds the chain at five hops, as before #1451, and additionally refuses a +/// hop whose target fails [`ensure_media_url_shape`], so a public origin cannot +/// redirect the fetch onto `http://169.254.169.254/` or a `file:` URL. Name +/// resolution is deliberately not attempted here: the policy callback is +/// synchronous, and the post-connect [`ensure_socket_addr_allowed`] check on +/// the final response covers the resolved address. +pub(crate) fn media_redirect_policy() -> reqwest::redirect::Policy { + reqwest::redirect::Policy::custom(|attempt| { + if attempt.previous().len() >= 5 { + let url = attempt.url().to_string(); + return attempt.error(MediaUrlError::TooManyRedirects { url }); + } + match ensure_media_url_shape(attempt.url()) { + Ok(()) => attempt.follow(), + Err(err) => attempt.error(err), + } + }) +} + +#[cfg(test)] +#[path = "media_net_tests.rs"] +mod tests; diff --git a/src/server/media_net_tests.rs b/src/server/media_net_tests.rs new file mode 100644 index 000000000..0a504f93e --- /dev/null +++ b/src/server/media_net_tests.rs @@ -0,0 +1,183 @@ +// Copyright 2025-2026 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Address-policy tests for remote media URLs (issue #1451). +//! +//! Nothing here opens a socket or resolves a name: every case goes through the +//! synchronous shape check or the pure address predicate, so the suite is +//! deterministic on a machine with no network and cannot be steered by whatever +//! the local resolver happens to answer. + +use super::*; + +fn url(raw: &str) -> reqwest::Url { + reqwest::Url::parse(raw).expect("test URL parses") +} + +#[test] +fn loopback_and_unspecified_addresses_are_refused() { + for addr in [ + "127.0.0.1", + "127.4.5.6", + "0.0.0.0", + "::1", + "::", + "::ffff:127.0.0.1", + ] { + let parsed: IpAddr = addr.parse().expect("address parses"); + assert!(!is_public_address(parsed), "{addr} must be refused"); + } +} + +#[test] +fn the_cloud_metadata_addresses_are_refused() { + // 169.254.169.254 is EC2 / GCE / Azure instance metadata, reachable from + // inside the instance and the single highest-value SSRF target. + for addr in ["169.254.169.254", "169.254.0.1", "fe80::1"] { + let parsed: IpAddr = addr.parse().expect("address parses"); + assert!(!is_public_address(parsed), "{addr} must be refused"); + } +} + +#[test] +fn private_and_reserved_ranges_are_refused() { + for addr in [ + "10.0.0.1", + "172.16.0.1", + "172.31.255.254", + "192.168.1.1", + "100.64.0.1", + "192.0.0.1", + "192.0.2.1", + "198.18.0.1", + "198.51.100.1", + "203.0.113.1", + "240.0.0.1", + "255.255.255.255", + "224.0.0.1", + "fc00::1", + "fd12:3456::1", + "2001:db8::1", + "ff02::1", + ] { + let parsed: IpAddr = addr.parse().expect("address parses"); + assert!(!is_public_address(parsed), "{addr} must be refused"); + } +} + +#[test] +fn ordinary_public_addresses_are_allowed() { + // Over-blocking is a compatibility bug, so the neighbours of every + // reserved prefix are asserted too. + for addr in [ + "8.8.8.8", + "1.1.1.1", + "172.15.0.1", + "172.32.0.1", + "100.63.255.255", + "100.128.0.1", + "192.0.1.1", + "192.0.3.1", + "198.17.255.255", + "198.20.0.1", + "198.51.99.1", + "198.51.101.1", + "203.0.112.1", + "203.0.114.1", + "8.8.4.4", + "2606:4700::1111", + "::ffff:8.8.8.8", + ] { + let parsed: IpAddr = addr.parse().expect("address parses"); + assert!(is_public_address(parsed), "{addr} must be allowed"); + } +} + +#[test] +fn a_non_http_scheme_is_refused() { + for raw in [ + "file:///etc/passwd", + "ftp://example.com/x.png", + "gopher://x/", + ] { + let parsed = reqwest::Url::parse(raw).expect("test URL parses"); + assert!( + matches!( + ensure_media_url_shape_with(&parsed, false), + Err(MediaUrlError::Scheme { .. }) + ), + "{raw} must be refused" + ); + } +} + +#[test] +fn credentials_in_a_media_url_are_refused() { + assert!(matches!( + ensure_media_url_shape_with(&url("http://user:pass@example.com/a.png"), false), + Err(MediaUrlError::Credentials) + )); + assert!(matches!( + ensure_media_url_shape_with(&url("http://user@example.com/a.png"), false), + Err(MediaUrlError::Credentials) + )); +} + +#[test] +fn an_ip_literal_host_is_checked_without_resolution() { + for raw in [ + "http://127.0.0.1:9090/admin", + "http://169.254.169.254/latest/meta-data/", + "http://[::1]:8080/x.png", + "http://10.1.2.3/internal.png", + "http://[::ffff:127.0.0.1]/x.png", + ] { + let parsed = url(raw); + assert!( + matches!( + ensure_media_url_shape_with(&parsed, false), + Err(MediaUrlError::BlockedAddress { .. }) + ), + "{raw} must be refused" + ); + } + assert!(ensure_media_url_shape_with(&url("https://8.8.8.8/a.png"), false).is_ok()); +} + +#[test] +fn a_public_hostname_passes_the_shape_check() { + // The shape check is name-agnostic; resolution happens in the async half. + assert!(ensure_media_url_shape_with(&url("https://example.com/a.png"), false).is_ok()); + assert!(ensure_media_url_shape_with(&url("http://example.com:8080/a.png"), false).is_ok()); +} + +#[test] +fn a_blocked_peer_address_is_reported_with_the_host_and_the_escape_hatch() { + let addr: SocketAddr = "127.0.0.1:9090".parse().expect("socket address"); + let err = ensure_socket_addr_allowed_with("images.example.com", addr, false) + .expect_err("loopback is refused"); + let rendered = err.to_string(); + assert!(rendered.contains("images.example.com"), "{rendered}"); + assert!(rendered.contains("127.0.0.1"), "{rendered}"); + assert!( + rendered.contains(ALLOW_PRIVATE_MEDIA_URLS_ENV), + "the operator has to be told how to opt in: {rendered}" + ); +} + +#[test] +fn a_public_peer_address_passes() { + let addr: SocketAddr = "93.184.216.34:443".parse().expect("socket address"); + assert!(ensure_socket_addr_allowed_with("example.com", addr, false).is_ok()); +} diff --git a/src/server/media_root.rs b/src/server/media_root.rs new file mode 100644 index 000000000..7f6c1a922 --- /dev/null +++ b/src/server/media_root.rs @@ -0,0 +1,305 @@ +// Copyright 2025-2026 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! The confined root local media URLs are resolved against (`--media-path`). +//! +//! This is the mlxcel side of llama-server b10621's `--media-path` (issue +//! #1451). Upstream keeps the value on `common_params::media_path`, appends the +//! platform separator to it at parse time, and resolves a request's `file://` +//! URL by string concatenation in +//! [`handle_media`](https://github.com/ggml-org/llama.cpp/blob/c1d0e7a004015f23bc0233470b747b596f29b264/tools/server/server-common.cpp): +//! +//! ```text +//! if (media_path.empty()) -> "file:// URLs are not allowed unless --media-path is specified" +//! file_path = url.substr(7) // everything after "file://" +//! if (!fs_validate_filename(file_path, true)) -> "file path is not allowed: ..." +//! std::ifstream file(media_path + file_path, std::ios::binary) +//! ``` +//! +//! Three properties of that upstream code decide this module's shape: +//! +//! 1. **It concatenates, it does not join.** `file:///etc/passwd` becomes +//! `//etc/passwd`, not `/etc/passwd`. A Rust [`Path::join`] with a +//! leading-slash relative part would replace the root instead of extending +//! it, so leading separators are stripped before joining. Getting this wrong +//! turns a compatibility feature into an arbitrary-file read. +//! 2. **It never percent-decodes.** `%2e%2e` names a file literally called +//! `%2e%2e`. Not decoding is therefore both compatible and safe, and this +//! module additionally refuses the traversal-shaped escapes outright so the +//! property is asserted rather than inherited from an absent call. +//! 3. **It is a pure string check.** `fs_validate_filename` rejects `..`, +//! control characters, the Windows-illegal punctuation, and a few Unicode +//! look-alikes, but it cannot see the filesystem: a symlink inside the media +//! root pointing at `/etc/shadow` passes every one of its rules and upstream +//! reads the target. mlxcel canonicalizes and requires the result to stay +//! inside the canonical root, then opens with `O_NOFOLLOW` so the +//! canonicalize-to-open window cannot be won by swapping the last component. +//! That is deliberately stricter than b10621 and is recorded as a divergence +//! on the `--media-path` manifest entry. +//! +//! Before #1451 mlxcel had no root at all: `try_read_image_url_with_limits` +//! opened any `file://` path, and any bare string that happened to name an +//! existing file, straight off the request. Every local read now goes through +//! [`resolve_media_file`]. +//! +//! Used by: chat / responses / anthropic image parts, chat audio parts, +//! embeddings and rerank image inputs. + +use std::path::{Path, PathBuf}; +use std::sync::OnceLock; + +/// b10621's own wording for a `file://` URL with no configured root. +pub(crate) const NO_MEDIA_ROOT_MESSAGE: &str = + "file:// URLs are not allowed unless --media-path is specified"; + +/// Why a local media reference was refused. +#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)] +pub(crate) enum MediaPathError { + /// No `--media-path` is configured, so local files are disabled. + #[error("{NO_MEDIA_ROOT_MESSAGE}")] + NoRoot, + /// The relative path failed the b10621-compatible name validation. + #[error("file path is not allowed: {path}")] + NotAllowed { path: String }, + /// The path does not resolve to an existing file under the root. + #[error("file does not exist or cannot be opened: {path}")] + Unresolvable { path: String }, + /// The resolved path left the configured root. + #[error("file path escapes the --media-path root: {path}")] + Escape { path: String }, + /// The resolved path is not a regular file. + #[error("file path is not a regular file: {path}")] + NotRegular { path: String }, +} + +/// The process-wide media root, installed once at startup. +static MEDIA_ROOT: OnceLock> = OnceLock::new(); + +/// Install the confined root before the server starts accepting requests. +/// +/// Installing the same value twice is accepted so the model-switch path, which +/// re-enters startup, is not an error. +/// +/// # Errors +/// +/// Returns `Err` when a different root was already installed. +pub fn install_media_root(root: Option) -> Result<(), String> { + match MEDIA_ROOT.set(root.clone()) { + Ok(()) => Ok(()), + Err(_) if MEDIA_ROOT.get() == Some(&root) => Ok(()), + Err(_) => Err(format!( + "a --media-path root is already installed ({:?}); it must be set once, before the \ + server starts", + MEDIA_ROOT.get() + )), + } +} + +/// The installed root, if the operator configured one. +pub(crate) fn media_root() -> Option<&'static Path> { + MEDIA_ROOT.get().and_then(|slot| slot.as_deref()) +} + +/// b10621's `fs_validate_filename(path, /*allow_subdirs=*/true)`, plus the +/// percent-encoded traversal refusal described in the module documentation. +/// +/// Upstream's rules, in its own order: non-empty, at most 255 bytes, valid +/// UTF-8 with no overlong encodings, no C0/C1 control characters or `DEL`, none +/// of the Unicode separator look-alikes (`U+FF0E`, `U+2215`, `U+2216`), no +/// UTF-16 surrogate, no `U+FFFD` or `U+FEFF`, none of `: * ? " < > |`, no +/// leading or trailing ASCII space, no trailing `.`, no `..` anywhere, and not +/// exactly `.`. +pub(crate) fn validate_media_filename(path: &str) -> Result<(), MediaPathError> { + let refuse = || { + Err(MediaPathError::NotAllowed { + path: path.to_owned(), + }) + }; + if path.is_empty() || path.len() > 255 { + return refuse(); + } + for c in path.chars() { + // `char` is already a scalar value, so Rust's own UTF-8 decoding has + // ruled out overlong encodings and surrogates before this loop; the + // remaining codepoint rules are upstream's. + if c <= '\u{1F}' + || c == '\u{7F}' + || ('\u{80}'..='\u{9F}').contains(&c) + || c == '\u{FF0E}' + || c == '\u{2215}' + || c == '\u{2216}' + || c == '\u{FFFD}' + || c == '\u{FEFF}' + || matches!(c, ':' | '*' | '?' | '"' | '<' | '>' | '|') + { + return refuse(); + } + } + if path.starts_with(' ') || path.ends_with(' ') || path.ends_with('.') { + return refuse(); + } + if path.contains("..") || path == "." { + return refuse(); + } + // Upstream never percent-decodes, so `%2e%2e` is a literal filename there + // and cannot traverse. Refusing the traversal-shaped escapes makes that a + // checked property here rather than one inherited from an absent call, and + // costs only filenames that literally spell a separator or a dot in + // percent-encoding. An ordinary `%20` is untouched. + if contains_encoded_traversal(path) { + return refuse(); + } + Ok(()) +} + +/// True when `path` carries a percent escape for `.`, `/`, `\` or NUL. +fn contains_encoded_traversal(path: &str) -> bool { + const ENCODED: [&str; 4] = ["%2e", "%2f", "%5c", "%00"]; + let lowered = path.to_ascii_lowercase(); + ENCODED.iter().any(|needle| lowered.contains(needle)) +} + +/// Strip the leading separators upstream's concatenation makes inert. +/// +/// `file:///etc/passwd` reaches upstream as `/etc/passwd` and is concatenated +/// onto the root, landing at `//etc/passwd`. Stripping the leading +/// separators and joining reproduces that; joining without stripping would let +/// the absolute path replace the root. +fn relative_component(path: &str) -> &str { + path.trim_start_matches(['/', '\\']) +} + +/// Resolve a request's local media reference to a canonical file inside the +/// configured root. +/// +/// `reference` is either the whole `file://...` URL or a bare relative path; +/// both are resolved identically, because a bare path is what an operator who +/// configured `--media-path` most naturally writes and neither may leave the +/// root. +/// +/// # Errors +/// +/// Every rejection is fail-closed: no root configured, a name upstream would +/// refuse, a path that does not resolve, a resolved path outside the root, or a +/// resolved path that is not a regular file. +pub(crate) async fn resolve_media_file(reference: &str) -> Result { + let Some(root) = media_root() else { + return Err(MediaPathError::NoRoot); + }; + resolve_media_file_in(root, reference).await +} + +/// [`resolve_media_file`] against an explicit root. +/// +/// The root must already be canonical; [`crate::cli::multimodal_compat_args::resolve_media_root_path`] +/// is what makes it so at startup. Separated from the global-reading wrapper so +/// the containment rules can be tested against a temporary directory without +/// installing a process-wide root. +pub(crate) async fn resolve_media_file_in( + root: &Path, + reference: &str, +) -> Result { + let raw = reference.strip_prefix("file://").unwrap_or(reference); + validate_media_filename(raw)?; + let relative = relative_component(raw); + if relative.is_empty() { + return Err(MediaPathError::NotAllowed { + path: raw.to_owned(), + }); + } + + let canonical = tokio::fs::canonicalize(root.join(relative)) + .await + .map_err(|_| MediaPathError::Unresolvable { + path: raw.to_owned(), + })?; + // The root was canonicalized at startup, so this prefix test compares two + // fully resolved paths: a symlink anywhere in `relative` has already been + // followed and shows up here as a canonical path outside the root. + if !canonical.starts_with(root) { + return Err(MediaPathError::Escape { + path: raw.to_owned(), + }); + } + let metadata = + tokio::fs::metadata(&canonical) + .await + .map_err(|_| MediaPathError::Unresolvable { + path: raw.to_owned(), + })?; + if !metadata.is_file() { + return Err(MediaPathError::NotRegular { + path: raw.to_owned(), + }); + } + Ok(canonical) +} + +/// Open a resolved media file without following a final symlink. +/// +/// `O_NOFOLLOW` closes the canonicalize-to-open window: a swap of the last +/// component for a symlink after [`resolve_media_file`] returned makes the +/// `open` fail with `ELOOP` instead of silently reading the new target. The +/// same primitive already guards the video path (`media.rs::open_video_source`). +/// +/// # Errors +/// +/// Returns [`MediaPathError::Unresolvable`] when the file cannot be opened, +/// which includes the symlink-swap case. +pub(crate) async fn open_confined(canonical: &Path) -> Result { + #[cfg(unix)] + { + // `custom_flags` comes from `std::os::unix::fs::OpenOptionsExt`, which + // `tokio::fs::OpenOptions` re-exposes on its own type. + tokio::fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_NOFOLLOW) + .open(canonical) + .await + .map_err(|_| MediaPathError::Unresolvable { + path: canonical.display().to_string(), + }) + } + #[cfg(not(unix))] + { + tokio::fs::File::open(canonical) + .await + .map_err(|_| MediaPathError::Unresolvable { + path: canonical.display().to_string(), + }) + } +} + +/// Install a shared, process-wide media root for the tests that need a +/// readable local file. +/// +/// The real root is a `OnceLock`, so the first installer in a test binary wins +/// and every later identical call is accepted. Routing every test that wants a +/// root through this one helper is what keeps that first-writer-wins rule from +/// depending on test order. +#[cfg(test)] +pub(crate) fn install_test_root_once() -> &'static Path { + static TEST_ROOT: OnceLock = OnceLock::new(); + let root = TEST_ROOT.get_or_init(|| { + let dir = std::env::temp_dir().join(format!("mlxcel-media-root-{}", uuid::Uuid::new_v4())); + std::fs::create_dir_all(&dir).expect("create the shared test media root"); + std::fs::canonicalize(&dir).expect("canonicalize the shared test media root") + }); + install_media_root(Some(root.clone())).expect("install the shared test media root"); + media_root().expect("the shared test media root is installed") +} + +#[cfg(test)] +#[path = "media_root_tests.rs"] +mod tests; diff --git a/src/server/media_root_tests.rs b/src/server/media_root_tests.rs new file mode 100644 index 000000000..5417e18f5 --- /dev/null +++ b/src/server/media_root_tests.rs @@ -0,0 +1,314 @@ +// Copyright 2025-2026 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Containment tests for the `--media-path` root (issue #1451). +//! +//! Every case here is an escape attempt or the legitimate access it must not +//! break. The resolver is exercised through [`resolve_media_file_in`], which +//! takes the root explicitly, so no test installs the process-wide `OnceLock` +//! and the cases can run in any order. + +use super::*; + +/// A media root holding `ok.png` and a `sub/` directory with `nested.png`. +struct Fixture { + _dir: tempfile::TempDir, + root: PathBuf, + outside: PathBuf, +} + +fn fixture() -> Fixture { + let dir = tempfile::tempdir().expect("tempdir"); + // The tempdir itself is the parent; the root is a directory inside it so + // there is somewhere outside the root but on the same filesystem to escape + // to. + let root = dir.path().join("media"); + std::fs::create_dir(&root).expect("create root"); + std::fs::write(root.join("ok.png"), b"PNG-BYTES").expect("write ok.png"); + std::fs::create_dir(root.join("sub")).expect("create sub"); + std::fs::write(root.join("sub/nested.png"), b"NESTED").expect("write nested"); + let outside = dir.path().join("secret.txt"); + std::fs::write(&outside, b"SECRET").expect("write secret"); + Fixture { + root: std::fs::canonicalize(&root).expect("canonical root"), + outside: std::fs::canonicalize(&outside).expect("canonical secret"), + _dir: dir, + } +} + +#[tokio::test] +async fn a_relative_file_url_resolves_inside_the_root() { + let f = fixture(); + let resolved = resolve_media_file_in(&f.root, "file://ok.png") + .await + .expect("a plain relative reference resolves"); + assert_eq!(resolved, f.root.join("ok.png")); + + let nested = resolve_media_file_in(&f.root, "file://sub/nested.png") + .await + .expect("subdirectories are allowed, as upstream allows them"); + assert_eq!(nested, f.root.join("sub/nested.png")); +} + +#[tokio::test] +async fn a_bare_relative_path_resolves_the_same_way() { + let f = fixture(); + let resolved = resolve_media_file_in(&f.root, "ok.png") + .await + .expect("a bare relative path resolves"); + assert_eq!(resolved, f.root.join("ok.png")); +} + +#[tokio::test] +async fn an_absolute_looking_path_is_concatenated_not_joined() { + // b10621 does `media_path + file_path`, so `file:///etc/passwd` lands at + // `//etc/passwd` and reads nothing. A `Path::join` would have + // replaced the root with `/etc/passwd`, which is the arbitrary-file read + // this test exists to keep out. + let f = fixture(); + let err = resolve_media_file_in(&f.root, "file:///etc/passwd") + .await + .expect_err("an absolute path may not escape"); + assert!( + matches!(err, MediaPathError::Unresolvable { .. }), + "expected an unresolvable path under the root, got {err:?}" + ); + + // The same absolute-looking form *inside* the root still resolves, which is + // what proves the leading separator was stripped rather than refused. + let resolved = resolve_media_file_in(&f.root, "file:///ok.png") + .await + .expect("a leading separator is stripped, not rejected"); + assert_eq!(resolved, f.root.join("ok.png")); +} + +#[tokio::test] +async fn dot_dot_traversal_is_refused() { + let f = fixture(); + for attempt in [ + "file://../secret.txt", + "file://sub/../../secret.txt", + "file://..%2fsecret.txt", + "file://....//secret.txt", + "../secret.txt", + ] { + assert!( + matches!( + resolve_media_file_in(&f.root, attempt).await, + Err(MediaPathError::NotAllowed { .. }) + ), + "{attempt} must be refused by the name validation" + ); + } +} + +#[tokio::test] +async fn percent_encoded_traversal_is_refused() { + let f = fixture(); + for attempt in [ + "file://%2e%2e/secret.txt", + "file://%2E%2E%2Fsecret.txt", + "file://sub%2f%2e%2e%2fsecret.txt", + "file://ok%00.png", + "file://sub%5c..%5csecret.txt", + ] { + assert!( + matches!( + resolve_media_file_in(&f.root, attempt).await, + Err(MediaPathError::NotAllowed { .. }) + ), + "{attempt} must be refused" + ); + } +} + +#[cfg(unix)] +#[tokio::test] +async fn a_symlink_pointing_outside_the_root_is_refused() { + let f = fixture(); + std::os::unix::fs::symlink(&f.outside, f.root.join("escape.png")).expect("symlink"); + let err = resolve_media_file_in(&f.root, "file://escape.png") + .await + .expect_err("a symlink target outside the root is refused"); + assert!( + matches!(err, MediaPathError::Escape { .. }), + "expected an escape, got {err:?}" + ); +} + +#[cfg(unix)] +#[tokio::test] +async fn a_symlinked_directory_component_cannot_escape_either() { + let f = fixture(); + let parent = f.root.parent().expect("root has a parent").to_path_buf(); + std::os::unix::fs::symlink(&parent, f.root.join("up")).expect("symlink"); + let err = resolve_media_file_in(&f.root, "file://up/secret.txt") + .await + .expect_err("an intermediate symlink is followed and then caught"); + assert!( + matches!(err, MediaPathError::Escape { .. }), + "expected an escape, got {err:?}" + ); +} + +#[cfg(unix)] +#[tokio::test] +async fn a_symlink_that_stays_inside_the_root_still_resolves() { + // Confinement is about where the target lands, not about symlinks as such; + // an operator who organises the media root with links keeps working. + let f = fixture(); + std::os::unix::fs::symlink(f.root.join("ok.png"), f.root.join("alias.png")).expect("symlink"); + let resolved = resolve_media_file_in(&f.root, "file://alias.png") + .await + .expect("an internal symlink resolves"); + assert_eq!(resolved, f.root.join("ok.png")); +} + +#[cfg(unix)] +#[tokio::test] +async fn open_confined_refuses_a_symlink_swapped_in_after_resolution() { + // `resolve_media_file_in` canonicalizes; `open_confined` then opens with + // `O_NOFOLLOW`, so a last-component swap between the two fails loudly + // instead of reading the new target. + let f = fixture(); + let victim = f.root.join("swapped.png"); + std::fs::write(&victim, b"REAL").expect("write"); + let resolved = resolve_media_file_in(&f.root, "file://swapped.png") + .await + .expect("resolves before the swap"); + std::fs::remove_file(&victim).expect("remove"); + std::os::unix::fs::symlink(&f.outside, &victim).expect("symlink swap"); + assert!( + open_confined(&resolved).await.is_err(), + "O_NOFOLLOW must refuse the swapped-in symlink" + ); +} + +#[tokio::test] +async fn a_directory_is_not_a_media_file() { + let f = fixture(); + let err = resolve_media_file_in(&f.root, "file://sub") + .await + .expect_err("a directory is refused"); + assert!( + matches!(err, MediaPathError::NotRegular { .. }), + "expected a non-regular file, got {err:?}" + ); +} + +#[cfg(unix)] +#[tokio::test] +async fn a_fifo_inside_the_root_is_refused_before_it_can_block() { + // Reading a FIFO would hang the request task until a writer appears. + let f = fixture(); + let fifo = f.root.join("pipe"); + let c_path = std::ffi::CString::new(fifo.as_os_str().as_encoded_bytes()).expect("cstring"); + // SAFETY: `c_path` is a valid NUL-terminated path inside a fresh tempdir, + // and `mkfifo` only creates a filesystem entry at it. + let rc = unsafe { libc::mkfifo(c_path.as_ptr(), 0o600) }; + assert_eq!(rc, 0, "mkfifo failed: {}", std::io::Error::last_os_error()); + let err = resolve_media_file_in(&f.root, "file://pipe") + .await + .expect_err("a FIFO is refused"); + assert!( + matches!(err, MediaPathError::NotRegular { .. }), + "expected a non-regular file, got {err:?}" + ); +} + +#[tokio::test] +async fn a_missing_file_reports_upstreams_wording() { + let f = fixture(); + let err = resolve_media_file_in(&f.root, "file://absent.png") + .await + .expect_err("a missing file is refused"); + assert!( + err.to_string() + .contains("file does not exist or cannot be opened"), + "{err}" + ); +} + +#[test] +fn with_no_root_configured_local_files_report_upstreams_own_sentence() { + // The wrapper's `NoRoot` arm is what an operator who never passed + // `--media-path` sees, and clients may match on the wording, so it is + // pinned here character for character. The arm is asserted through the + // error value rather than by calling the wrapper, because other tests in + // this binary install the process-wide root. + assert_eq!(MediaPathError::NoRoot.to_string(), NO_MEDIA_ROOT_MESSAGE); + assert_eq!( + NO_MEDIA_ROOT_MESSAGE, "file:// URLs are not allowed unless --media-path is specified", + "the wording is b10621's own and clients may match on it" + ); +} + +#[tokio::test] +async fn the_installed_root_is_what_the_wrapper_resolves_against() { + let root = install_test_root_once(); + std::fs::write(root.join("installed.png"), b"INSTALLED").expect("write"); + let resolved = resolve_media_file("file://installed.png") + .await + .expect("the installed root resolves"); + assert_eq!(resolved, root.join("installed.png")); + assert!( + resolve_media_file("file://../escape.png").await.is_err(), + "the wrapper applies the same containment rules as the explicit-root form" + ); +} + +#[test] +fn filename_validation_matches_upstreams_rule_set() { + // Accepted by b10621's `fs_validate_filename(path, allow_subdirs=true)`. + for ok in [ + "a.png", + "sub/dir/a.png", + "a b.png", + "ünïcödé.png", + "a%20b.png", + ] { + assert!(validate_media_filename(ok).is_ok(), "{ok} must be accepted"); + } + // Refused by it. + for bad in [ + "", + ".", + "..", + "a/../b", + "a.png ", + " a.png", + "a.png.", + "a:b.png", + "a*b.png", + "a?b.png", + "a\"b.png", + "ab.png", + "a|b.png", + "a\u{7F}.png", + "a\u{FF0E}\u{FF0E}/b.png", + "a\u{2215}b.png", + "a\u{2216}b.png", + "a\u{FFFD}.png", + "a\u{FEFF}.png", + ] { + assert!( + validate_media_filename(bad).is_err(), + "{bad:?} must be refused" + ); + } + assert!(validate_media_filename("a\n.png").is_err()); + assert!(validate_media_filename(&"a".repeat(256)).is_err()); + assert!(validate_media_filename(&"a".repeat(255)).is_ok()); +} diff --git a/src/server/media_tests.rs b/src/server/media_tests.rs index 0d226130b..8ca853975 100644 --- a/src/server/media_tests.rs +++ b/src/server/media_tests.rs @@ -23,7 +23,8 @@ use super::{ ImageInputLimits, MediaRequestMetadata, collect_image_data, extract_chat_image_data, extract_chat_video_paths_with_allowlist, read_image_url, scan_insecure_allowlist_dirs, try_collect_image_data_with_limits, try_extract_chat_audio_data, - try_extract_chat_audio_data_with_cancellation, try_read_image_url_with_limits, + try_extract_chat_audio_data_with_cancellation, try_extract_chat_image_data, + try_read_image_url_with_limits, }; use crate::server::types::request::{InputAudio, VideoUrl}; use crate::server::types::{ @@ -154,11 +155,15 @@ async fn local_audio_acquisition_cancels_between_bounded_read_chunks() { } } - let path = std::env::temp_dir().join(format!("mlxcel-audio-{}.wav", uuid::Uuid::new_v4())); + // Local media is confined to the `--media-path` root (issue #1451), so the + // fixture lives inside the shared test root and is named relative to it. + let root = crate::server::media_root::install_test_root_once(); + let name = format!("mlxcel-audio-{}.wav", uuid::Uuid::new_v4()); + let path = root.join(&name); fs::write(&path, vec![0u8; 128 * 1024]).unwrap(); let request = build_chat_request(vec![ContentPart::InputAudio { input_audio: InputAudio { - data: format!("file://{}", path.display()), + data: format!("file://{name}"), format: "wav".to_string(), }, }]); @@ -192,6 +197,10 @@ async fn aborting_audio_acquisition_task_drops_slow_http_stream() { const CHUNK_BYTES: usize = 16 * 1024; const CHUNK_COUNT: usize = 512; + // The origin is a loopback socket, which the request-path address policy + // refuses by default (issue #1451); this test is about stream teardown, so + // it opts in for its own duration. + let _private = crate::server::media_net::allow_private_media_urls_in_tests(); let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); let addr = listener.local_addr().unwrap(); let (first_chunk_tx, first_chunk_rx) = tokio::sync::oneshot::channel(); @@ -292,19 +301,52 @@ async fn read_image_url_rejects_malformed_data_uri() { } #[tokio::test] -async fn read_image_url_reads_bare_local_paths() { - let path = std::env::temp_dir().join(format!("mlxcel-media-{}.png", uuid::Uuid::new_v4())); +async fn read_image_url_reads_bare_paths_inside_the_media_root() { + // A bare relative path is resolved against the `--media-path` root, the + // same as a `file://` URL (issue #1451). Before that it was any path on + // the filesystem that happened to exist. + let root = crate::server::media_root::install_test_root_once(); + let name = format!("mlxcel-media-{}.png", uuid::Uuid::new_v4()); + let path = root.join(&name); let payload = tiny_png_bytes(); fs::write(&path, &payload).unwrap(); - let bytes = read_image_url(path.to_str().unwrap()).await.unwrap(); + let bytes = read_image_url(&name).await.unwrap(); assert_eq!(bytes, payload); + let outside = std::env::temp_dir().join(format!("mlxcel-outside-{}.png", uuid::Uuid::new_v4())); + fs::write(&outside, &payload).unwrap(); + assert!( + read_image_url(outside.to_str().unwrap()).await.is_none(), + "a bare absolute path outside the root is not readable" + ); + fs::remove_file(path).unwrap(); + fs::remove_file(outside).unwrap(); +} + +#[tokio::test] +async fn read_image_url_refuses_a_loopback_origin() { + // The address policy runs before the socket opens, so a request that names + // the server's own admin port gets a rejection rather than a fetch. The + // deny guard holds the same lock the opt-in tests take, so a concurrent + // loopback-origin test cannot make this pass or fail vacuously. + let _private = crate::server::media_net::deny_private_media_urls_in_tests(); + for origin in [ + "http://127.0.0.1:9090/admin", + "http://169.254.169.254/latest/meta-data/", + "http://[::1]:8080/x.png", + ] { + let err = try_read_image_url_with_limits(origin, ImageInputLimits::default()) + .await + .expect_err("a non-public origin is refused"); + assert!(err.to_string().contains("image URL rejected"), "{err}"); + } } #[tokio::test] async fn read_image_url_fetches_http_urls() { + let _private = crate::server::media_net::allow_private_media_urls_in_tests(); let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); let addr = listener.local_addr().unwrap(); let payload = tiny_png_bytes(); @@ -460,12 +502,14 @@ async fn read_image_url_rejects_oversized_data_uri() { } #[tokio::test] -async fn extract_chat_image_data_reads_file_urls() { - let path = std::env::temp_dir().join(format!("mlxcel-media-{}.bin", uuid::Uuid::new_v4())); +async fn extract_chat_image_data_reads_file_urls_inside_the_media_root() { + let root = crate::server::media_root::install_test_root_once(); + let name = format!("mlxcel-media-{}.bin", uuid::Uuid::new_v4()); + let path = root.join(&name); fs::write(&path, b"image-bytes").unwrap(); let request = build_chat_request(vec![ContentPart::ImageUrl { - image_url: ImageUrl::new(format!("file://{}", path.display())), + image_url: ImageUrl::new(format!("file://{name}")), }]); let images = extract_chat_image_data(&request).await; @@ -474,6 +518,39 @@ async fn extract_chat_image_data_reads_file_urls() { fs::remove_file(path).unwrap(); } +#[tokio::test] +async fn extract_chat_image_data_refuses_a_file_url_outside_the_media_root() { + // The pre-#1451 behavior: any absolute path the request named was opened. + // The root confines it now, so an absolute path outside the root resolves + // to nothing and the image is dropped rather than read. + crate::server::media_root::install_test_root_once(); + let outside = std::env::temp_dir().join(format!("mlxcel-outside-{}.bin", uuid::Uuid::new_v4())); + fs::write(&outside, b"secret-bytes").unwrap(); + + let request = build_chat_request(vec![ContentPart::ImageUrl { + image_url: ImageUrl::new(format!("file://{}", outside.display())), + }]); + // Image resolution is deliberately tolerant, so the refusal shows up as a + // resolved count of zero against one declared image; the shared cardinality + // validator is what turns that into a request rejection, rather than a + // silently text-only answer. + let resolved = try_extract_chat_image_data(&request) + .await + .expect("resolution itself is tolerant"); + assert!( + resolved.is_empty(), + "a path outside the root must not be read" + ); + assert!( + MediaRequestMetadata::new(1, 0, 0, resolved.len(), 0, 0) + .validate_resolved_image_count() + .is_err(), + "one declared image that resolved to nothing must reject the request" + ); + + fs::remove_file(outside).unwrap(); +} + #[tokio::test] async fn extract_chat_image_data_collects_images_across_messages() { let request = ChatCompletionRequest { @@ -900,6 +977,7 @@ async fn fetch_remote_video_streaming_rejects_oversized() { "test fixture assumes the documented 1 GiB cap" ); + let _private = crate::server::media_net::allow_private_media_urls_in_tests(); let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); let addr = listener.local_addr().unwrap(); // We declare a Content-Length larger than the cap, then start writing @@ -1194,3 +1272,113 @@ fn scan_insecure_allowlist_dirs_passes_strict_directory() { fs::set_permissions(&dir, fs::Permissions::from_mode(0o755)).unwrap(); fs::remove_dir_all(dir).unwrap(); } + +// ── modality gate at the request boundary (issue #1451) ───────────────────── + +#[test] +fn media_capability_rejection_refuses_each_modality_a_checkpoint_cannot_consume() { + use crate::server::state::ModelMediaSupport; + + let text_only = ModelMediaSupport::none(); + let image_request = build_chat_request(vec![ContentPart::ImageUrl { + image_url: ImageUrl::new("data:image/png;base64,aGVsbG8=".to_string()), + }]); + let rejection = super::media_capability_rejection(&image_request, text_only, "qwen3-0.6b-4bit") + .expect("a text-only checkpoint refuses an image part"); + // b10621 answers ERROR_TYPE_NOT_SUPPORTED, which is HTTP 501 with the + // `not_supported_error` type string. + assert_eq!(rejection.status, axum::http::StatusCode::NOT_IMPLEMENTED); + assert_eq!(rejection.error.error_type, "not_supported_error"); + assert!( + rejection + .error + .message + .starts_with("image input is not supported"), + "{}", + rejection.error.message + ); + assert!( + rejection.error.message.contains("qwen3-0.6b-4bit"), + "the refusal has to name the checkpoint that cannot serve it: {}", + rejection.error.message + ); + assert!( + !rejection.error.message.contains("provide the mmproj"), + "an integrated MLX checkpoint has no projector file to provide: {}", + rejection.error.message + ); + + let audio_request = build_chat_request(vec![ContentPart::InputAudio { + input_audio: InputAudio { + data: "aGVsbG8=".to_string(), + format: "wav".to_string(), + }, + }]); + assert!( + super::media_capability_rejection(&audio_request, text_only, "m") + .expect("audio is refused") + .error + .message + .starts_with("audio input is not supported") + ); + + let video_request = build_chat_request(vec![ContentPart::VideoUrl { + video_url: VideoUrl { + url: "file://clip.mp4".to_string(), + fps: None, + }, + }]); + assert!( + super::media_capability_rejection(&video_request, text_only, "m") + .expect("video is refused") + .error + .message + .starts_with("video input is not supported") + ); +} + +#[test] +fn media_capability_rejection_admits_what_the_checkpoint_supports() { + use crate::server::state::ModelMediaSupport; + + let vlm = ModelMediaSupport { + image: true, + audio: true, + video: false, + }; + let image_request = build_chat_request(vec![ContentPart::ImageUrl { + image_url: ImageUrl::new("data:image/png;base64,aGVsbG8=".to_string()), + }]); + assert!(super::media_capability_rejection(&image_request, vlm, "qwen2.5-vl-7b-4bit").is_none()); + + // Video stays gated per family, as it was before the modality gate existed. + let video_request = build_chat_request(vec![ContentPart::VideoUrl { + video_url: VideoUrl { + url: "file://clip.mp4".to_string(), + fps: None, + }, + }]); + assert!(super::media_capability_rejection(&video_request, vlm, "qwen2.5-vl-7b-4bit").is_some()); + + // A text-only request is never refused, whatever the checkpoint is. + let text = build_chat_request(vec![ContentPart::Text { + text: "hello".to_string(), + }]); + assert!(super::media_capability_rejection(&text, ModelMediaSupport::none(), "m").is_none()); +} + +#[test] +fn no_mmproj_refusal_names_the_flag_rather_than_the_checkpoint() { + // `--no-mmproj` is an operator decision, so the hint has to say so instead + // of blaming the checkpoint for lacking a tower it may well have. + let _guard = crate::test_support::env_lock::env_lock(); + let previous = super::media_admission_disabled(); + super::configure_media_admission(true); + let message = super::media_kind_refusal("image", "qwen2.5-vl-7b-4bit"); + super::configure_media_admission(previous); + assert!( + message.starts_with("image input is not supported"), + "{message}" + ); + assert!(message.contains("--no-mmproj"), "{message}"); +} diff --git a/src/server/mod.rs b/src/server/mod.rs index e6a492752..acf69f5c1 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -38,6 +38,8 @@ pub mod infill; pub mod kokoro_tts; mod listen; mod media; +mod media_net; +pub mod media_root; pub mod model_provider; pub mod model_source; pub mod prompt_cache; @@ -96,11 +98,13 @@ pub use config::{ RemotePipelineStageConfig, ServerConfig, ServerGenerateOptions, }; pub use cors::{CorsPolicy, OriginPolicy}; -pub(crate) use media::current_image_input_limits; pub use media::{ DEFAULT_MAX_IMAGE_DECODE_ALLOC_BYTES, DEFAULT_MAX_IMAGE_HEIGHT, DEFAULT_MAX_IMAGE_PAYLOAD_SIZE, DEFAULT_MAX_IMAGE_WIDTH, DEFAULT_MAX_IMAGES_PER_REQUEST, ImageInputLimits, + configure_media_admission, media_admission_disabled, }; +pub(crate) use media::{current_image_input_limits, media_capability_rejection}; +pub use media_net::{configure_private_media_urls, private_media_urls_allowed_from_env}; pub use model_provider::{GenerationResult, ModelProvider}; pub use model_source::{ LLAMA_ARG_OFFLINE, LlamaModelSourceArgs, ResolvedModelSource, env_fallback_offline, diff --git a/src/server/model_worker.rs b/src/server/model_worker.rs index 98aa092de..0638dc57b 100644 --- a/src/server/model_worker.rs +++ b/src/server/model_worker.rs @@ -290,6 +290,18 @@ pub(crate) fn spawn_model_worker_with_batch_config( tracing::error!("{message}"); return; } + // Same rule for a b10621 `--image-min-tokens` / + // `--image-max-tokens` budget that reached no image + // preprocessing path: the server would encode at the + // checkpoint's own resolution while the operator believed + // the budget applied (#1451). + if let Err(message) = + crate::vision::image_token_overrides::verify_applied(&worker_model_id) + { + chat_unavailable.store(true, Ordering::Release); + tracing::error!("{message}"); + return; + } loaded.store(true, Ordering::Release); (model, tokenizer) } @@ -989,6 +1001,18 @@ pub(crate) fn spawn_legacy_model_worker( tracing::error!("{message}"); return; } + // Same rule for a b10621 `--image-min-tokens` / + // `--image-max-tokens` budget that reached no image + // preprocessing path: the server would encode at the + // checkpoint's own resolution while the operator believed + // the budget applied (#1451). + if let Err(message) = + crate::vision::image_token_overrides::verify_applied(&worker_model_id) + { + chat_unavailable.store(true, Ordering::Release); + tracing::error!("{message}"); + return; + } loaded.store(true, Ordering::Release); (model, tokenizer) } @@ -1318,6 +1342,17 @@ pub(crate) fn prepare_request_vlm_embeddings( "Audio input is not supported by the loaded non-VLM model" )); } + // Images and videos used to fall through to the text-only branch below, so + // a request that sent a picture to a text-only checkpoint was answered + // fluently from the prompt alone and nothing said the picture was dropped + // (issue #1451). The HTTP boundary refuses this first; this is the + // defence-in-depth copy that covers any path that reaches the worker + // without passing the route gate. + if (!images.is_empty() || !videos.is_empty()) && !model.is_vlm() { + return Err(anyhow!( + "Image and video input are not supported by the loaded non-VLM model" + )); + } if !has_media || !model.is_vlm() { if !has_media && let LoadedModel::MuseGlimmerVLM(muse) = model { crate::multimodal::muse_glimmer_runtime::reject_muse_glimmer_text_fallback( diff --git a/src/server/routes/anthropic.rs b/src/server/routes/anthropic.rs index eb568c9eb..5e2235302 100644 --- a/src/server/routes/anthropic.rs +++ b/src/server/routes/anthropic.rs @@ -96,6 +96,24 @@ pub async fn anthropic_messages( } let translated = anthropic_request_to_chat(&request); + // Refuse media parts the loaded checkpoint cannot consume, with b10621's + // own ` input is not supported` wording, before any referenced URL or + // file is read (issue #1451). Rendered in the Anthropic error envelope this + // route uses, so a client parsing it does not have to special-case one + // rejection. + if let Some(rejection) = crate::server::media_capability_rejection( + &translated.chat_request, + state.media_support, + state.display_model_id(), + ) { + return AnthropicErrorResponse::new( + StatusCode::NOT_IMPLEMENTED, + rejection.error.message, + "not_supported_error", + ) + .into_response(); + } + // Reject requests with no effective input before any model dispatch // (issue #805, extending the #773 guard to the Anthropic Messages // route). Runs on the translated `ChatCompletionRequest` so a non-empty diff --git a/src/server/routes/chat.rs b/src/server/routes/chat.rs index 8e54271b7..08a705403 100644 --- a/src/server/routes/chat.rs +++ b/src/server/routes/chat.rs @@ -274,20 +274,19 @@ pub async fn chat_completions( return ErrorResponse::new(message, "invalid_request_error").into_response(); } - // reject `video_url` content blocks early for models that do - // not support video. Detected once at startup from `config.json` and - // cached on `AppState.media_support`. Returning a 400 keeps the client - // contract honest — silently dropping video frames would still consume - // tokens and produce a confusing reply. - if !state.media_support.video && request_has_video_blocks(&request) { - return ErrorResponse::new( - format!( - "video_url content blocks are not supported by model '{}'", - state.display_model_id() - ), - "invalid_request_error", - ) - .into_response(); + // Reject image, audio and video content blocks the loaded checkpoint + // cannot consume, before any referenced URL or file is read. Capability is + // detected once at startup from `config.json` and cached on + // `AppState.media_support`; the refusal carries b10621's own + // ` input is not supported` wording (issue #1451). Silently dropping + // an image would still consume tokens and produce a reply the caller could + // not tell apart from one that saw the picture. + if let Some(rejection) = crate::server::media_capability_rejection( + &request, + state.media_support, + state.display_model_id(), + ) { + return rejection.into_response(); } // Keep tool validation shared with the disaggregated router front so both @@ -1233,6 +1232,10 @@ async fn stream_chat_completion( /// content part anywhere in `messages`. The check matches before the heavier /// `extract_chat_video_paths` resolution step so non-video-capable models /// can refuse the request without paying canonicalisation / disk-I/O cost. +/// Retained as the narrow `video_url`-only predicate the modality gate +/// subsumes; the shared `media_capability_rejection` covers image, audio and +/// video together (issue #1451). +#[cfg_attr(not(test), allow(dead_code))] fn request_has_video_blocks(request: &ChatCompletionRequest) -> bool { !request.video_urls().is_empty() } diff --git a/src/server/routes/responses.rs b/src/server/routes/responses.rs index 0212bfbf8..928d0505f 100644 --- a/src/server/routes/responses.rs +++ b/src/server/routes/responses.rs @@ -90,6 +90,17 @@ pub async fn create_response( Err(err) => return translate_error_to_response(err).into_response(), }; + // Refuse media parts the loaded checkpoint cannot consume, with b10621's + // own ` input is not supported` wording, before any referenced URL or + // file is read (issue #1451). + if let Some(rejection) = crate::server::media_capability_rejection( + &translated.chat_request, + state.media_support, + state.display_model_id(), + ) { + return rejection.into_response(); + } + // Reject requests with no effective input before any model dispatch // (issue #773), mirroring the chat-completions check. Runs on the // translated `ChatCompletionRequest` so history pulled in from diff --git a/src/server/startup.rs b/src/server/startup.rs index ffc1ee662..d2e0e96f4 100644 --- a/src/server/startup.rs +++ b/src/server/startup.rs @@ -820,6 +820,13 @@ fn warn_on_insecure_video_allowlist() { fn detect_model_media_support(model_path: &Path) -> ModelMediaSupport { use crate::models::ModelType; + // b10621's `--no-mmproj` / `--no-mmproj-auto` refuses every media part, + // whatever the checkpoint could do (issue #1451). + if crate::server::media_admission_disabled() { + tracing::info!("--no-mmproj: refusing image, audio and video content blocks"); + return ModelMediaSupport::none(); + } + let model_type = match crate::models::get_model_type(model_path) { Ok(t) => t, Err(err) => { @@ -849,7 +856,20 @@ fn detect_model_media_support(model_path: &Path) -> ModelMediaSupport { ); } - ModelMediaSupport { video } + // Image and audio parts need a checkpoint with a vision/audio tower at all. + // `is_vlm_model_type` is the same config.json-only predicate the model + // registry uses, so a family that lands as `ModelKind::Vlm` is admitted + // with no list here to update (issue #1451). A multimodal checkpoint whose + // towers cannot take audio still refuses on the worker, where the loaded + // family is known; this boundary only separates "could in principle" from + // "text-only". + let multimodal = crate::model_metadata::is_vlm_model_type(model_type); + + ModelMediaSupport { + image: multimodal, + audio: multimodal, + video, + } } fn is_muse_glimmer_model_path(model_path: &Path) -> bool { diff --git a/src/server/state.rs b/src/server/state.rs index c2f596ed5..5a9347890 100644 --- a/src/server/state.rs +++ b/src/server/state.rs @@ -359,12 +359,44 @@ impl PromptCacheMetrics for BatchMetricsCacheAdapter { /// process, not what any individual request asks for. #[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] pub struct ModelMediaSupport { + /// `true` when the loaded checkpoint is a vision-language model, so + /// `image_url` content blocks can reach a vision tower. + /// + /// Before issue #1451 a text-only checkpoint accepted an image part at the + /// HTTP boundary, dropped it in `prepare_request_vlm_embeddings`, and + /// answered from the text alone: a caller could not tell an ignored image + /// from a described one. It is now refused with b10621's own + /// `image input is not supported` wording. + pub image: bool, + /// `true` when the loaded checkpoint can consume `input_audio` content + /// blocks. A checkpoint that is not multimodal at all refuses them here; + /// a multimodal checkpoint without an audio tower still refuses later, on + /// the worker, where the loaded family is known. + pub audio: bool, /// `true` when the loaded model supports `video_url` content blocks. /// Currently this is exactly the Gemma 4 VLM family; expand the /// detection logic alongside any new video-capable model. pub video: bool, } +impl ModelMediaSupport { + /// The all-refusing set, used when the operator passed b10621's + /// `--no-mmproj` / `--no-mmproj-auto`. + /// + /// Upstream's `--no-mmproj` leaves the projector unloaded, and every media + /// part in a request is then refused. mlxcel cannot leave the projector + /// unloaded (it is inside the checkpoint) but declines the same requests, + /// which is the observable half of the flag. + #[must_use] + pub const fn none() -> Self { + Self { + image: false, + audio: false, + video: false, + } + } +} + /// Shared application state passed into route handlers. #[derive(Clone)] pub struct AppState { diff --git a/src/vision/image_token_overrides.rs b/src/vision/image_token_overrides.rs new file mode 100644 index 000000000..062a7749d --- /dev/null +++ b/src/vision/image_token_overrides.rs @@ -0,0 +1,270 @@ +// Copyright 2025-2026 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Operator-supplied image-token budgets, applied to a dynamic-resolution +//! vision processor's own pixel bounds before an image is resized. +//! +//! This is the mlxcel side of llama-server b10621's `--image-min-tokens` and +//! `--image-max-tokens` (issue #1451). Upstream stores them on +//! `clip_hparams::custom_image_min_tokens` / `custom_image_max_tokens` and +//! converts them into pixel bounds in +//! [`set_limit_image_tokens`](https://github.com/ggml-org/llama.cpp/blob/c1d0e7a004015f23bc0233470b747b596f29b264/tools/mtmd/clip-model.h): +//! +//! ```text +//! patch_area = patch_size^2 * n_merge^2 +//! image_min_pixels = image_min_tokens * patch_area +//! image_max_pixels = image_max_tokens * patch_area +//! ``` +//! +//! mlxcel's dynamic-resolution processors express exactly the same two bounds +//! as `min_pixels` / `max_pixels`, so the translation is the identity: the same +//! multiplication against the same patch area. That is why these two options +//! are honored rather than refused. `--mtmd-batch-max-tokens`, which names an +//! encode *batch* width rather than a per-image budget, has no such counterpart +//! and is refused in [`crate::cli::multimodal_compat_args`]. +//! +//! # Why the applications counter exists +//! +//! Only the processors that resize dynamically have pixel bounds for a token +//! budget to move. A fixed-tile encoder (LLaVA-style square resize, SigLIP at a +//! fixed `image_size`, the SAM/CLIP channel-concat towers) crops or pads to a +//! geometry its weights were trained at, and emits the same token count for +//! every input; a minimum or maximum token budget has nothing to act on there. +//! Accepting the flag on such a checkpoint and encoding at the checkpoint's own +//! geometry is the silent-acceptance failure epic #1431 exists to remove. +//! +//! Rather than maintain a hand-written table of which architecture honors the +//! budget (which goes stale the moment a family is ported, the lesson +//! [`crate::models::rope_overrides`] already paid for), every processor that +//! consumes the override increments [`applications`]. Server startup compares +//! that count against zero after the model is loaded and refuses to serve when +//! a budget was requested and never applied. A new family that routes its +//! resize through a bound-consuming processor starts being accepted with no +//! list to update, and one that does not is named in the error. +//! +//! Used by: Qwen2-VL, Qwen2.5-VL, Qwen3-VL, Qwen3.5-VL, Qwen3-VL-MoE, +//! Qwen3-Omni, GLM-4V, GLM-4V-MoE, GLM-OCR, ColQwen2.5 (every family whose +//! preprocessing goes through [`crate::vision::processors::qwen2_vl`]). + +use std::sync::OnceLock; +use std::sync::atomic::{AtomicUsize, Ordering}; + +/// A requested per-image token budget, in b10621's units. +/// +/// At least one half is always `Some`: [`ImageTokenOverride::from_bounds`] +/// returns `None` when neither was requested, so an installed override always +/// means the operator asked for something. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct ImageTokenOverride { + min_tokens: Option, + max_tokens: Option, +} + +impl ImageTokenOverride { + /// Build an override from the two CLI halves, or `None` when neither was + /// requested. + #[must_use] + pub const fn from_bounds(min_tokens: Option, max_tokens: Option) -> Option { + if min_tokens.is_none() && max_tokens.is_none() { + return None; + } + Some(Self { + min_tokens, + max_tokens, + }) + } + + /// Render the request the way the operator wrote it, for diagnostics. + #[must_use] + pub fn describe(&self) -> String { + let mut parts = Vec::with_capacity(2); + if let Some(min) = self.min_tokens { + parts.push(format!("--image-min-tokens {min}")); + } + if let Some(max) = self.max_tokens { + parts.push(format!("--image-max-tokens {max}")); + } + parts.join(" ") + } + + /// Apply this override to a processor's declared pixel bounds. + /// + /// `patch_size` and `merge_size` are the processor's own geometry; the + /// product `(patch_size * merge_size)^2` is upstream's `patch_area`. A half + /// the operator did not request keeps the processor's declared bound, which + /// is what upstream's `custom_image_*_tokens > 0` guard does. + /// + /// A degenerate geometry (either factor zero, or an overflowing product) + /// leaves both declared bounds alone rather than producing a zero-pixel + /// budget that would resize every image to one patch. + #[must_use] + pub fn apply( + &self, + patch_size: usize, + merge_size: usize, + declared_min_pixels: usize, + declared_max_pixels: usize, + ) -> (usize, usize) { + let Some(patch_area) = patch_area(patch_size, merge_size) else { + return (declared_min_pixels, declared_max_pixels); + }; + let min_pixels = self + .min_tokens + .and_then(|tokens| (tokens as usize).checked_mul(patch_area)) + .unwrap_or(declared_min_pixels); + let max_pixels = self + .max_tokens + .and_then(|tokens| (tokens as usize).checked_mul(patch_area)) + .unwrap_or(declared_max_pixels); + (min_pixels, max_pixels) + } +} + +/// Upstream's `patch_area`: `patch_size^2 * n_merge^2`. +/// +/// `None` for a zero factor or an overflowing product. +#[must_use] +fn patch_area(patch_size: usize, merge_size: usize) -> Option { + let factor = patch_size.checked_mul(merge_size)?; + if factor == 0 { + return None; + } + factor.checked_mul(factor) +} + +/// The process-wide override, installed once before the first model load. +static INSTALLED: OnceLock> = OnceLock::new(); + +/// How many processors have consumed the installed override. +static APPLICATIONS: AtomicUsize = AtomicUsize::new(0); + +/// Install the process-wide override before any model is loaded. +/// +/// Installing the same value twice is accepted, so the server's model-switch +/// path (which re-enters startup) is not an error. +/// +/// # Errors +/// +/// Returns `Err` when an override was already installed with a different value, +/// which can only mean two startup paths raced or a caller installed after the +/// first load. +pub fn install(override_value: Option) -> Result<(), String> { + match INSTALLED.set(override_value) { + Ok(()) => Ok(()), + Err(_) if INSTALLED.get() == Some(&override_value) => Ok(()), + Err(_) => Err(format!( + "an image-token budget is already installed ({:?}); it must be set once, before the \ + first model load", + INSTALLED.get() + )), + } +} + +/// The installed override, if the process has one. +#[must_use] +pub fn installed() -> Option<&'static ImageTokenOverride> { + INSTALLED.get().and_then(|slot| slot.as_ref()) +} + +/// How many processors have consumed the installed override. +#[must_use] +pub fn applications() -> usize { + APPLICATIONS.load(Ordering::Relaxed) +} + +/// Record that a processor consumed the override. +pub(crate) fn note_application() { + APPLICATIONS.fetch_add(1, Ordering::Relaxed); +} + +/// Record that a processor able to consume the override was constructed. +/// +/// [`resolve_pixel_bounds`] only runs when an image is actually preprocessed, +/// which is per request, long after startup has to decide whether the budget +/// reached anything. A processor that *has* the two bounds registers itself at +/// construction instead, so [`verify_applied`] can refuse to serve immediately +/// after the checkpoint loads rather than after the first image arrives. +pub(crate) fn note_dynamic_resolution_processor() { + if installed().is_some() { + note_application(); + } +} + +/// Resolve the pixel bounds a processor should resize against. +/// +/// With no override installed this returns the declared bounds and counts +/// nothing, so the hot path for every ordinary preprocess is one relaxed atomic +/// load. +#[must_use] +pub(crate) fn resolve_pixel_bounds( + patch_size: usize, + merge_size: usize, + declared_min_pixels: usize, + declared_max_pixels: usize, +) -> (usize, usize) { + let Some(over) = installed() else { + return (declared_min_pixels, declared_max_pixels); + }; + note_application(); + over.apply( + patch_size, + merge_size, + declared_min_pixels, + declared_max_pixels, + ) +} + +/// The diagnostic returned when a requested budget reached no processor. +/// +/// Split out so the message can be asserted without installing a process-wide +/// override in a unit test. +#[must_use] +pub fn unapplied_diagnostic(model_label: &str, over: &ImageTokenOverride) -> String { + format!( + "{model_label}: {} was accepted on the command line but reached no image preprocessing \ + path, so images would be encoded at the geometry the checkpoint's own preprocessor \ + config declares. b10621 applies an image-token budget only to vision models with \ + dynamic resolution, and this checkpoint's preprocessor has no per-image pixel bound for \ + it to move: it resizes to a fixed geometry and emits the same token count for every \ + input. Drop the flag to serve this checkpoint at its own image resolution.", + over.describe() + ) +} + +/// Confirm that the budget an operator asked for actually reached the model. +/// +/// Called once, on the worker thread, immediately after the checkpoint loads +/// and a first image would be preprocessed. Three outcomes: +/// +/// - No override installed: `Ok(())`, and nothing was ever consulted. +/// - Override installed and a processor applied it: `Ok(())`. +/// - Override installed and no processor saw it: `Err`, naming the checkpoint +/// and what was asked for. +/// +/// # Errors +/// +/// Returns the diagnostic above when an installed budget reached no processor. +pub fn verify_applied(model_label: &str) -> Result<(), String> { + let Some(over) = installed() else { + return Ok(()); + }; + if applications() == 0 { + return Err(unapplied_diagnostic(model_label, over)); + } + Ok(()) +} + +#[cfg(test)] +#[path = "image_token_overrides_tests.rs"] +mod tests; diff --git a/src/vision/image_token_overrides_tests.rs b/src/vision/image_token_overrides_tests.rs new file mode 100644 index 000000000..250b040fb --- /dev/null +++ b/src/vision/image_token_overrides_tests.rs @@ -0,0 +1,121 @@ +// Copyright 2025-2026 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Arithmetic and diagnostic tests for the b10621 image-token budget +//! (issue #1451). +//! +//! The process-wide `INSTALL`ed override is deliberately never set here: a +//! `OnceLock` set by one test would leak into every other test in the same +//! binary. Everything these tests need is reachable through the explicit-value +//! entry points, which is why [`ImageTokenOverride::apply`] and +//! [`unapplied_diagnostic`] take their inputs rather than reading the global. + +use super::*; + +/// Qwen2-VL geometry: 14-pixel patches merged 2x2, so `patch_area == 784`. +const PATCH: usize = 14; +const MERGE: usize = 2; +const PATCH_AREA: usize = PATCH * PATCH * MERGE * MERGE; + +#[test] +fn patch_area_matches_upstreams_definition() { + assert_eq!(PATCH_AREA, 784); + assert_eq!(patch_area(PATCH, MERGE), Some(784)); +} + +#[test] +fn both_halves_convert_tokens_to_pixels() { + let over = ImageTokenOverride::from_bounds(Some(8), Some(1024)).expect("an override"); + let (min, max) = over.apply(PATCH, MERGE, 4 * 784, 16_384 * 784); + assert_eq!(min, 8 * PATCH_AREA); + assert_eq!(max, 1024 * PATCH_AREA); +} + +#[test] +fn a_half_that_was_not_requested_keeps_the_checkpoints_own_bound() { + let declared_min = 4 * PATCH_AREA; + let declared_max = 16_384 * PATCH_AREA; + + let min_only = ImageTokenOverride::from_bounds(Some(32), None).expect("an override"); + assert_eq!( + min_only.apply(PATCH, MERGE, declared_min, declared_max), + (32 * PATCH_AREA, declared_max) + ); + + let max_only = ImageTokenOverride::from_bounds(None, Some(256)).expect("an override"); + assert_eq!( + max_only.apply(PATCH, MERGE, declared_min, declared_max), + (declared_min, 256 * PATCH_AREA) + ); +} + +#[test] +fn neither_half_means_no_override_at_all() { + assert!(ImageTokenOverride::from_bounds(None, None).is_none()); +} + +#[test] +fn a_degenerate_geometry_leaves_the_declared_bounds_alone() { + // A zero patch size would make `patch_area` zero and collapse every image + // to a single patch; returning the declared bounds is the safe reading. + let over = ImageTokenOverride::from_bounds(Some(8), Some(64)).expect("an override"); + assert_eq!(over.apply(0, MERGE, 111, 222), (111, 222)); + assert_eq!(over.apply(PATCH, 0, 111, 222), (111, 222)); + assert_eq!(over.apply(usize::MAX, 2, 111, 222), (111, 222)); +} + +#[test] +fn an_overflowing_token_count_falls_back_to_the_declared_bound() { + let over = ImageTokenOverride::from_bounds(Some(u32::MAX), None).expect("an override"); + let (min, _) = over.apply(usize::MAX / 3, 1, 999, 1000); + assert_eq!(min, 999); +} + +#[test] +fn describe_renders_what_the_operator_typed() { + let both = ImageTokenOverride::from_bounds(Some(8), Some(64)).expect("an override"); + assert_eq!( + both.describe(), + "--image-min-tokens 8 --image-max-tokens 64" + ); + let min_only = ImageTokenOverride::from_bounds(Some(8), None).expect("an override"); + assert_eq!(min_only.describe(), "--image-min-tokens 8"); +} + +#[test] +fn the_unapplied_diagnostic_names_the_checkpoint_and_the_flag() { + let over = ImageTokenOverride::from_bounds(None, Some(256)).expect("an override"); + let message = unapplied_diagnostic("llava-1.5-7b-4bit", &over); + assert!(message.contains("llava-1.5-7b-4bit"), "{message}"); + assert!(message.contains("--image-max-tokens 256"), "{message}"); + // The operator has to be told what to do, not only that it failed. + assert!(message.contains("Drop the flag"), "{message}"); + assert!(message.contains("dynamic resolution"), "{message}"); +} + +#[test] +fn resolve_pixel_bounds_is_a_pass_through_without_an_installed_override() { + // Nothing installs an override in this binary, so this exercises the hot + // path every ordinary preprocess takes. + assert!(installed().is_none()); + assert_eq!(resolve_pixel_bounds(PATCH, MERGE, 7, 9), (7, 9)); +} + +#[test] +fn installing_the_same_value_twice_is_accepted() { + // `install(None)` is what an ordinary startup does, and the model-switch + // path re-enters it; both must succeed. + assert!(install(None).is_ok()); + assert!(install(None).is_ok()); +} diff --git a/src/vision/mod.rs b/src/vision/mod.rs index b994a4a81..a330b5c76 100644 --- a/src/vision/mod.rs +++ b/src/vision/mod.rs @@ -32,6 +32,7 @@ pub mod connectors; pub mod detection; pub mod encoders; pub mod feature_cache; +pub mod image_token_overrides; pub mod merge; pub mod processors; diff --git a/src/vision/processors/qwen2_vl.rs b/src/vision/processors/qwen2_vl.rs index ca0a9683e..e4708bd0a 100644 --- a/src/vision/processors/qwen2_vl.rs +++ b/src/vision/processors/qwen2_vl.rs @@ -73,6 +73,7 @@ pub fn max_image_tokens( impl Qwen2VLProcessor { /// Used by: Qwen2-VL, Qwen2.5-VL (CLIP normalization) pub fn new(patch_size: usize, temporal_patch_size: usize, spatial_merge_size: usize) -> Self { + crate::vision::image_token_overrides::note_dynamic_resolution_processor(); Self { patch_size, temporal_patch_size, @@ -92,6 +93,7 @@ impl Qwen2VLProcessor { mean: [f32; 3], std: [f32; 3], ) -> Self { + crate::vision::image_token_overrides::note_dynamic_resolution_processor(); Self { patch_size, temporal_patch_size, @@ -103,10 +105,32 @@ impl Qwen2VLProcessor { } } + /// The pixel bounds this processor resizes against. + /// + /// Normally the checkpoint's own `min_pixels` / `max_pixels`. When an + /// operator passed b10621's `--image-min-tokens` / `--image-max-tokens`, + /// [`crate::vision::image_token_overrides`] converts the token budget into + /// the same two bounds with upstream's own `tokens * patch_size^2 * + /// merge^2` arithmetic and returns those instead, counting the application + /// so startup can tell an ignored budget from an applied one. + /// + /// Used by: Qwen2-VL, Qwen2.5-VL, Qwen3-VL, Qwen3.5-VL, Qwen3-VL-MoE, + /// Qwen3-Omni, GLM-4V, GLM-4V-MoE, GLM-OCR, ColQwen2.5. + #[must_use] + pub(crate) fn effective_pixel_bounds(&self) -> (usize, usize) { + crate::vision::image_token_overrides::resolve_pixel_bounds( + self.patch_size, + self.spatial_merge_size, + self.min_pixels, + self.max_pixels, + ) + } + /// Compute target size that satisfies constraints /// Returns (height, width) padded to multiples of factor pub(crate) fn smart_resize(&self, orig_h: u32, orig_w: u32) -> (u32, u32) { let factor = (self.patch_size * self.spatial_merge_size) as u32; // 28 + let (min_pixels, max_pixels) = self.effective_pixel_bounds(); // Start with original size, round to factor let mut h = ((orig_h as f64 / factor as f64).round() as u32).max(1) * factor; @@ -114,20 +138,20 @@ impl Qwen2VLProcessor { // Ensure within pixel limits let pixels = (h * w) as usize; - if pixels > self.max_pixels { - let scale = (self.max_pixels as f64 / pixels as f64).sqrt(); + if pixels > max_pixels { + let scale = (max_pixels as f64 / pixels as f64).sqrt(); h = ((h as f64 * scale / factor as f64).round() as u32).max(1) * factor; w = ((w as f64 * scale / factor as f64).round() as u32).max(1) * factor; } - if (h * w) as usize > self.max_pixels { + if (h * w) as usize > max_pixels { // Further reduce if needed - let scale = (self.max_pixels as f64 / (h * w) as f64).sqrt(); + let scale = (max_pixels as f64 / (h * w) as f64).sqrt(); h = ((h as f64 * scale / factor as f64).floor() as u32).max(1) * factor; w = ((w as f64 * scale / factor as f64).floor() as u32).max(1) * factor; } let pixels = (h * w) as usize; - if pixels < self.min_pixels { - let scale = (self.min_pixels as f64 / pixels as f64).sqrt(); + if pixels < min_pixels { + let scale = (min_pixels as f64 / pixels as f64).sqrt(); h = ((h as f64 * scale / factor as f64).ceil() as u32).max(1) * factor; w = ((w as f64 * scale / factor as f64).ceil() as u32).max(1) * factor; }