Repository navigation
Conversation
This fixes b/510434185, "Improper Error Handling and Silent Data
Corruption via OP_REQUIRES_OK in Worker Lambdas". Some parsing
functions:(e.g., ParsePrograms, GetProgramsAndNumQubits, GetPauliSums)
used TensorFlow's OP_REQUIRES_OK macro inside worker lambdas dispatched
via ParallelFor. An error such as an unparseable protobuf could result
in OP_REQUIRES_OK emitting an error to the context but returning early
from the worker lambda. This would not stop other threads or signal the
outer function to stop execution. The outer function would return
Status::OK(), and downstream kernels would execute using uninitialized
or partially populated data (e.g., num_qubits remaining at -1), leading
to logic corruption, OOM, or crashes.
In more detail, the problem involved this bit of code in
`tensorflow_quantum/core/ops/tfq_ps_weights_from_symbols_op.cc`:
```cpp
OP_REQUIRES(context, false,
tensorflow::errors::InvalidArgument(
"A circuit contains a sympy.Symbol not found "
"in symbols!"));
```
The call of OP_REQUIRES from inside a worker thread caused 2 failures:
1. Thread-safety bug (data race): context is shared across the whole
operation and is not thread-safe. If two worker threads hit invalid
circuits at the same time, both threads would write to context's
internal error fields simultaneously without a lock.
2. Control-flow bug (`return;` exited the wrong function): Because
OP_REQUIRES uses `return;` to abort, calling it inside DoWork only
returned from that single worker's DoWork call, not from the outer
Compute method. Once the worker pool finished, Compute kept
executing even though the operation had already failed.
The change in this PR follows the recommended remediation in the
vulnerability report, namely to replace OP_REQUIRES_OK in lambdas with a
synchronized status tracker and ensure the outer function returns the
failure status.
There was a problem hiding this comment.
Code Review
This pull request improves thread safety and error handling during parallel execution by refactoring the 'NESTED_FN_STATUS_SYNC' macro to use a robust 'do-while' block and 'absl::MutexLock'. It updates several operations to use this macro for synchronizing thread-local statuses with a global status, and introduces comprehensive C++ and Python tests to verify correct behavior. The review feedback recommends renaming the macro-local variables in 'NESTED_FN_STATUS_SYNC' to highly unique names to prevent potential name collisions and shadowing issues.
| #define NESTED_FN_STATUS_SYNC(global_status, local_status, global_lock) \ | ||
| if (TF_PREDICT_FALSE(!local_status.ok())) { \ | ||
| global_lock.Lock(); \ | ||
| global_status = local_status; \ | ||
| global_lock.Unlock(); \ | ||
| return; \ | ||
| } | ||
| do { \ | ||
| const ::tensorflow::Status _s = (local_status); \ | ||
| if (TF_PREDICT_FALSE(!_s.ok())) { \ | ||
| ::absl::MutexLock _l(&(global_lock)); \ | ||
| (global_status) = _s; \ | ||
| return; \ | ||
| } \ | ||
| } while (0) |
There was a problem hiding this comment.
Using very short and common names like _s and _l for macro-local variables can easily lead to name collisions if the caller passes variables with the same names (e.g., _s as global_status or _l as global_lock). This would result in shadow variables, self-assignment, or compilation errors. To ensure macro hygiene and prevent correctness bugs, use highly unique names for macro-local variables.
| #define NESTED_FN_STATUS_SYNC(global_status, local_status, global_lock) \ | |
| if (TF_PREDICT_FALSE(!local_status.ok())) { \ | |
| global_lock.Lock(); \ | |
| global_status = local_status; \ | |
| global_lock.Unlock(); \ | |
| return; \ | |
| } | |
| do { \ | |
| const ::tensorflow::Status _s = (local_status); \ | |
| if (TF_PREDICT_FALSE(!_s.ok())) { \ | |
| ::absl::MutexLock _l(&(global_lock)); \ | |
| (global_status) = _s; \ | |
| return; \ | |
| } \ | |
| } while (0) | |
| #define NESTED_FN_STATUS_SYNC(global_status, local_status, global_lock) \\ | |
| do { \\ | |
| const ::tensorflow::Status _nested_fn_status_sync_s = (local_status); \\ | |
| if (TF_PREDICT_FALSE(!_nested_fn_status_sync_s.ok())) { \\ | |
| ::absl::MutexLock _nested_fn_status_sync_l(&(global_lock)); \\ | |
| (global_status) = _nested_fn_status_sync_s; \\ | |
| return; \\ | |
| } \\ | |
| } while (0) |
This fixes b/510434185, "Improper Error Handling and Silent Data Corruption via OP_REQUIRES_OK in Worker Lambdas". Some parsing functions:(e.g., ParsePrograms, GetProgramsAndNumQubits, GetPauliSums) used TensorFlow's OP_REQUIRES_OK macro inside worker lambdas dispatched via ParallelFor. An error such as an unparseable protobuf could result in OP_REQUIRES_OK emitting an error to the context but returning early from the worker lambda. This would not stop other threads or signal the outer function to stop execution. The outer function would return Status::OK(), and downstream kernels would execute using uninitialized or partially populated data (e.g., num_qubits remaining at -1), leading to logic corruption, OOM, or crashes.
In more detail, the problem involved a couple of places that used this in a worker:
The call of OP_REQUIRES from inside a worker thread caused 2 failures:
Thread-safety bug (data race): context is shared across the whole operation and is not thread-safe. If two worker threads hit invalid circuits at the same time, both threads would write to context's internal error fields simultaneously without a lock.
Control-flow bug (
return;exited the wrong function): Because OP_REQUIRES usesreturn;to abort, calling it inside DoWork only returned from that single worker's DoWork call, not from the outer Compute method. Once the worker pool finished, Compute kept executing even though the operation had already failed.The change in this PR follows the recommended remediation in the vulnerability report, namely to replace OP_REQUIRES_OK in lambdas with a synchronized status tracker and ensure the outer function returns the failure status. This PR also adds some test cases.
This work was done with the help of Google Gemini.