Revert D100115639

[lucylq]

Summary:
This diff reverts D100115639
T264111177

Depends on D100115639

Differential Revision: D100228771

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/18800

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

• Rolling out OSDC (ARC) runners on pull workflow for PyTorch trunk commits

❌ 1 Cancelled Job, 2 Unrelated Failures

As of commit 906fe30 with merge base ecd97d5 ():

CANCELLED JOB - The following job was cancelled. Please retry:

• periodic (gh)

BROKEN TRUNK - The following jobs failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

• pull / unittest / windows / windows-job (gh) (trunk failure)
  ##[error]The operation was canceled.
• pull / unittest-editable / windows / windows-job (gh) (trunk failure)
  ##[error]The operation was canceled.

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[meta-codesync]

@lucylq has exported this pull request. If you are a Meta employee, you can view the originating Diff in D100228771.

[github-actions]

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

[Copilot]

Pull request overview

This PR reverts a prior change (D100115639) in the extension data loaders, primarily backing out use of c10::safe_numerics overflow-checked arithmetic and simplifying some bounds checks/error text.

Changes:

• Removed c10/util/safe_numerics.h usage and the explicit add-overflow validation from several DataLoader implementations.
• Replaced overflow-safe range validation with direct offset + size <= ... checks.
• Simplified an error message in BufferDataLoader to no longer mention overflow.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
extension/data_loader/shared_ptr_data_loader.h	Drops overflow-safe bounds check for shared-buffer loads.
extension/data_loader/mmap_data_loader.cpp	Drops overflow-safe validation for mmap read ranges.
extension/data_loader/file_descriptor_data_loader.cpp	Drops overflow-safe validation for file-descriptor read ranges (load/load_into).
extension/data_loader/file_data_loader.cpp	Drops overflow-safe validation for file read ranges (load/load_into).
extension/data_loader/buffer_data_loader.h	Updates error message text while still using overflow detection.

Comments suppressed due to low confidence (8)

extension/data_loader/mmap_data_loader.cpp:169

• offset + size <= file_size_ is not overflow-safe: if offset + size wraps, validation can incorrectly succeed, and later get_overlapping_pages() also does offset + size arithmetic that can overflow. Prefer an overflow-safe check like offset <= file_size_ && size <= file_size_ - offset (or use an explicit add-overflow helper).

  ET_CHECK_OR_RETURN_ERROR(
      offset + size <= file_size_,
      InvalidArgument,
      "File %s: offset %zu + size %zu > file_size_ %zu",
      file_name_,
      offset,
      size,
      file_size_);

extension/data_loader/file_descriptor_data_loader.cpp:167

• This bounds check is vulnerable to size_t wraparound (offset + size can overflow and become small), which can allow out-of-bounds reads. Use an overflow-safe formulation such as offset <= file_size_ && size <= file_size_ - offset, or an explicit add-overflow check.

  ET_CHECK_OR_RETURN_ERROR(
      offset + size <= file_size_,
      InvalidArgument,
      "File %s: offset %zu + size %zu > file_size_ %zu",
      file_descriptor_uri_,
      offset,
      size,
      file_size_);

extension/data_loader/file_descriptor_data_loader.cpp:228

• Same overflow issue here: offset + size <= file_size_ can wrap and bypass validation. Use an overflow-safe check (offset <= file_size_ && size <= file_size_ - offset) or an explicit add-overflow helper before calling pread.

  ET_CHECK_OR_RETURN_ERROR(
      offset + size <= file_size_,
      InvalidArgument,
      "File %s: offset %zu + size %zu > file_size_ %zu",
      file_descriptor_uri_,
      offset,
      size,
      file_size_);

extension/data_loader/file_data_loader.cpp:153

• offset + size <= file_size_ can overflow and wrap, allowing invalid ranges to pass validation. Prefer an overflow-safe check such as offset <= file_size_ && size <= file_size_ - offset, or use an explicit add-overflow helper.

  ET_CHECK_OR_RETURN_ERROR(
      // Probably had its value moved to another instance.
      fd_ >= 0,
      InvalidState,
      "Uninitialized");
  ET_CHECK_OR_RETURN_ERROR(
      offset + size <= file_size_,
      InvalidArgument,
      "File %s: offset %zu + size %zu > file_size_ %zu",
      file_name_,
      offset,
      size,
      file_size_);

extension/data_loader/file_data_loader.cpp:214

• Same overflow concern in load_into: offset + size can wrap and bypass the bounds check, potentially leading to incorrect reads. Use an overflow-safe check (offset <= file_size_ && size <= file_size_ - offset) or an explicit add-overflow helper.

  ET_CHECK_OR_RETURN_ERROR(
      // Probably had its value moved to another instance.
      fd_ >= 0,
      InvalidState,
      "Uninitialized");
  ET_CHECK_OR_RETURN_ERROR(
      offset + size <= file_size_,
      InvalidArgument,
      "File %s: offset %zu + size %zu > file_size_ %zu",
      file_name_,
      offset,
      size,
      file_size_);

extension/data_loader/mmap_data_loader.cpp:170

• Consider adding a unit test that verifies validate_input/load rejects ranges where offset + size would overflow (wraparound). This module has OOB tests already, but overflow-specific coverage would prevent regressions.

  ET_CHECK_OR_RETURN_ERROR(
      offset + size <= file_size_,
      InvalidArgument,
      "File %s: offset %zu + size %zu > file_size_ %zu",
      file_name_,
      offset,
      size,
      file_size_);
  return Error::Ok;

extension/data_loader/file_data_loader.cpp:153

• The existing FileDataLoader tests cover out-of-bounds ranges but not integer-overflow ranges. Add a regression test that uses a huge size (e.g., SIZE_MAX - offset + 1) and expects InvalidArgument, to ensure wraparound can’t bypass the bounds check.

  ET_CHECK_OR_RETURN_ERROR(
      // Probably had its value moved to another instance.
      fd_ >= 0,
      InvalidState,
      "Uninitialized");
  ET_CHECK_OR_RETURN_ERROR(
      offset + size <= file_size_,
      InvalidArgument,
      "File %s: offset %zu + size %zu > file_size_ %zu",
      file_name_,
      offset,
      size,
      file_size_);

extension/data_loader/file_descriptor_data_loader.cpp:167

• The test suite covers out-of-bounds loads but doesn’t appear to cover the offset + size wraparound case. Add an overflow regression test (similar to BufferDataLoaderTest.OverflowLoadFails) to ensure the bounds check can’t be bypassed via integer overflow.

  ET_CHECK_OR_RETURN_ERROR(
      offset + size <= file_size_,
      InvalidArgument,
      "File %s: offset %zu + size %zu > file_size_ %zu",
      file_descriptor_uri_,
      offset,
      size,
      file_size_);

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The bounds check offset + size <= size_ can overflow size_t and wrap, allowing an out-of-bounds slice to pass validation. Use an overflow-safe check (e.g., offset <= size_ && size <= size_ - offset) or __builtin_add_overflow/similar to reject wrapped additions.

[Copilot]

The check still uses c10::add_overflows(...) and fails when overflow is detected, but the error message no longer mentions overflow. Either restore the message text to include overflow, or restructure the validation so the message matches the actual failure conditions.

Suggested change

	"offset %zu + size %zu > size_ %zu",
	"offset %zu + size %zu overflows or exceeds size_ %zu",

[Copilot]

There are out-of-bounds tests for SharedPtrDataLoader, but no regression test for the offset + size overflow case. Add a unit test that passes values like offset=128, size=SIZE_MAX-127 and expects InvalidArgument, so future refactors don’t reintroduce wraparound bugs.