Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Concurrent AVP encoding issue - AvpDataException: Not enough data in buffer! (fixes #1) #2

Closed
wants to merge 3 commits into from
Closed

Concurrent AVP encoding issue - AvpDataException: Not enough data in buffer! (fixes #1) #2

wants to merge 3 commits into from
+293 −84

Conversation

tomazas
Copy link

@tomazas tomazas commented Aug 17, 2015

Hi all,

We have found 2 concurrency issues with AVP encoding/decoding when doing load tests.

  1. Ecoding problem causes such exceptions:
2015-08-14 13:47:23,832 DEBUG [org.jdiameter.client.impl.controller.PeerImpl] (TCPReader-1) internalError
org.jdiameter.client.api.io.TransportException: Avp Data Exception:
        at org.jdiameter.client.impl.transport.tcp.TCPClientConnection.onEvent(TCPClientConnection.java:319)
        at org.jdiameter.client.impl.transport.tcp.TCPClientConnection.onAvpDataException(TCPClientConnection.java:276)
        at org.jdiameter.client.impl.transport.tcp.TCPTransportClient.seekMessage(TCPTransportClient.java:446)
        at org.jdiameter.client.impl.transport.tcp.TCPTransportClient.append(TCPTransportClient.java:392)
        at org.jdiameter.client.impl.transport.tcp.TCPTransportClient.run(TCPTransportClient.java:213)
        at java.lang.Thread.run(Thread.java:745)
Caused by: org.jdiameter.api.AvpDataException: org.jdiameter.api.AvpDataException: Not enough data in buffer!
        at org.jdiameter.client.impl.parser.MessageParser.createMessage(MessageParser.java:121)
        at org.jdiameter.client.impl.transport.tcp.TCPClientConnection.onEvent(TCPClientConnection.java:314)
        at org.jdiameter.client.impl.transport.tcp.TCPClientConnection.onMessageReceived(TCPClientConnection.java:271)
        at org.jdiameter.client.impl.transport.tcp.TCPTransportClient.seekMessage(TCPTransportClient.java:440)
        ... 3 more
Caused by: org.jdiameter.api.AvpDataException: Not enough data in buffer!
        at org.jdiameter.client.impl.parser.ElementParser.decodeAvpSet(ElementParser.java:319)
        at org.jdiameter.client.impl.parser.MessageParser.createMessage(MessageParser.java:116)
        ... 6 more

And also a BufferOverflow in the end:

        2015-08-14 13:47:23,835 ERROR [org.jdiameter.client.impl.transport.tcp.TCPTransportClient] (TCPReader-1) Buffer overflow occured
java.nio.BufferOverflowException
        at java.nio.HeapByteBuffer.put(HeapByteBuffer.java:183)
        at java.nio.ByteBuffer.put(ByteBuffer.java:832)
        at org.jdiameter.client.impl.transport.tcp.TCPTransportClient.append(TCPTransportClient.java:385)
        at org.jdiameter.client.impl.transport.tcp.TCPTransportClient.run(TCPTransportClient.java:213)
        at java.lang.Thread.run(Thread.java:745)

Problem - encoded/decoded Diameter packets get corrupted (missing child AVPs).

Reason - This is due to the fact that the function getGrouped that converts AvpImpl.java rawData to groupedData is called concurrently when doing encodeAvpSet(). If this is done in the loop before data.write(raw) operation in encodeAvpSet, then all child AVPs could be left unencoded (not written) to the packet (since avp.getRaw().length becomes 0). And since a correct AVP length is written here, the packet gets corrupted.

2.We also obtained a NullpointerException in SessionImpl.java, when doing a release, due to the fact that container concurrently was set to null via setContainer, so this was also fixed. However, don't know what the "// FIXME" is for, but I guess it relates to the same problem.

With this pull I propose a possible fix for this issue. Also to encapsulate more the existing data in AvpImpl.java, i.e. make rawData/groupedData not accessible from outside. ALSO to forbid to swap between rawData/groupedData in runtime, i.e. to not change created AVP.

@tomazas tomazas mentioned this pull request Aug 20, 2015
Closed
@tomazas tomazas changed the title Concurrent AVP encoding issue - AvpDataException: Not enough data in buffer! Concurrent AVP encoding issue - AvpDataException: Not enough data in buffer! (fixes #1) Aug 20, 2015
dariusliep and others added 2 commits September 2, 2015 12:17
@dariusliep
Fixing issue that peer is not added after initiating jmx-console comm…
47448f3 
…and, also adding logging and adding condition to skip adding peer to realm if it already exists
@tomazas
Merge pull request #1 from dariusliep/master
74a3ef2 
Fix issue that peer cannot be added through jmx-console command.
@brainslog
Copy link
Contributor

Hi @tomazas, would you have the time to revisit this PR so it can be merged ? It has a few conflicts now, and some of the changes are already covered in other ways (eg, addPeerName duplicate check is already done with

jdiameter/core/jdiameter/impl/src/main/java/org/jdiameter/client/impl/controller/RealmImpl.java

Line 106 in 6e911bf

if(!hasPeerName(name)) {
).

Ideally a PR should solve a specific problem, this way it's easier to review and merge each fix. Feel free to break into smaller PRs if you feel like.

Thanks.

@brainslog brainslog self-requested a review March 28, 2017 16:07
@brainslog brainslog self-assigned this Mar 28, 2017
@brainslog brainslog added the bug label Mar 28, 2017
@ammendonca
Copy link
Contributor

This PR includes several changes:

  • Changes to Avp/AvpSet encoding and decoding due to possible corruption in concurrency scenarios;
    • This issue hasn't been observed recently, even with high load and the proposed fix doesn't work properly as addGroupedAvp returns a different instance of the AVP from what is added, so operations performed on the returned AvpSet are not reflected in the parent;
    • AvpSet messageAvps = message.getAvps();
AvpSet proxyInfoOne = messageAvps.addGroupedAvp(Avp.PROXY_INFO);
// the returned proxyInfoOne is a different instance from what was added to messageAvps...
proxyInfoOne.addAvp(Avp.PROXY_HOST, "one.mobicents.org", true);
// at this point only proxyInfoOne contains the new Avp, the instance in messsageAvps is still empty```
  • Add lock to set/getContainer methods for concurrency issues;
    • This is a safe fix, not causing side-effects, will apply.
  • Log improvements to TCP receive/send to show packets in HEX format;
    • Nice improvement for debugging puproses, will apply.
  • Fix to JMX operation for add Peer to include realm name;
    • Valid issue and fix, will apply.

Will prepare commits with the valid fixes, in case the first issue is observed again we can revisit and try to fix it a better way.

ammendonca added a commit that referenced this pull request May 16, 2017
@ammendonca
Add realm name parameter to _Network_Peers_addPeer
7daf1b9 
Fixes #1, closes #2
ammendonca added a commit that referenced this pull request May 16, 2017
@ammendonca
Add trace log to HEX dump in/out Diameter messages
a6d0eac 
Also add length to Avp `toString` method.

Fixes #1, closes #2
elost pushed a commit to elost/jdiameter that referenced this pull request Aug 5, 2019
@azulkarnyaev
Merge pull request RestComm#2 from elost/feature/fix-concurrency-issu…

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
91d14fa 
…es-in-PeerFSM

Feature/fix concurrency issues in peer fsm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brainslog brainslog

Assignees

@brainslog brainslog

Labels
bug
Projects
None yet
Milestone
1.7.0.FINAL-Sprint2
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tomazas @brainslog @ammendonca @deruelle @dariusliep