Add protection against some network issues by anjohnson · Pull Request #226 · epics-base/pvAccessCPP

anjohnson · 2026-07-10T04:40:49Z

Moved from the private area.

By commit:

Reject negative payload sizes in messages over UDP and TCP, with tests.
It looks like this module can't handle TCP or UDP messages over 2^31 Bytes, although the TCP code apparently understands joining segmented messages.
Ignore over-long channel names in search messages.
This commit skips over names longer than would be accepted by the channel create code. A better response might be to immediately discard the search message, research needed on how to implement that.
Uncomment Lock guards that were improperly disabled long ago.
See the gist here (2nd document) for Claude's assessment of the lock guard history.
Prevent deadlocks in destroy() methods.
See the gist here (1st document) for Claude's explanation of fixes like one by @mdavidsaver in 2018.
Limit strings accepted as channel names to MAX_CHANNEL_NAME_LENGTH, ignoring anything larger. This requires a change implemented in my pvData add-protection branch, Add protection from some network issues pvDataCPP#107

codacy-production · 2026-07-10T04:41:44Z

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 8 complexity · 0 duplication

Metric Results

Complexity 8

Duplication 0

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-10T09:35:45Z

✅ Build pvAccessCPP 1.0.159 completed (commit 8c5d306e77 by @anjohnson)

Control messages treat this int32 as an opaque value, but application messages use it as a length, so must reject it. Applies to both UDP and TCP streams. Test added to testCodec.cpp

They're already validated in the channel-creation path.

I had AI analyse these commented-out guards and produce a report, which said they should all be uncommented. Happy to discuss them or provide a copy on request.

anjohnson · 2026-07-24T21:21:14Z

Oops, that last commit needs a change from my pvDataCPP add-protection branch.

mdavidsaver · 2026-07-24T22:24:20Z

Reject negative payload sizes in messages over UDP and TCP, with tests.
It looks like this module can't handle TCP or UDP messages over 2^31 Bytes, although the TCP code apparently understands joining segmented messages.

No problem with UDP.

I have some hesitation about TCP. Have you instead tried to switch _payloadSize to uint32?

The semantics around segmented messages has never been clear to me. Currently PVXS can receive segmented messages, but will never send them. It does handle message length as unsigned 32-bit.

@kasemir Does core.pva do anything with the segmentation flags?

AppVeyorBot · 2026-07-25T00:41:29Z

❌ Build pvAccessCPP 1.0.161 failed (commit 1308b3737f by @anjohnson)

kasemir · 2026-07-25T03:46:12Z

@kasemir Does core.pva do anything with the segmentation flags?

There is code to detect a segmented message and assemble the pieces, but I don't think I've ever been able to check that against a PVA server which generates such messages.

https://github.com/ControlSystemStudio/phoebus/blob/51a9bf6e4f882b21830f94531ba795a42f915c58/core/pva/src/main/java/org/epics/pva/common/TCPHandler.java#L426

anjohnson · 2026-07-29T23:08:09Z

Have you instead tried to switch _payloadSize to uint32?

Why? Doing that would require much more complicated changes to the code and tests.

The PVA protocol Wiki explicitly says about sizes:

If the number of elements is less than 2^31-1, then the size MUST be encoded as an unsigned 8-bit integer with value 254, followed by a positive signed 32-bit integer indicating the number of elements.
Values greater than or equal to 2^31-1 are currently not implemented.

It does go on to suggest how larger sizes would be encoded if they were supported though. Have you implemented them in PVXS without updating the spec? Does the new Java implementation also support that?

AppVeyorBot · 2026-08-05T14:05:24Z

✅ Build pvAccessCPP 1.0.165 completed (commit 459f6df111 by @anjohnson)

AppVeyorBot · 2026-08-05T19:13:15Z

✅ Build pvAccessCPP 1.0.166 completed (commit cef563ab8d by @anjohnson)

anjohnson

Core comments with detail fixes.

anjohnson · 2026-08-19T14:28:23Z

 {
    if(impl) {
-        strm<<typeid(*impl->channel.get()).name()<<" : ";
+        pva::Channel& channel = *impl->channel;


Needs to call get() to get the type-id of the contained object. Ditto below.

Move to a non-hardening PR.

anjohnson · 2026-08-19T14:29:43Z

    POINTER_DEFINITIONS(PipelineControl);

-    virtual ~PipelineControl() {};
+    virtual ~PipelineControl();


Not hardening.

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


        // command ID and paylaod
        int8 command = receiveBuffer->getByte();
        // TODO check this cast (size_t must be 32-bit)


Remove TODO comment.

anjohnson · 2026-08-19T14:45:52Z

        size_t payloadSize = receiveBuffer->getInt();

        // control message check (skip message)
        if (flags & 0x01)
            continue;

+        // reject negative payload size (sign-extends to a huge size_t)
+        if (static_cast<int32>(payloadSize) < 0)
+            return false;
+


Core: Add an int32 payload size var above which is assigned from the buffer, then exclude negatives and assign to the size_t version (with no renames or casts below).

anjohnson · 2026-08-19T14:51:53Z

    _command = _socketBuffer.getByte();

    // read payload size
    _payloadSize = _socketBuffer.getInt();


Do the –ve checks for payload size with the magicCode check on 193 below, not later on.

anjohnson · 2026-08-19T15:13:53Z

+
+    if (channelGet)
+        channelGet->destroy();


MAD checked and is good with this.

anjohnson · 2026-08-19T15:18:53Z

-            //Lock guard(_mutex);
+            Lock guard(_mutex);
            _pvPutBitSet->serialize(buffer, control);
            _pvPutStructure->serialize(buffer, control, _pvPutBitSet.get());
        }


What data is being guarded here?

anjohnson · 2026-08-19T15:20:05Z

+            Lock guard(_mutex);
            ScopedLock lock(channelArray);
            _pvArray->serialize(buffer, control, 0, _pvArray->getLength());


Locking order here is different than above QOS stuff, is this correct?

anjohnson · 2026-08-19T15:21:00Z

+    Channel& channel = *_channel;
    fprintf(fd,"CLASS        : %s\n", typeid(*this).name());
-    fprintf(fd,"CHANNEL      : %s\n", typeid(*_channel).name());
+    fprintf(fd,"CHANNEL      : %s\n", typeid(channel).name());


Ack, but not hardening.

anjohnson · 2026-08-19T15:30:46Z

+                // reject negative payload size (sign-extends to a huge size_t)
+                if (_payloadSize < 0)
+                {
+                    invalidDataStreamHandler();
+                    throw invalid_data_stream_exception("negative payload size");
+                }
+


Remove this change, and the next one.

anjohnson marked this pull request as draft July 10, 2026 04:43

anjohnson added 4 commits July 24, 2026 15:46

Reject payloadSize < 0 in application messages

6bbb109

Control messages treat this int32 as an opaque value, but application messages use it as a length, so must reject it. Applies to both UDP and TCP streams. Test added to testCodec.cpp

Ignore over-length channel names in searches

5045a4c

They're already validated in the channel-creation path.

Uncomment lock guards

bb7a3d4

I had AI analyse these commented-out guards and produce a report, which said they should all be uncommented. Happy to discuss them or provide a copy on request.

Clean up locking in some destroy() methods

8e409d1

anjohnson force-pushed the add-protection branch from cc2de0f to f6b31b9 Compare July 24, 2026 20:48

anjohnson marked this pull request as ready for review July 29, 2026 23:11

anjohnson force-pushed the add-protection branch from f6b31b9 to 555fff2 Compare August 5, 2026 05:27

anjohnson added 5 commits August 5, 2026 00:54

Limit channel name lengths

d343824

Missing return after plugin check

161ee6c

Avoid null dereference, even if not used

ece22fc

Clean up annoying Clang warnings

1798385

Protect against array size overflows

fead494

anjohnson force-pushed the add-protection branch from 555fff2 to fead494 Compare August 5, 2026 05:55

anjohnson commented Aug 19, 2026

View reviewed changes

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!

anjohnson commented Jul 24, 2026

Uh oh!

mdavidsaver commented Jul 24, 2026

Uh oh!

AppVeyorBot commented Jul 25, 2026

Uh oh!

kasemir commented Jul 25, 2026

Uh oh!

anjohnson commented Jul 29, 2026

Uh oh!

AppVeyorBot commented Aug 5, 2026

Uh oh!

AppVeyorBot commented Aug 5, 2026

Uh oh!

anjohnson left a comment

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!

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!

Choose a reason for hiding this comment

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