KDAB / KDSoap

A Qt-based client-side and server-side SOAP component
https://www.kdab.com/development-resources/qt-tools/kd-soap
Other
146 stars 92 forks source link

KDSoapServerRawXMLInterface seems to have a design flaw #288

Closed jtjbrady closed 2 weeks ago

jtjbrady commented 1 month ago

I'm using KDSoapServerRawXMLInterface to receive POST data for a test application, however it appears to have a design flaw.

setServerSocket is called on the m_serverObject newRequest is called to pass the http headers into the object. processXML is called to pass in any data that was sent in the initial request. If the amount of data that has been received < the content length header then the slotReadyRead function returns. On subsequent invocations more data is passed in using processXML Once sufficient data has been received endRequest is called.

The problem is that if more requests comes in whilst data is being still being received then:

  1. the server socket is changed to that of the new request
  2. processXML is called with data from multiple connections, resulting in corrupt data.

When developing this software and running both the server and client on the same machine I had no issues, however when running the server and client on separate machines, I started to see corrupt data, even when sending relatively small amounts of data (content length of around 2500 bytes, which was apparently split into 3 calls to slotReadyRead).

This could be fixed by:

  1. Not sending data to processXML in batches as it comes in and treating it the same as normal SOAP requests - I can see the reason for doing things the current way - in order not to build up a massive buffer in KDSoapServerSocket.
  2. Redesign the interface and pass a unique value (probably the "this" pointer) into the newRequest, processXML and endRequest calls so the data can be disambiguated.
  3. Call setServerSocket before each call to processXML and endRequest and the recipient can then use serverSocket() to disambiguate.
dfaure-kdab commented 1 month ago

Hi, glad to see you're still using KDSoap ;-)

I'm confused though. There was supposed to be one instance of KDSoapServerObjectInterface per thread. So if your object-interface-subclass also derives from KDSoapServerRawXMLInterface, there should be oneKDSoapServerRawXMLInterface instance per thread and therefore this shouldn't happen? Concurrent requests should be processed by another server socket, another KDSoapServerObjectInterface and KDSoapServerRawXMLInterface subclass, no?

It's been a very long time since I last looked at KDSoap server's code, am I missing something?

jtjbrady commented 1 month ago

Hi David,

Yes, still using KDSoap. I stumbled upon this issue when writing a test program to simulator a speaker system that receives audio via http streaming.

I don't think I've ever used it multithreaded though, so I only have one KDSoapServerObjectInterface instance.

This problem occurs because KDSoapServerRawXMLInterface returns to the event loop allowing more incoming connections to be processed if the full payload has not been received. If a second connection comes in before the full payload is delivered that is where this issue occurs, the same KDSoapServerRawXMLInterface is called and the incoming data is "merged".

dfaure-kdab commented 1 month ago

You're right, KDSoapServer doesn't use a threadpool unless requested. The main point stands though: each new connection creates a different server object, so requests should not be mixed up. See KDSoapServer::incomingConnection => KDSoapSocketList::handleIncomingConnection => new KDSoapServerSocket => wait, this is broken, there's a single server object for all sockets in that list. Should be easy to fix. Let me make a merge request you can test.

dfaure-kdab commented 1 month ago

Can you test https://github.com/KDAB/KDSoap/pull/289? I hope the constructor of your server-object-subclass doesn't do much, because it will be called much more often now.

jtjbrady commented 3 weeks ago

Took a while to test this as I had to redesign the server class, as I was only expecting it to be created the once there was a fair bit of logic in there. However having made those changes #289 does seem to fix the issue with data being mixed up.