search

epics-base / pvDataCPP

pvDataCPP is an EPICS V4 C++ module
https://epics-base.github.io/pvDataCPP/
Other
6 stars 16 forks source link

Buffer overflows during serialization #20

Closed mdavidsaver closed 8 years ago

mdavidsaver commented 8 years ago

Today @shengb and I were troubleshooting a crash of the masarService. The crash appeared first in a memcpy() in DefaultPVArray::serialize() due to an obviously corrupt 'space_for'. Further investigation showed that the byteBuffer involved had _position >> _limit so that getRemaining() returned a negative value cast to size_t. This was happening in a PVA send thread.

We changed PVUnionArray::serialize() to flush after serializing each element. This avoids a crash, and suggests to me that there are some missing ensureBuffer() calls. I see immediately that DefaultPVArray::serialize() calls SerializeHelper::writeSize() with no check to see that space is available. There may be others.

mdavidsaver commented 8 years ago

oops, DefaultPVArray::serialize() is calling the three argument version of writeSize().

mdavidsaver commented 8 years ago

Ok, so the culprit is probably PVUnion::serialize() which if NULL adds one byte without checking capacity. This would be consistent since the situation with masar appeared when when lots of PVs were disconnected, which is a union array with lots of NULL elements.

mdavidsaver commented 8 years ago

Should be fixed with 14b0e409f21a7a0cda417734dd2ceab3d6c0e4a2, but need confirmation.

shengb commented 8 years ago

Yes, that fixed masar crash problem.

mdavidsaver commented 8 years ago

FYI this effects all releases of pvDataCPP since unions were introduced (3.0.2 -> 5.0.0). I would proposed that this bug is serious enough to make patch release soon (say in the next month).

mrkraimer commented 8 years ago

I think that the same fix is needed in pvDataJava

msekoranja commented 8 years ago

Yes, it is. Same bug there too (the code is really consistent :) ).

mdavidsaver commented 8 years ago

As discussed in the meeting on the 12th there will be a patch/bug fix/minor release (call it what you will) to address this issue and epics-base/pvDataJava#5. This issue will be closed once the release is out.

@dhickin and I will coordinate. I'd like to do so in this thread. The steps as I see are:

  1. Merge epics-base/pvDataJava#6
  2. apply 14b0e40 to the pvDataCPP release/5.0 branch.
  3. apply epics-base/pvDataJava@731f3782c41f597a24d967027bee79cfff9a0989 to the pvDataJava release/5.0 branch.
  4. Make the CPP release tar
  5. Make the Java release

Dave, how much of this can you do? I know we discussed pull requests for the release/5.0 updates. I'm happy to do this, but given that this is just a single rev. with no conflicts I'd suggest that you just git cherry-pick when you have time to make the release tars. I think we have adequately documented this issue w/o another pair of pull requests.

dhickin commented 8 years ago

I'm happy to do 1-4. I can do 5 as well as long as Greg has no objections, since he's Java packager. I did most of the the poms for the 4.5.0 release so this should be no problem.

gregoryraymondwhite commented 8 years ago

No worries, fire when ready.

On Jan 13, 2016, at 9:22 AM, dhickin notifications@github.com wrote:

I'm happy to do 1-4. I can do 5 as well as long as Greg has no objections, since he's Java packager. I did most of the the poms for the 4.5.0 release so this should be no problem.

— Reply to this email directly or view it on GitHub.

anjohnson commented 8 years ago

Important: The epics-base/pvaClientCPP@7f972fc commit should also be included in this release, that being the one Known Problems patch that we've published so far for 4.5.0.

@dhickin I assume you'll be adding the new module release tags to the Module Release Status spreadsheet and committing a new scripts/RELEASE_VERSIONS file. AIUI the Cloudbees automated builds including publishing updated documentation versions and generating directories and soft-links in the website docbuild tree are dependent on the RELEASE_VERSIONS file, so it must be updated (just making sure).

Is someone writing/collecting some Release Notes for this version? Currently the README in the SourceForge files area for 4.5.0 points to the git scripts directory to find the Release Notes, but that isn't future-proof. I would like to put HTML versions of the Release Notes in each website docbuild// directory, but we need a standard way to convert the Markdown files into HTML for that.

I am creating and populating the website docbuild/4.5.1 area with the index and an updated issues file. Ralph's Cloudbees scripts will finish off by adding the soft-links to the individual modules when they make the release tarfiles.

dhickin commented 8 years ago

Thanks Andrew, I was just about to hit send on an email about the pvaClient fix. I can cherry-pick that commit onto the branch.

I guess I'll need to make the additions for the release notes too.

dhickin commented 8 years ago

Fixed with 14b0e40