RestComm / jdiameter

RestComm Diameter Stack and Services
http://www.restcomm.com/
GNU Affero General Public License v3.0
112 stars 151 forks source link

SCTP buffer overflow #59

Closed SummaNetworks closed 7 years ago

SummaNetworks commented 7 years ago

With the current SCTP version of the library used on the diameter, there is a hard limit on the buffer size for the SCTP message. When the buffer is smaller than the message, the exception is not well handled, so, an exception will be thrown: 2016-09-19 13:03:52,839 ERROR SelectorThread Error while selecting the ready keys java.nio.BufferOverflowException at java.nio.DirectByteBuffer.put(DirectByteBuffer.java:357) at java.nio.ByteBuffer.put(ByteBuffer.java:832) at org.mobicents.protocols.sctp.AssociationImpl.write(AssociationImpl.java:611) at org.mobicents.protocols.sctp.SelectorThread.write(SelectorThread.java:436) at org.mobicents.protocols.sctp.SelectorThread.run(SelectorThread.java:157) at java.lang.Thread.run(Thread.java:745) And the message on the next iteration willl be sent.

We are using the stack to send the SAR message with a big XML (8000bytes), so its not working: We had to patch the SCTP library with new values:

// The buffer into which we'll read data when it's available private ByteBuffer rxBuffer = ByteBuffer.allocateDirect(16384); private ByteBuffer txBuffer = ByteBuffer.allocateDirect(16384);

Do you plan to upgrade the SCTP library?

deruelle commented 7 years ago

We support Netty now in the master, I guess adding SCTP shouldn't be much work and potentially provide greater flexibility in that regard ? We could potentially upgrade the SCTP library backed by Netty from https://github.com/RestComm/sctp that is used in jSS7 as well for consistency. @SummaNetworks would you like to try to contribute that ?

SummaNetworks commented 7 years ago

Hi Jean, Thanks for the quick reply. Unfortunately, we tried updating the SCTP library, but the methods signatures have changed, so it might imply a backward compatibility issue. Could you asses the impact?

vetss commented 7 years ago

Hi @SummaNetworks

"Do you plan to upgrade the SCTP library?"

We have implemented new version of SCTP lib with netty support and some bug (including mentioned by you) fixing. We have two different implementation now: 1) nio based (an old one) that uses AssociationImpl and ManagementImpl classes 2) netty based (a new one) that uses NettyAssociationImpl and NettySctpManagementImpl classes You uses the first one. May I ask you to check if it is possible to switch to the second one ? Changing of a biffer size is also possible of cause althow we faced also with other problems with the nio version solving of which is difficult

Can you please exaplain more this (which exactly changes are you afraid):

but the methods signatures have changed, so it might imply a backward compatibility issue

SummaNetworks commented 7 years ago

Hi all,

Looking at the pom files from the diameter, we can see:

org.mobicents.protocols.sctp sctp-impl 1.6.0.FINAL
<dependency>
  <groupId>org.mobicents.protocols.sctp</groupId>
  <artifactId>sctp-api</artifactId>
  <version>1.6.0.FINAL</version>

Which is the SCTP version without netty. We tried upgrading to a newer version of the SCTP library in the jdiameter, but it didnt work, it failed as method signature was incorrect (related to parameters not matching). Not sure if the upgrade could potentially break anything that might affect the jdiameter... :-)

deruelle commented 7 years ago

@SummaNetworks the workaround you did in https://github.com/RestComm/jdiameter/issues/59#issue-179459462 is the safest for now if you don't want to upgrade jdiameter SCTP to Netty but on the long term upgrading the sctp library to the netty is the best and can be done as part of the 1.7.0 release. Feel free to contribute the upgrade and we can run testsuite and perf tests to check for any issues.

vetss commented 7 years ago

Hello @SummaNetworks @deruelle

I have created a separate issue for switching to a new SCTP lib version: https://github.com/RestComm/jdiameter/issues/60

Let's discuss of "switching to netty" stuff there.

vetss commented 7 years ago

Hello @SummaNetworks

1) I can just increase of buffer length to some more value (which one do you think is reasonable from diameter point of view ?)

2) it is better to switch jdiameter lib for usage of the latest SCTP lib + reuse of netty (for SCTP protocol) https://github.com/RestComm/jdiameter/issues/60

SummaNetworks commented 7 years ago

Hi all,

  1. We did patch the 1.6 version to increase the buffer to 16MB. At this point, not sure what should be a reasonable size, the XML for the Cx interface can be pretty big if you have 50 triggers.
  2. Yes, the idea is to upgrade the lib specially if the netty implementation is more up to date, and will have more bug-fixing. We tried, but had an issue with method signatures, so, it will require a carefull lookup.

Summa