-
Notifications
You must be signed in to change notification settings - Fork 1
fix(config): harden integer callback boundaries #873
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
9ea1b6c
test(config): expose integer callback trust gap
seonghobae 67ecdf3
fix(config): reject caller-controlled integer coercion
seonghobae a005092
test(config): cover bounded integer trust controls
seonghobae 3ea7c09
docs(changelog): record config integer boundary hardening
seonghobae 2faa822
docs(doctoring): record config integer trust boundary
seonghobae 805df46
fix(config): validate simulation and fit controls at construction
cursoragent File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| # Harden configuration integer trust boundaries | ||
|
|
||
| ## Fixed | ||
|
|
||
| - Reject caller-defined integer subclasses and arbitrary `__index__` providers before public simulation and fit configuration validation can dispatch caller-controlled coercion. | ||
| - Preserve exact built-in integers and genuine NumPy integer scalars while validating simulation size, optimizer-work, quadrature, and latent-integration controls through built-in integer values. | ||
| - Run the same simulation and fit validators at construction so memory-safety bounds cannot be bypassed by skipping an explicit `validate()` call. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| # Configuration integer callback safety | ||
|
|
||
| ## Problem | ||
|
|
||
| Public configuration validation accepted Python's generic integer protocol. Calling `operator.index()` or comparing caller-controlled integer-like objects before trust was established allowed arbitrary `__index__` implementations or integer subclasses to participate in validation. | ||
|
|
||
| ## Boundary decision | ||
|
|
||
| Configuration validation is marshalling and trust-boundary work, not psychometric arithmetic. The package now accepts only exact built-in `int` values and exact supported NumPy integer scalar types for validated integer controls. Accepted NumPy scalars are converted to built-in integers for bounds and work-budget calculations; booleans, caller-defined `int` subclasses, and arbitrary index providers are rejected without invoking their coercion hooks. | ||
|
|
||
| `MLS2PLMConfig` and `FitConfig` run that same validator from `__post_init__`, so invalid or untrusted controls cannot exist as constructed objects. `validate()` remains public and idempotent for callers that already invoke it at simulate/fit entry points. | ||
|
|
||
| The hardened surface covers simulation sizes and latent dimension plus fit latent dimension, optimizer iteration/restart/history controls, quadrature node counts, marginal M-step count, and latent-space integration point/seed controls. Numerical model ownership and Rust-first computation are unchanged. | ||
|
|
||
| ## Test evidence | ||
|
|
||
| `tests/test_config_integer_callback_safety.py` provides hostile `__index__` regressions, valid-valued caller `int` subclasses, and genuine NumPy scalar compatibility. The original RED commit is `4c81e4dc465312d13f044b9b47e14d839af6cc1a`; exact-head hosted CI/security/package/coverage/review evidence remains authoritative as the branch advances. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,10 @@ | ||
| from __future__ import annotations | ||
|
|
||
| import math | ||
| import operator | ||
| from dataclasses import dataclass | ||
|
|
||
| import numpy as np | ||
|
|
||
| from .backend import normalize_backend, normalize_device | ||
|
|
||
|
|
||
|
|
@@ -37,6 +38,31 @@ | |
| MAX_SIM_ITEMS_PER_DIM = 10_000 | ||
| MAX_SIM_CELLS = 200_000_000 | ||
|
|
||
| _NUMPY_INTEGER_SCALAR_TYPES = ( | ||
| np.int8, | ||
| np.int16, | ||
| np.int32, | ||
| np.int64, | ||
| np.intp, | ||
| np.longlong, | ||
| np.uint8, | ||
| np.uint16, | ||
| np.uint32, | ||
| np.uint64, | ||
| np.uintp, | ||
| np.ulonglong, | ||
| ) | ||
|
|
||
|
|
||
| def _trusted_integer(value: object, name: str) -> int: | ||
| """Return one package-trusted integer without caller-controlled coercion.""" | ||
| value_type = type(value) | ||
| if value_type is int: | ||
| return value | ||
| if any(value_type is trusted_type for trusted_type in _NUMPY_INTEGER_SCALAR_TYPES): | ||
| return int(value) | ||
| raise ValueError(f"{name} must be an integer") | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class MLS2PLMConfig: | ||
|
|
@@ -58,6 +84,10 @@ class MLS2PLMConfig: | |
| seed: int = 1 | ||
| dtype: str = "float64" | ||
|
|
||
| def __post_init__(self) -> None: | ||
| """Reject invalid simulation controls at construction.""" | ||
| self.validate() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| @property | ||
| def n_items(self) -> int: | ||
| """Total item count (``n_dims * items_per_dim``).""" | ||
|
|
@@ -70,40 +100,35 @@ def validate(self) -> None: | |
| a positive-definite trait equicorrelation from ``phi``, a finite | ||
| non-negative ``gamma``, and a supported ``dtype``. | ||
| """ | ||
| for name, value in ( | ||
| ("n_persons", self.n_persons), | ||
| ("n_dims", self.n_dims), | ||
| ("items_per_dim", self.items_per_dim), | ||
| ("latent_dim", self.latent_dim), | ||
| ): | ||
| if isinstance(value, bool): | ||
| raise ValueError(f"{name} must be an integer") | ||
| try: | ||
| operator.index(value) | ||
| except TypeError as exc: | ||
| raise ValueError(f"{name} must be an integer") from exc | ||
| if self.n_persons < 1: | ||
| n_persons = _trusted_integer(self.n_persons, "n_persons") | ||
| n_dims = _trusted_integer(self.n_dims, "n_dims") | ||
| items_per_dim = _trusted_integer(self.items_per_dim, "items_per_dim") | ||
| latent_dim = _trusted_integer(self.latent_dim, "latent_dim") | ||
|
|
||
| if n_persons < 1: | ||
| raise ValueError("n_persons must be >= 1") | ||
| if self.n_dims < 1: | ||
| if n_dims < 1: | ||
| raise ValueError("n_dims must be >= 1") | ||
| if self.items_per_dim < 1: | ||
| if items_per_dim < 1: | ||
| raise ValueError("items_per_dim must be >= 1") | ||
| if self.n_persons > MAX_SIM_PERSONS: | ||
| if n_persons > MAX_SIM_PERSONS: | ||
| raise ValueError(f"n_persons must be <= {MAX_SIM_PERSONS}") | ||
| if self.n_dims > MAX_SIM_DIMS: | ||
| if n_dims > MAX_SIM_DIMS: | ||
| raise ValueError(f"n_dims must be <= {MAX_SIM_DIMS}") | ||
| if self.items_per_dim > MAX_SIM_ITEMS_PER_DIM: | ||
| if items_per_dim > MAX_SIM_ITEMS_PER_DIM: | ||
| raise ValueError(f"items_per_dim must be <= {MAX_SIM_ITEMS_PER_DIM}") | ||
| if self.n_persons * self.n_items > MAX_SIM_CELLS: | ||
| n_items = n_dims * items_per_dim | ||
| simulation_cells = n_persons * n_items | ||
| if simulation_cells > MAX_SIM_CELLS: | ||
| raise ValueError( | ||
| f"n_persons x n_items ({self.n_persons * self.n_items}) exceeds the " | ||
| f"n_persons x n_items ({simulation_cells}) exceeds the " | ||
| f"{MAX_SIM_CELLS}-cell simulation budget" | ||
| ) | ||
| if self.latent_dim < 1: | ||
| if latent_dim < 1: | ||
| raise ValueError("latent_dim must be >= 1") | ||
| if self.latent_dim > MAX_LATENT_DIM: | ||
| if latent_dim > MAX_LATENT_DIM: | ||
| raise ValueError(f"latent_dim must be <= {MAX_LATENT_DIM}") | ||
| if not (-1.0 / max(self.n_dims - 1, 1) < self.phi < 1.0): | ||
| if not (-1.0 / max(n_dims - 1, 1) < self.phi < 1.0): | ||
| raise ValueError("phi must produce a positive-definite equicorrelation matrix") | ||
| try: | ||
| gamma_is_finite = math.isfinite(self.gamma) | ||
|
|
@@ -193,6 +218,10 @@ class FitConfig: | |
| # cf. the ZI count-model guidance of Perumean-Chaney et al. (2013). | ||
| zero_inflation: bool = False | ||
|
|
||
| def __post_init__(self) -> None: | ||
| """Reject invalid fit controls at construction.""" | ||
| self.validate() | ||
|
|
||
| def normalized_model(self) -> str: | ||
| """Return the model name upper-cased for case-insensitive matching.""" | ||
| return self.model.upper() | ||
|
|
@@ -209,31 +238,31 @@ def validate(self) -> None: | |
| model = self.normalized_model() | ||
| if model not in VALID_MODELS: | ||
| raise ValueError(f"model must be one of {sorted(VALID_MODELS)}") | ||
| if not (1 <= self.latent_dim <= MAX_LATENT_DIM): | ||
| latent_dim = _trusted_integer(self.latent_dim, "latent_dim") | ||
| if not (1 <= latent_dim <= MAX_LATENT_DIM): | ||
| raise ValueError(f"latent_dim must be >= 1 and <= {MAX_LATENT_DIM}") | ||
| if self.optimizer not in VALID_OPTIMIZERS: | ||
| raise ValueError(f"optimizer must be one of {sorted(VALID_OPTIMIZERS)}") | ||
| if self.estimator not in VALID_ESTIMATORS: | ||
| raise ValueError(f"estimator must be one of {sorted(VALID_ESTIMATORS)}") | ||
| if model == "BIFAC2PLM" and self.estimator == "jmle": | ||
| raise ValueError("BIFAC2PLM requires estimator 'mmle'") | ||
| if isinstance(self.lbfgs_history, bool): | ||
| raise ValueError("lbfgs_history must be an integer") | ||
| try: | ||
| lbfgs_history = operator.index(self.lbfgs_history) | ||
| except TypeError as exc: | ||
| raise ValueError("lbfgs_history must be an integer") from exc | ||
|
|
||
| lbfgs_history = _trusted_integer(self.lbfgs_history, "lbfgs_history") | ||
| if not (1 <= lbfgs_history <= MAX_LBFGS_HISTORY): | ||
| raise ValueError( | ||
| f"lbfgs_history must be >= 1 and <= {MAX_LBFGS_HISTORY}" | ||
| ) | ||
| if not (1 <= self.max_iter <= MAX_MAX_ITER): | ||
| max_iter = _trusted_integer(self.max_iter, "max_iter") | ||
| if not (1 <= max_iter <= MAX_MAX_ITER): | ||
| raise ValueError(f"max_iter must be >= 1 and <= {MAX_MAX_ITER}") | ||
| if not (1 <= self.n_restarts <= MAX_RESTARTS): | ||
| n_restarts = _trusted_integer(self.n_restarts, "n_restarts") | ||
| if not (1 <= n_restarts <= MAX_RESTARTS): | ||
| raise ValueError(f"n_restarts must be >= 1 and <= {MAX_RESTARTS}") | ||
| if self.max_iter * self.n_restarts > MAX_AGGREGATE_ITERS: | ||
| aggregate_iters = max_iter * n_restarts | ||
| if aggregate_iters > MAX_AGGREGATE_ITERS: | ||
| raise ValueError( | ||
| f"max_iter x n_restarts ({self.max_iter * self.n_restarts}) exceeds the " | ||
| f"max_iter x n_restarts ({aggregate_iters}) exceeds the " | ||
| f"aggregate optimizer-work budget {MAX_AGGREGATE_ITERS}" | ||
| ) | ||
| # non-finite floats (NaN/Inf) slip past bare `<= 0` comparisons | ||
|
|
@@ -249,24 +278,26 @@ def validate(self) -> None: | |
| not math.isfinite(self.gradient_clip) or self.gradient_clip <= 0 | ||
| ): | ||
| raise ValueError("gradient_clip must be > 0 and finite, or None") | ||
|
|
||
| supported_q = {7, 11, 15, 21, 31, 41} | ||
| for name in ("q_theta", "q_xi", "q_u"): | ||
| if getattr(self, name) not in supported_q: | ||
| quadrature_nodes = _trusted_integer(getattr(self, name), name) | ||
| if quadrature_nodes not in supported_q: | ||
| raise ValueError(f"{name} must be one of {sorted(supported_q)}") | ||
| if not (1 <= self.m_steps <= MAX_M_STEPS): | ||
| m_steps = _trusted_integer(self.m_steps, "m_steps") | ||
| if not (1 <= m_steps <= MAX_M_STEPS): | ||
| raise ValueError(f"m_steps must be >= 1 and <= {MAX_M_STEPS}") | ||
| if self.xi_rule.lower() not in {"gh", "qmc", "halton", "mc", "montecarlo", "monte-carlo"}: | ||
| if self.xi_rule.lower() not in { | ||
| "gh", | ||
| "qmc", | ||
| "halton", | ||
| "mc", | ||
| "montecarlo", | ||
| "monte-carlo", | ||
| }: | ||
| raise ValueError("xi_rule must be one of ['gh', 'qmc', 'mc']") | ||
| for name in ("xi_points", "xi_seed"): | ||
| value = getattr(self, name) | ||
| if isinstance(value, bool): | ||
| raise ValueError(f"{name} must be an integer") | ||
| try: | ||
| operator.index(value) | ||
| except TypeError as exc: | ||
| raise ValueError(f"{name} must be an integer") from exc | ||
| xi_points = operator.index(self.xi_points) | ||
| xi_seed = operator.index(self.xi_seed) | ||
| xi_points = _trusted_integer(self.xi_points, "xi_points") | ||
| xi_seed = _trusted_integer(self.xi_seed, "xi_seed") | ||
| if not (1 <= xi_points <= MAX_XI_POINTS): | ||
| raise ValueError(f"xi_points must be >= 1 and <= {MAX_XI_POINTS}") | ||
| if not (0 <= xi_seed <= (1 << 64) - 1): | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int(value)is only used for local bounds.n_itemsstill multiplies the stored fields, sonp.uint8(16) * np.uint8(16)wraps to0after a successful validate. Write the trusted built-in ints back beforesimulate()readsn_items.