Add protection from some network issues by anjohnson · Pull Request #107 · epics-base/pvDataCPP

anjohnson · 2026-07-10T05:20:21Z

Malicious traffic from the network wasn't being checked properly. These changes don't prevent sending huge arrays or string values, but they do stop issues related to overflows caused by negative sizes, which PVA specifies to be positive int32 values.

Limit allocation in BitSet::deserialize() to the size actually needed. Support deserializing a NULL value. Includes new tests.

2, 3. The PVStructureArray, PVUnionArray and PVValueArray<string> deserialize() methods now allocate their storage as data arrives, and reject negative sizes. Includes new basic and malicious tests.

Add a maximum string length to SerializeHelper::deserializeString(), with a default of INT32_MAX.
Protect Status::::deserialize() from a bad status.
Protect PVUnion::deserialize() from a bad selector value.

codacy-production · 2026-07-10T05:24:00Z

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity · -2 duplication

Metric Results

Complexity 0

Duplication -2

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer}
_{TIP This summary will be updated as you push new changes.}

AppVeyorBot · 2026-07-10T13:32:50Z

✅ Build pvDataCPP 1.0.81 completed (commit 3ab6d1c8c3 by @anjohnson)

AppVeyorBot · 2026-07-14T15:10:45Z

✅ Build pvDataCPP 1.0.82 completed (commit d0d55663d1 by @anjohnson)

anjohnson · 2026-07-14T19:09:20Z

How to handle the POD array template PVValueArray<T>::deserialize(), which I haven't done yet? There is a short-cut there: when the endianness matches all the elements can be copied straight from the message buffer, but that could be a huge read.

I'm currently looking at the RTEMS-4.9 test failure.

anjohnson · 2026-07-14T19:30:04Z

The pvData / 7.0 Ub-22 gcc + RT-4.9 pc386 build is failing in the new testSerialization tests, the .tap output starts and ends like this:

1..290
# Testing introspection serialization...
ok  1 - factory.get()!=NULL
…
ok 246 - grow/reuse round trip, 129 elements
ok 247 - grow/reuse round trip, 129 elements
ok 248 - grow/reuse round trip, 1000 elements
rtems_gxx_mutex_init
fatal error, exiting

Those symptoms look like a stack overflow or similar issue, 1000 elements is the largest the new tests request. The RT-4.10 and 5.1 builds don't fail; is this worth chasing?

AppVeyorBot · 2026-07-14T22:35:13Z

✅ Build pvDataCPP 1.0.83 completed (commit 61dbdc3e7c by @anjohnson)

anjohnson

/code-review comments from Claude; some fixes to come, but not the generic PVValueArray<T>::deserialize() fix which we haven't decided on yet.

anjohnson · 2026-07-21T19:43:09Z

+    data.resize(0);
+    data.reserve(size < 64 ? size : 64);


Claude Sonnet 5: The old per-element reuse optimization (reuse still-unique PVStructure/PVUnion objects across deserialize calls) is silently discarded.
The new reserve(min(size,64)) + push_back growth pattern interacts badly with shared_vector's fixed +1024-element growth increment above 1024 items, causing O(n²) copy work for legitimately large arrays.

The shared_vector<…>::push_back() method only grows geometrically up to 1024 elements, thereafter it adds 1024 elements every time it expands. An areaDetector NTNDArray has an epics:nt/NTAttribute:1.0[] attribute array field which would hit the O(n²) allocation, but hopefully only the first time the data is published.

Subsequent monitor updates should re-use the array storage where the attribute list doesn't change, but some experiments may continue to add attributes over time, and those would be the ones I worry about.

Subsequent monitor updates should re-use the array storage ...

A reuse() is attempted because it is low cost. However, I think it is unlikely to succeed in the most important cases (AD). It would be unlikely in the case of a QSRV1 group mapping as a reference to the most recent value is kept.

I worry that this attempt to "harden" against a malicious peer will unduly harm performance on all others. So I am going to hold off on this part until the core developer group agrees on a policy on regarding DoS mitigations which negatively impact performance.

AppVeyorBot · 2026-07-22T02:52:27Z

✅ Build pvDataCPP 1.0.85 completed (commit b43c30a277 by @anjohnson)

Move bounds check to before the allocation. Treat NULL bitset values as empty to avoid overflow. Add tests for excessive allocation, and Null values.

Ensure container types PVStructureArray and PVUnionArray can't be used to allocate huge arrays, unless they really are that big. New containers start small and grow as as elements are deserialized from the input stream.

Ensure PVValueArray<string> containers can't allocate huge arrays unless the client provides string values for all the elements. Start containers small and grow them as elements are deserialized from the input stream. Adds tests for all the array deserialization hardening. Limit array growth tests on RTEMS 4.x.

AppVeyorBot · 2026-07-27T20:48:00Z

✅ Build pvDataCPP 1.0.86 completed (commit 4d46ed1dbe by @anjohnson)

AppVeyorBot · 2026-07-28T00:42:22Z

✅ Build pvDataCPP 1.0.87 completed (commit 4fbed20dcd by @anjohnson)

AppVeyorBot · 2026-07-28T04:32:34Z

✅ Build pvDataCPP 1.0.88 completed (commit 95f04cfc9d by @anjohnson)

AppVeyorBot · 2026-08-01T00:14:56Z

✅ Build pvDataCPP 1.0.91 completed (commit 7f84e62be6 by @anjohnson)

Defaults to INT32_MAX if not provided, but several places in pvAccess can set limits, e.g. MAX_CHANNEL_NAME_LENGTH, and this is the best way to harden them against huge values. Document that SerializeHelper::readSize() returns -1 for NULL.

codacy-production · 2026-08-05T05:25:01Z

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity · -2 duplication

Metric Results

Complexity 0

Duplication -2

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer}
_{TIP This summary will be updated as you push new changes.}

AppVeyorBot · 2026-08-05T08:54:18Z

❌ Build pvDataCPP 1.0.92 failed (commit 1f6f3b31dc by @anjohnson)

AppVeyorBot · 2026-08-05T19:39:24Z

✅ Build pvDataCPP 1.0.92 completed (commit 1f6f3b31dc by @anjohnson)

mdavidsaver

I have picked off the uncontentious parts of this PR as 5012ff9, 2c5854a, e7c69cb, and 89d2702.

mdavidsaver · 2026-08-10T01:13:07Z

+            selector = index;
+            value.reset();
+        }
+        else if (index < 0 || size_t(index) >= unionPtr->getFields().size())


This check is redundant to the later getField(selector);, which bounds checks its argument. Still, an explicit check here gives a better error message.

mdavidsaver · 2026-08-10T01:19:09Z

             */
            static std::string deserializeString(ByteBuffer* buffer,
-                    DeserializableControl* control);
+                    DeserializableControl* control, std::size_t max = INT32_MAX);


No objection to the added argument. I have a small concern about whether the INT32_MAX macro will be defined on all targets. Strictly speaking, the C++ way is std::numeric_limits. However, since this is a fixed value, I will just change it to 0x7fffffff.

mdavidsaver · 2026-08-10T01:21:56Z

+        m_statusType = STATUSTYPE_FATAL;
+        m_message = "deserialized type code was invalid";
+        m_stackDump.clear();
+        THROW_BASE_EXCEPTION("invalid Status type code");


While I do not see much chance that we will want to add codes in future, it seems unnecessary to close the door on this possibility now. I will instead treat all currently unknown codes as FATAL.

mdavidsaver · 2026-08-10T01:33:13Z

+    data.resize(0);
+    data.reserve(size < 64 ? size : 64);


Subsequent monitor updates should re-use the array storage ...

A reuse() is attempted because it is low cost. However, I think it is unlikely to succeed in the most important cases (AD). It would be unlikely in the case of a QSRV1 group mapping as a reference to the most recent value is kept.

I worry that this attempt to "harden" against a malicious peer will unduly harm performance on all others. So I am going to hold off on this part until the core developer group agrees on a policy on regarding DoS mitigations which negatively impact performance.

anjohnson · 2026-08-19T14:17:36Z

Core: Some changes have been merged by cherry-pick. This PR needs further thought, the performance-affecting changes might not be wanted.

anjohnson commented Jul 10, 2026

View reviewed changes

Comment thread src/factory/PVStructureArray.cpp Outdated

anjohnson force-pushed the add-protection branch from 050cf76 to c22ea27 Compare July 14, 2026 05:32

anjohnson force-pushed the add-protection branch from c22ea27 to 2cc9ee0 Compare July 14, 2026 18:39

anjohnson commented Jul 21, 2026

View reviewed changes

anjohnson force-pushed the add-protection branch from 2cc9ee0 to be0d506 Compare July 21, 2026 22:57

anjohnson mentioned this pull request Jul 24, 2026

Add protection against some network issues epics-base/pvAccessCPP#226

Open

anjohnson added 2 commits July 27, 2026 11:42

Harden array allocation in BitSet::deserialize()

f568032

Move bounds check to before the allocation. Treat NULL bitset values as empty to avoid overflow. Add tests for excessive allocation, and Null values.

Harden Struct/Union Arrays in deserialize()

8923d38

Ensure container types PVStructureArray and PVUnionArray can't be used to allocate huge arrays, unless they really are that big. New containers start small and grow as as elements are deserialized from the input stream.

anjohnson force-pushed the add-protection branch 2 times, most recently from df17c76 to a8e4961 Compare July 27, 2026 19:17

anjohnson force-pushed the add-protection branch from a8e4961 to a0e1375 Compare July 27, 2026 19:51

anjohnson marked this pull request as ready for review July 27, 2026 20:19

anjohnson added 3 commits August 5, 2026 00:04

Protect against bad Status values

741903b

Protect PVUnion selector in deserialize

84e29e8

anjohnson force-pushed the add-protection branch from 8b56d62 to 84e29e8 Compare August 5, 2026 05:24

mdavidsaver reviewed Aug 10, 2026

View reviewed changes

anjohnson marked this pull request as draft August 19, 2026 14:17

Uh oh!

Conversation

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

Uh oh!

Uh oh!

codacy-production Bot commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Up to standards ✅

Uh oh!

AppVeyorBot commented Jul 10, 2026

Uh oh!

Uh oh!

AppVeyorBot commented Jul 14, 2026

Uh oh!

anjohnson commented Jul 14, 2026

Uh oh!

anjohnson commented Jul 14, 2026

Uh oh!

AppVeyorBot commented Jul 14, 2026

Uh oh!

anjohnson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

anjohnson Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

anjohnson Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

mdavidsaver Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AppVeyorBot commented Jul 22, 2026

Uh oh!

AppVeyorBot commented Jul 27, 2026

Uh oh!

AppVeyorBot commented Jul 28, 2026

Uh oh!

AppVeyorBot commented Jul 28, 2026

Uh oh!

AppVeyorBot commented Aug 1, 2026

Uh oh!

codacy-production Bot commented Aug 5, 2026

Up to standards ✅

Uh oh!

AppVeyorBot commented Aug 5, 2026

Uh oh!

AppVeyorBot commented Aug 5, 2026

Uh oh!

mdavidsaver left a comment • 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!

mdavidsaver Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

mdavidsaver Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

mdavidsaver Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

mdavidsaver Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

anjohnson commented Aug 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

anjohnson commented Jul 10, 2026 •

edited

Loading

codacy-production Bot commented Jul 10, 2026 •

edited

Loading

mdavidsaver left a comment •

edited

Loading