[pull] master from tensorflow:master - #8784
Merged
Merged
Conversation
…qw72)
ValidateOpDef()/ValidateArg() in op_def_util.cc validated the op's own
name against IsValidOpName but never validated the character set of
attribute names (OpDef.AttrDef.name()) or argument names
(OpDef.ArgDef.name()) -- only their uniqueness.
TensorFlow's op-wrapper code generators (cc_op_gen.cc, python_op_gen.cc)
splice these names directly into generated C++/Python source with no
escaping. A crafted attr/arg name containing ", ), ;, or a comma can
break out of its syntactic context and inject arbitrary code, executed
at compile time (C++) or import time (Python) of the generated wrapper.
This is the same class of bug already fixed in the sibling file
cc_op_fuzz_gen.cc (escaping via absl::CEscape).
This change:
1. Adds IsValidAttrOrArgName(), a safe-identifier check (must start
with a letter/underscore, contain only letters/digits/underscores),
and applies it in ValidateOpDef() and ValidateArg(). This closes
the injection class at the single validation choke-point, so it
protects every current and future downstream consumer -- not just
the two known generators.
2. Applies absl::CEscape() to graph_attr.name() at the cc_op_gen.cc
splice site as defense-in-depth, matching the precedent already
merged in cc_op_fuzz_gen.cc.
Fixes GHSA-2xp6-8g4h-qw72.
…rArgName Per gemini-code-assist review feedback on PR #124374: std::isalpha is locale-dependent and can behave unexpectedly depending on environment locale settings, and mixing it with Scanner's strictly-ASCII character classes was inconsistent. Replaces both with Abseil's locale-independent ASCII utility functions (absl::ascii_isalpha, absl::ascii_isalnum), which also simplifies the function and drops the Scanner dependency entirely. No behavioral change: verified against the same test vectors as before (malicious attr name from the advisory rejected; legitimate names like dtype, T, num_threads_2, _internal still accepted).
Per review feedback on this PR: 1. The PR description claimed defense-in-depth escaping was applied to both generators, but python_op_gen.cc was untouched. Investigating why revealed the real issue: both generators splice names into IDENTIFIER positions (C++ parameter names, Python keyword argument names) via ApiDef.Arg/Attr.rename_to() -- a field entirely separate from the OpDef.ArgDef/AttrDef.name() fields validated by ValidateOpDef/ValidateArg. CEscape (for string literals) doesn't apply to identifier positions at all, so 'add escaping to python_op_gen.cc' was the wrong frame -- rename_to() needed validation, not escaping, in both generators. 2. This also directly addresses the GetConstructorDecl() case flagged in review: op_info.arg_names[i] traces to AvoidCPPKeywords(api_def_arg.rename_to()) in cc_op_gen_util.cc, which was unvalidated. Same pattern for the attr_name construction a few lines below it. Fix: expose IsValidAttrOrArgName() via op_def_util.h (previously file-local to op_def_util.cc) and validate rename_to() at its use sites in cc_op_gen_util.cc and python_op_gen.cc's ParamNames constructor, falling back to the original, already-validated OpDef name when rename_to() isn't a safe identifier. rename_to is a display-name preference, not required data, so a safe fallback is more appropriate than a hard failure. 3. Adds the suggested regression tests to op_def_util_test.cc, covering invalid characters and invalid start characters for both attribute and argument names.
Signed-off-by: Cocoa <i@uwucocoa.moe>
signbit compared its argument with zero, so negative zero reported False even though NumPy reports True because the sign bit is set, and a negative NaN was also misreported. Bitcast float inputs to an integer type of the same size and compare that with zero instead.
Addresses dmiltr3's review: Google's internal presubmit on import failed because tensorflow/core/framework/BUILD's op_def_util cc_library target did not declare @com_google_absl//absl/strings in its deps, even though op_def_util.cc includes absl/strings/ascii.h (for absl::ascii_isalpha / absl::ascii_isalnum, used by this PR's own fix). Under strict layering checks this failed compilation for anything depending on op_def_util: error: module //third_party/tensorflow/core/framework:op_def_util does not directly depend on a module exporting 'third_party/absl/strings/ascii.h' ... Adds "@com_google_absl//absl/strings" to that target's deps, in the same place and form as the reviewer's own suggested diff. Verified: op_def_util.cc's use of absl/strings/ascii.h confirmed current before editing. Basic Starlark/Python-like syntax check on the whole BUILD file passes after the edit -- this sandbox has no Bazel available, so a real 'bazel build' could not be run here; please confirm CI passes.
Addresses review: the runtime validation added to ValidateArg/
ValidateOpDef broke internal presubmit across 1,400+ targets. Root
cause: several legitimate, already-registered ops (e.g.
TFLite_Detection_PostProcess, registered in
tensorflow/compiler/mlir/lite/python/tf_tfl_flatbuffer_helpers.cc) use
argument names containing '/' and ':' ("raw_outputs/box_encodings",
"TFLite_Detection_PostProcess:1") for reasons unrelated to code
generation. Rejecting those at OpDef registration/validation time --
which runs for every op in the runtime, not just ones that go through
code generation -- broke that legitimate, pre-existing usage.
The actual injection risk only exists where a name is spliced,
unescaped, as a raw identifier into generated C++/Python source by
cc_op_gen.cc / cc_op_gen_util.cc / python_op_gen.cc. Moves the
defense to exactly those splice sites instead of the runtime-wide
check:
1. Removes the IsValidAttrOrArgName VALIDATE() calls from ValidateArg
and ValidateOpDef, keeping IsValidAttrOrArgName itself as a utility
function for code generators.
2. Both cc_op_gen_util.cc and python_op_gen.cc already had a fallback
pattern for ApiDef rename_to() built on the assumption that the
ORIGINAL arg/attr name was "already validated" by the runtime check
removed in (1). That assumption no longer holds, so all three call
sites are upgraded to a three-tier fallback: rename_to if safe, else
the original name if THAT is safe, else a new SanitizeToIdentifier()
helper (added alongside IsValidAttrOrArgName in op_def_util.{h,cc})
that guarantees a syntactically safe identifier for any input by
replacing unsafe characters with underscores.
3. Replaces op_def_util_test.cc's InvalidAttrOrArgName test (which
asserted ValidateOpDef rejects non-identifier names) with direct
unit tests of IsValidAttrOrArgName and SanitizeToIdentifier as pure
functions, plus a positive regression test confirming ValidateOpDef
now accepts TFLite_Detection_PostProcess's exact real-world argument
names.
Verified with a standalone C++ harness (no Bazel available in this
environment) reproducing both functions exactly: 14 checks, including
confirming every SanitizeToIdentifier output itself re-passes
IsValidAttrOrArgName -- the invariant the whole three-tier fallback
depends on. All 14 pass. The exact GHSA-2xp6-8g4h-qw72 payload and
TFLite_Detection_PostProcess's real argument names are both correctly
rejected by IsValidAttrOrArgName and correctly sanitized into safe
identifiers by SanitizeToIdentifier.
Please confirm the real build and the 1,400+ previously-failing
presubmit targets pass in CI before merging -- this environment has no
Bazel to verify that directly.
…he C++ op generators
Addresses review round 3 (SensitiveApiFuzzingCheck): several C++
operator-wrapper code-generation paths in cc_op_gen_util.cc and
cc_op_gen.cc still concatenated raw, unsanitized ApiDef rename_to()
values directly into generated C++ source, even after the previous
round fixed python_op_gen.cc and two call sites in cc_op_gen_util.cc.
Adds a single shared SafeRenameTo(name, rename_to) helper to
cc_op_gen_util.{h,cc} (the shared C++ code-gen utility file both
cc_op_gen.cc and cc_op_gen_util.cc already include), matching the
review's exact suggested signature and three-tier fallback: rename_to
if it's a safe identifier, else the original name if THAT is safe,
else SanitizeToIdentifier(name). Refactors the two previously-fixed
call sites to use this shared helper too, replacing duplicated inline
ternary chains with one implementation.
Fixes every remaining unsanitized splice site found, beyond the
reviewer's own list:
- cc_op_gen_util.cc: output_names (mirrors the already-fixed input
case in the same constructor)
- cc_op_gen_util.cc's GetOpAttrStruct(): four separate splice points
for the same attribute (the setter method name, the field-access
expression, the static defaults function name, and the struct
field declaration) -- computed once per loop iteration so all four
agree on the same identifier
- cc_op_gen.cc: the static Attrs::<Name>(x) helper generator (a
near-duplicate of GetOpAttrStruct's setter-name generation)
- cc_op_gen.cc's GetConstructorBody(): the input-arg declaration
loop AND the separate .Input() reference loop, which reference the
same generated local variable by name from two different loops
over parallel ApiDef arg lists -- both fixed with the identical
sanitization call so the generated .Input(_name) reference always
matches its auto _name = ... declaration
- cc_op_gen.cc's .Attr() value-reference (both the attrs.<name>_
field-access form and the bare-identifier form)
Verified with standalone C++ harnesses (no Bazel in this environment):
SafeRenameTo itself passes 6 checks (normal case, unsafe-rename_to/
safe-name fallback, TFLite_Detection_PostProcess's real both-unsafe
case, the exact GHSA payload in both fields, empty rename_to, a
colon-suffixed output name), confirming every result is itself a
valid identifier. A second harness specifically confirms the
GetConstructorBody consistency invariant: the declaration loop and
the .Input() loop, called independently with the same (name,
rename_to) pair, always produce byte-identical output. Confirmed via
grep that zero unsanitized .rename_to() calls remain in either file --
every occurrence is now an argument to SafeRenameTo. Please confirm
the real build passes (specifically //tensorflow/cc:cc_ops or
equivalent) before merging.
tf.image.adjust_contrast could not be differentiated: the raw AdjustContrastv2 op had no Python gradient registration, so any tape through it raised "LookupError: gradient registry has no entry for: AdjustContrastv2". The kernel computes (images - mean) * factor + mean, where mean is taken per batch and channel over the last three dimensions, interpreted as [height, width, channels]. The new registration reduces the incoming gradient over those same axes and returns sum(grad * (images - mean)) for the scalar factor. The half and float paths form the factor reduction in float32 to avoid half-precision overflow. Filed as issue 126083; adjust_hue and adjust_saturation need piecewise HSV derivations and are left for separate changes. Test Plan: Added AdjustContrastOpTestBase with gradient_checker_v2 cases for rank 3, 4 and 5 inputs so the analytical gradient is checked against finite differences of the real forward kernel. Ran image_grad_test.py AdjustContrastOpTest against a pip tf-nightly build with the patched image_grad.py overlaid: "Ran 4 tests in 2.486s / OK (skipped=1)". With pristine image_grad.py the rank 3 and rank 4 cases fail with the LookupError above.
The unknown-rank branch subtracted 3 from the Python value None instead of the symbolic rank, which would raise TypeError whenever a graph placeholder of unknown rank reached it. Read the rank through array_ops.rank there, matching the intent of the branch. Caught in code review on pull request 126086. Test Plan: python -m py_compile tensorflow/python/ops/image_grad.py image_grad_test.py AdjustContrastOpTest against the nightly overlay: "Ran 4 tests in 1.393s / OK (skipped=1)".
The new AdjustContrastv2 registration changes the gradient exclusion tables that pywrap_gradient_exclusions.cc holds: the registration reads only the grad argument and none of op.inputs, so the op gains a full unused-inputs entry. Regenerated with the documented generator entry point; the output for unmodified master reproduces the committed file byte for byte, and the only delta here is the single new AdjustContrastv2 line.
`fabs` was defined as `return abs(x)`, so it inherited `absolute`'s dtype
behaviour and handed back an integer for an integer argument:
>>> tnp.fabs(np.array([1, -2, 3], dtype=np.int32)).dtype
tf.int32
>>> np.fabs(np.array([1, -2, 3], dtype=np.int32)).dtype
dtype('float64')
Returning a float is the whole difference between `fabs` and `absolute`,
and the docstring `np_doc('fabs')` generates points at numpy's, which
documents the result as always floating point.
Pass `promote_to_float=True` to `_scalar()`, the same way `ceil`, `floor`
and the other always-float unary ops in this file do. Floating point
arguments are unaffected, since `_scalar()` only casts a dtype that is
not already inexact, so `float16` and `float32` inputs keep their own
dtype exactly as numpy does.
The values are unchanged either way, only the dtype moves, so the new
test checks the dtype rather than just the numbers.
… accumulator APIs. This change adds `PyObject_TypeCheck` validation to several tape, variable watcher, and forward accumulator functions in `pywrap_tfe_src.cc` to prevent crashes when invalid Python objects are passed. It also fixes potential double-decref issues in `TFE_Py_VariableWatcherRemove` and `TFE_Py_ForwardAccumulatorSetRemove` by ensuring `Py_DECREF` is only called if the object was successfully erased from the tracking set. Finally, it updates the pybind11 wrappers to correctly propagate Python exceptions raised by these type checks and adds corresponding unit tests. PiperOrigin-RevId: 971552863
PiperOrigin-RevId: 971558059
PiperOrigin-RevId: 971577926
… are implemented generically in the base class. PiperOrigin-RevId: 971601833
… properties of a given slice PiperOrigin-RevId: 971614915
There are some parameter type mismatch issues. In the TfLiteConvParams struct , the stride and dilation fields are 32-bit integers, but in the ConvParams struct , these fields are declared as 16-bit integers. It is a disconnection between TFLite's public C API and the practical kernel implementations. There is no need to support 32-bit large strides/dilations, but the kernel implementations still need to check the parameter ranges for security reasons. PiperOrigin-RevId: 971703093
PiperOrigin-RevId: 971707065
…ion::GetDefaultLayout` PiperOrigin-RevId: 971711429
An upstream LLVM change added Emscripten support to config.bzl, shifting line numbers and adding `@platforms//os:emscripten` to the context of `backtrace_defines`. This caused patch application to fail during OSS builds. PiperOrigin-RevId: 971781235
PiperOrigin-RevId: 971787143
…ient PiperOrigin-RevId: 971787320
PiperOrigin-RevId: 971791347
PiperOrigin-RevId: 971793159
Upstream LLVM removed PointerUnion::get<T>() and is<T>(). This patches tf_runtime to use LLVM cast<T>() in basic_kernels.cc for OSS builds. PiperOrigin-RevId: 971845980
…attr-arg-name-validation PiperOrigin-RevId: 971856762
…zero PiperOrigin-RevId: 971858011
All targets that previously depended on this target now either depend on computation_placer or device_assignment targets. PiperOrigin-RevId: 971858217
Configure XLA workspace import macros to support `@cuda_tile` and `@tensor_ir` dependencies in open-source XLA builds. PiperOrigin-RevId: 971865012
Note: Alternative would be to add input channel padding as FP16 Tensor Core execution plans require channel to be 128bit aligned. PiperOrigin-RevId: 971890275
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
17 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )