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.

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

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

Closed
wants to merge 3 commits into from

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 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 Sep 2, 2015
…and, also adding logging and adding condition to skip adding peer to realm if it already exists
Fix issue that peer cannot be added through jmx-console command.
@brainslog
Copy link
Collaborator

@brainslog brainslog commented May 4, 2016

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

).

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 Mar 28, 2017
@brainslog brainslog self-assigned this Mar 28, 2017
@brainslog brainslog added the bug label Mar 28, 2017
@brainslog brainslog added this to the 1.7.0.FINAL milestone Mar 28, 2017
@brainslog brainslog added this to the 1.7.0.FINAL-Sprint1 milestone Apr 3, 2017
@brainslog brainslog removed this from the 1.7.0.FINAL milestone Apr 3, 2017
@deruelle deruelle added this to the 1.7.0.FINAL-Sprint2 milestone May 4, 2017
@deruelle deruelle removed this from the 1.7.0.FINAL-Sprint1 milestone May 4, 2017
@ammendonca
Copy link
Collaborator

@ammendonca ammendonca commented May 16, 2017

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 issue May 16, 2017
ammendonca added a commit that referenced this issue May 16, 2017
Also add length to Avp `toString` method.

Fixes #1, closes #2
elost pushed a commit to elost/jdiameter that referenced this issue Aug 5, 2019
…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
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants