[CORE] Align constant offsets to 8 B in .bin serialization by LeoHeller · Pull Request #36733 · openvinotoolkit/openvino

LeoHeller · 2026-07-06T10:18:27Z

Details:

ConstantWriter::write() previously wrote constant data at whatever stream position the topological traversal produced, yielding unaligned offsets when constants of different element sizes are serialised consecutively.
At load time, the runtime mmaps the .bin and constructs a SharedBuffer whose get_ptr<T>() performs reinterpret_cast<T*>(mmap_base + offset). An unaligned offset makes this undefined behaviour.
This patch inserts up to 7 zero-bytes of padding before each constant write to bring the offset to an 8-byte boundary (the maximum scalar alignment for any OpenVINO element type). Uses the existing ov::util::align_padding_size utility.
Deduplication is checked before padding so that a duplicate hit does not leave orphaned padding bytes in the stream.
Only newly serialised .bin files get aligned offsets; old files load unchanged (their offsets are not retroactively fixed, but the reader does not validate alignment, so they remain loadable).

Tickets:

Issue [Bug]: Unaligned constant offsets in .bin serialization cause undefined behavior with mmap #36732

AI Assistance:

AI assistance used: yes
AI was used to trace the full serialisation-to-deserialisation call chain across 6+ source files and identify all downstream UB sites. Human validated every finding against the actual source, compiled and ran the reproducer with UBSan to confirm the misaligned-load diagnostic, and reviewed the final diff.

ConstantWriter::write() previously wrote constant data at whatever stream position the topological traversal produced, yielding unaligned offsets when constants of different element sizes (bool 1 B, i32 4 B, i64 8 B) were serialized consecutively. At load time the runtime mmaps the .bin and constructs a SharedBuffer (subclass of AlignedBuffer) whose get_ptr<T>() does reinterpret_cast<T*> at the stored offset. When that offset is not a multiple of alignof(T) the reinterpret_cast and subsequent dereference are undefined behaviour. Fix inserts up to 7 zero-bytes of padding before each constant write to bring the offset to an 8-byte boundary (the maximum scalar alignment for any OpenVINO element type). Deduplication is checked before padding so that a duplicate hit does not leave orphaned padding bytes. Backward-compatible: only newly serialised .bin files get aligned offsets; old files continue to load (their offsets are unchanged). Uses the existing ov::util::align_padding_size utility already employed by CompiledBlobHeader for the same purpose.

mlukasze · 2026-07-06T10:31:15Z

please add tests to cover the change

Add a round-trip test that serializes a model with mixed element-size constants (bool 1 B, i32 4 B, i64 8 B), reloads it, and asserts every constant's raw data pointer satisfies uintptr_t % element_type.size() == 0. Without the alignment fix this test fails for the i64 constant whose data lands at offset 5 in the .bin file.

LeoHeller · 2026-07-06T10:39:18Z

please add tests to cover the change

Done.

The test asserted an exact .bin file size of 2 bytes to verify two distinct 1-byte constants were not deduplicated. Alignment padding now adds 7 bytes between them. Replace the brittle size assertion with a check that the two reloaded constants have different data pointers, proving no false deduplication occurred.

t-jankowski · 2026-07-06T11:59:47Z

build_jenkins

m_out_xml_path is std::filesystem::path in SerializePassTest but readModel expects std::string. MSVC does not implicitly convert filesystem::path for overload resolution. Add .string() to match the existing call site at line 94.

t-jankowski · 2026-07-06T12:28:58Z

+        auto c = std::dynamic_pointer_cast<Constant>(node);
+        if (!c || c->get_element_type() == element::string)
+            continue;
+        auto alignment = c->get_element_type().size();


Alignment value on writing is unconditional, while here it depends on type size. Would it pass without above changes in ConstantWriter::write?

No, it would fail without the fix. it lands at offset 5 without the fix, and 5 % 8 != 0.

Possibly in the future one could pass the element type/size to the write callsite to use minimal padding, but for now always padding to 8 seems more elegant and simpler.

LeoHeller · 2026-07-06T12:47:33Z

Tests now pass locally, hopefully also in CI.

praasz · 2026-07-07T05:35:04Z

@LeoHeller
Except issue reported by sanitizer, Is lack of align make some issue like corrupted/wrong data in constant nodes?

LeoHeller · 2026-07-07T07:20:51Z

@LeoHeller Except issue reported by sanitizer, Is lack of align make some issue like corrupted/wrong data in constant nodes?

No. I believe on ARM64 it can deliver SIGBUS but usually it is fine. However we build openvino with the address sanitizer which then complains.

Could you please take a look at the failed CI job, it seems to be unrelated to my changes?
FileNotFoundError: [Errno 2] No such file or directory: '/mount/build-artifacts/dldt/master/pre_commit/ada82834a36cf9d5e37c8ee52405691efb2e8556/public_linux_ubuntu_24_04_x86_64_release/workflow_link.txt'

t-jankowski · 2026-07-07T07:35:26Z

Could you please take a look at the failed CI job, it seems to be unrelated to my changes? FileNotFoundError: [Errno 2] No such file or directory: '/mount/build-artifacts/dldt/master/pre_commit/ada82834a36cf9d5e37c8ee52405691efb2e8556/public_linux_ubuntu_24_04_x86_64_release/workflow_link.txt'

Yes, it's unrelated.

We find this issue worth solving - thanks for catching it. Currently we're preparing for 2026.3 release and it's vacation seasons - we'll get back following weeks.

praasz · 2026-07-08T08:38:52Z

build_jenkins

praasz · 2026-07-16T05:53:59Z

+    auto a3 = std::make_shared<op::v1::Add>(a2, cvt_i8);
+
+    m_model = std::make_shared<Model>(OutputVector{a3}, ParameterVector{param});
+    pass::Serialize(m_out_xml_path, m_out_bin_path).run_on_model(m_model);


use ov::save_model the xml_path is enough the bin file will be created with same name.

praasz · 2026-07-16T06:57:31Z

                                                   bool ptr_is_temporary) {
    const FilePosition write_pos = m_binary_output.get().tellp();
-    const auto offset = write_pos - m_blob_offset;
+    const auto raw_offset = write_pos - m_blob_offset;


Suggested change

const auto raw_offset = write_pos - m_blob_offset;

const FilePosition offset = m_binary_output.get().tellp() - m_blob_offset;

praasz · 2026-07-16T06:58:43Z

+    const FilePosition aligned_pos = m_binary_output.get().tellp();
+    const auto offset = aligned_pos - m_blob_offset;
+
+    if (m_enable_compression) {


Instead break main body append insert aligned bytes at end.

const auto pad_size = align_padding_size(8, static_cast<size_t>(offset)); std::fill_n(std::ostream_iterator<char>(m_binary_output.get()), pad_size, 0); m_binary_output.get().write(data_ptr, new_size); return offset + pad_size;

Then other part should not be required to modify and have same logic:

prepare

hashing

write

praasz · 2026-07-16T07:00:06Z

+    // Verify the two constants were not deduplicated (different data, different pointers)
+    std::vector<const void*> ptrs;
+    for (auto& node : model_imported->get_ops()) {
+        if (auto c = std::dynamic_pointer_cast<ov::op::v0::Constant>(node))


Suggested change

if (auto c = std::dynamic_pointer_cast<ov::op::v0::Constant>(node))

if (auto c = ov::as_type_ptr<ov::op::v0::Constant>(node))

praasz · 2026-07-16T07:01:33Z

+        auto c = std::dynamic_pointer_cast<Constant>(node);
+        if (!c || c->get_element_type() == element::string)
+            continue;


Remove continue

Suggested change

auto c = std::dynamic_pointer_cast<Constant>(node);

if (!c || c->get_element_type() == element::string)

continue;

if (auto c = ov::as_type_ptr<Constant>(node); !c || check type) {

...

}

Why string is removed from testing?

praasz · 2026-07-16T07:04:48Z

+        auto c = std::dynamic_pointer_cast<Constant>(node);
+        if (!c || c->get_element_type() == element::string)
+            continue;
+        auto alignment = c->get_element_type().size();


Using size() will make more sense if this test will parametrized TEST_P and we created test model with different types of constant to check each type is serialized with aligned address.

Then we can validate all precision

praasz · 2026-07-16T07:05:21Z

+        EXPECT_EQ(reinterpret_cast<uintptr_t>(ptr) % alignment, 0)
+            << "Constant " << c->get_friendly_name() << " data pointer is not aligned to " << alignment << " bytes";


Suggested change

EXPECT_EQ(reinterpret_cast<uintptr_t>(ptr) % alignment, 0)

<< "Constant " << c->get_friendly_name() << " data pointer is not aligned to " << alignment << " bytes";

EXPECT_EQ(reinterpret_cast<uintptr_t>(ptr) % alignment, 0)

<< c << " data pointer is not aligned to " << alignment << " bytes";

It will give more information about failing node

olpipi · 2026-08-12T08:08:14Z

I don't recommend merging this PR as-is. These changes affect both the size of the .bin file and memory usage, since the entire .bin file is loaded into memory.
In a synthetic test with a model containing 1M u8 constants of shape (1, 3), the .bin file size increased from 3 MB to 8 MB.
We should perform additional analysis to understand how significant this overhead could be in real-world user scenarios before merging this PR.

LeoHeller requested a review from a team as a code owner July 6, 2026 10:18

github-actions Bot added the category: Core OpenVINO Core (aka ngraph) label Jul 6, 2026

sys-openvino-ci added the ExternalPR External contributor label Jul 6, 2026

mlukasze linked an issue Jul 6, 2026 that may be closed by this pull request

[Bug]: Unaligned constant offsets in .bin serialization cause undefined behavior with mmap #36732

Open

3 tasks

t-jankowski reviewed Jul 6, 2026

View reviewed changes

Merge branch 'master' into fix/constant-bin-alignment

ada8283

LeoHeller requested a review from t-jankowski July 6, 2026 14:02

praasz self-assigned this Jul 7, 2026

praasz added this to the 2026.4 milestone Jul 7, 2026

t-jankowski self-assigned this Jul 7, 2026

praasz reviewed Jul 16, 2026

View reviewed changes

olpipi added the do-not-merge Do not merge this PR label Aug 12, 2026

	const auto raw_offset = write_pos - m_blob_offset;
	const FilePosition offset = m_binary_output.get().tellp() - m_blob_offset;

	if (auto c = std::dynamic_pointer_cast<ov::op::v0::Constant>(node))
	if (auto c = ov::as_type_ptr<ov::op::v0::Constant>(node))

		EXPECT_EQ(reinterpret_cast<uintptr_t>(ptr) % alignment, 0)
		<< "Constant " << c->get_friendly_name() << " data pointer is not aligned to " << alignment << " bytes";

Uh oh!

Conversation

LeoHeller commented Jul 6, 2026

Details:

Tickets:

AI Assistance:

Uh oh!

mlukasze commented Jul 6, 2026

Uh oh!

LeoHeller commented Jul 6, 2026

Uh oh!

t-jankowski commented Jul 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LeoHeller commented Jul 6, 2026

Uh oh!

praasz commented Jul 7, 2026

Uh oh!

LeoHeller commented Jul 7, 2026

Uh oh!

t-jankowski commented Jul 7, 2026

Uh oh!

praasz commented Jul 8, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

praasz Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olpipi commented Aug 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

praasz Jul 16, 2026 •

edited

Loading