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

Handle oversized packets in tcp and udp socket tests. #9904

Merged
merged 1 commit into from Mar 28, 2019

Conversation

@michalpasztamobica
Copy link
Contributor

commented Mar 1, 2019

Description

Some modems will not be able to handle IPv4 packets larger than 536 B and we need to cope with this in our tests.
My proposal is to provide a generic function for handling the situation, in order not to obfuscate the test code too much. The function will limit the size of the packets in TCP_ECHOTEST_BURST(), where the test is able to continue. We could rewrite the other tests to do the same, but I thought that perhaps it is not their purpose to do so - they will simply

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[x] Test update
[ ] Breaking change

Reviewers

@VeijoPesonen
@SeppoTakalo
@mtomczykmobica
@KariHaapalehto
@tymoteuszblochmobica

Release Notes

Not applicable.

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

@VeijoPesonen , this PR addresses ONME-4166. Would you please let me know which platforms should trigger this behavior, so we can test? @AriParkkila, perhaps you would also know?

@ciarmcom ciarmcom requested review from KariHaapalehto, mtomczykmobica, SeppoTakalo, tymoteuszblochmobica, VeijoPesonen and ARMmbed/mbed-os-maintainers Mar 1, 2019

TESTS/netsocket/tcp/tcpsocket_echotest.cpp Outdated
@@ -71,7 +71,9 @@ void TCPSOCKET_ECHOTEST()
fill_tx_buffer_ascii(tcp_global::tx_buffer, BUFF_SIZE);

sent = sock.send(tcp_global::tx_buffer, pkt_s);
if (sent < 0) {
if (check_oversized_packets(sent, pkt_s)) {

This comment has been minimized.

Copy link
@VeijoPesonen

VeijoPesonen Mar 1, 2019

Contributor

How about just sending the same amount of data but doing it in multiple chunks instead? We are anyway talking here about a stream instead of datagram.

If the modem returns error when trying to send too much data at a time it should also be handled.
Maybe instead of proactively recognizing "oversized" packets make the check if the driver returns error like NSAPI_ERROR_PARAMETER. I would say we want to have a clear indication packet is too big instead of dropping it silently, or what ever the driver might do instead.

This comment has been minimized.

Copy link
@michalpasztamobica

michalpasztamobica Mar 1, 2019

Author Contributor

The check_oversized_packets() function does exactly what you said. It starts with:
if (error == NSAPI_ERROR_PARAMETER) {
I hid it away from the test code, as this is not a main point of any of our tests, but rather something we are forced to protect against. I think putting it all into test code would unnecessarily obfuscate it.

This comment has been minimized.

Copy link
@VeijoPesonen

VeijoPesonen Mar 4, 2019

Contributor

Ah, sorry that I didn't comprehend what I read :)

while (bt_left > 0) {
sent = sock.send(&(tcp_global::tx_buffer[BURST_SIZE - bt_left]), bt_left);

This comment has been minimized.

Copy link
@VeijoPesonen

VeijoPesonen Mar 4, 2019

Contributor

Should the receiving be skipped totally if nothing got sent?

This comment has been minimized.

Copy link
@michalpasztamobica

michalpasztamobica Mar 4, 2019

Author Contributor

I would let the test recover if it is able to. Perhaps we should not treat the situation where the modem returns an error, due to oversized package as a critical one? We can just retry and let the test time out if the modem really can't do the job. What do you think?

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Mar 4, 2019

Contributor

A TCP error is a fatal error. It means the stream has broken down. You can't get an error purely because of "I can't send this much data in one go". If the modem does do that, it's a fail.

(UDP case is totally different - there we do expect an EMSGSIZE-equivalent error for an attempt to send too large a packet. But NSAPI errors don't have a direct mapping for that)

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Mar 4, 2019

Contributor

Conceptually, this arises from TCP being a reliable transport. It's not allowed to report random errors. The data goes through cleanly in its entirety, or the connection is lost. Plus the fact it's a stream - it doesn't preserve datagram boundaries, so not being able to send 1500 bytes in one call is not a problem.

This comment has been minimized.

Copy link
@michalpasztamobica

michalpasztamobica Mar 4, 2019

Author Contributor

Ok, so what I really need to do is to rewrite all our TCP tests to match the pattern used in TCPSOCKET_ECHOTEST_BURST, as it is the only one that does the "just-send-the-remainder-and-don't-worry" part.
This particular test seems to do the job right, the other ones don't.
Thanks a lot for your remarks, @kjbracey-arm and @VeijoPesonen .

This comment has been minimized.

Copy link
@michalpasztamobica

michalpasztamobica Mar 4, 2019

Author Contributor

@kjbracey-arm , do we also expect recv to work the same way? I just got the TCPSocket::recv to return 536 instead of 600 with K64F+ethernet, using LWIP...

This comment has been minimized.

Copy link
@michalpasztamobica

michalpasztamobica Mar 4, 2019

Author Contributor

Ok, I can see the documentation explains it well enough:

     *  By default, send blocks until all data is sent.
     *  By default, recv blocks until some data is received.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Mar 4, 2019

Contributor

Asymmetry makes sense because for recv you don't (in general) know how much data you're expecting up-front - your size is usually the buffer size, not the expected data size. Plus you can often do something with partial data. Whereas for send, there's probably not a lot you can do after sending part of a buffer, if you've actually got all the data sitting there already in that buffer.

In full systems there is usually a MSG_WAITALL socket option to change the receive behaviour to wait for the buffer to be filled.

We don't provide that, even though in Mbed OS it would actually be more useful than in POSIX.

In POSIX, the "wait all" doesn't actually necessarily work because any blocking call can be interrupted by a signal, so you often need to retry on EINTR anyway. (This comes up a lot on Stack Overflow etc). We don't have that problem, so we can be confident that the send will do everything, as would MSG_WAITALL if we implemented it. (We could, in the TCPSocket layer).

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Mar 4, 2019

Contributor

Ah, the catch is that MSG_WAITALL isn't actually a socket option, is it? It's a flag for recv. And our recv doesn't currently have flags - we'd need to extend it.

This comment has been minimized.

Copy link
@michalpasztamobica

michalpasztamobica Mar 4, 2019

Author Contributor

All clear, thank you for the explanation. I update the tests to my best understanding of what we expect.
The blocking sends will now fail if the returned value is different from the expected one. (but the receives will loop until everything is received). No oversized checks are done in TCP tests, as that makes no sense.

Just waiting for @AriParkkila to let us know what is returned by the problematic modems in UDP tests.

TESTS/netsocket/tcp/main.cpp Outdated
if (error == NSAPI_ERROR_PARAMETER) {
if (get_ip_version() == NSAPI_IPv4) {
if (size > tcp_global::MAX_SEND_SIZE_IPV4) {
size = tcp_global::MAX_SEND_SIZE_IPV4;

This comment has been minimized.

Copy link
@SeppoTakalo

SeppoTakalo Mar 4, 2019

Contributor

I don't understand what you aim to do by modifying the size variable.
The function did not send the data, so you should not claim that it did.

Please refactor this function to take just int size parameter, so that it does not modify the original error code.

This comment has been minimized.

Copy link
@michalpasztamobica

michalpasztamobica Mar 4, 2019

Author Contributor

I was trying to make sure that the next loop would not use oversized packet (we want to limit the size to 536, so that the modem accepts it).
Another approach that would perhaps be more obvious is to use the return value somehow. For example return 0 (NSAPI_ERROR_OK) if the packet was not oversized or return the suggested size otherwise?

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Mar 4, 2019

Contributor

The modem can never reject a large TCP request (unless the TCP connection has broken). The underlying call may send a partial amount, which would be visible if you were using non-blocking. But that's always true. You just subtract off whatever they sent, then ask to send the remainder.

There should be no need to change the TCP tests specifically for modems with small IP MTUs, as the small IP MTU does not affect the TCP stream API. You do need to allow for limited UDP payloads.

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

Have you checked with modem that what do they return in case of TCP?

I think the NSAPI_ERROR_PARAMETER makes sense on UDP, but for TCP the modem should accept data to at least 536 bytes and return that. The concept of packet does not exist in TCP, so trying to force us to use some maximum value on send() is not acceptable. It should accept as much as it can. Then return.

In case of non-blocking sockets, it is up to application responsibility to loop until all data has been sent. On blocking case, we already do that in TCPSocket.cpp

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

@AriParkkila Can you comment here (or the internal ticket) what does the modem return if you try to Socket::send(data, 1220);

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:greentea_packet_size_fix branch 3 times, most recently to c3d3916 Mar 4, 2019

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

This review has been updated to address all the review remarks:

  • we only check the oversized packets in UDP tests,
  • TCP and TLS tests were rewritten to always expect that the whole packet gets sent, no matter how big it is even if it gets fragmented.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:greentea_packet_size_fix branch from c3d3916 to ebebfe8 Mar 21, 2019

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

Rebased and resolved conflicts.

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

@kjbracey-arm , @VeijoPesonen , I would appreciate your approvals or suggestions for further improvements, so we can finalize this PR :)

printf("[%02d] network error %d\n", i, sent);
TEST_FAIL();
goto END;
} else if (sent != BURST_SIZE) {

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Mar 25, 2019

Contributor

This set of conditions is inconsistent. If the call can return WOULDBLOCK, it can return a partial length. Either they're both acceptable (if nonblocking) or they're both not (if blocking). I'm not sure whether you're using blocking or non-blocking sockets here.

This comment has been minimized.

Copy link
@michalpasztamobica

michalpasztamobica Mar 27, 2019

Author Contributor

@kjbracey-arm , thanks, this is a good point.
The test that you commented on is a blocking one, so I removed the check for WOULD_BLOCK - we can handle it just like any other error (an fail if it shows up).
There is a non-blocking version of the echo test below this one which is allowing a WOULD_BLOCK to happen and will just retry until timeout is exceeded.
I checked that the above is true for all tcp/tls and burst/non-burst combinations.
Test are still passing.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:greentea_packet_size_fix branch from ebebfe8 to 266d4c4 Mar 27, 2019

@cmonr

cmonr approved these changes Mar 27, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 27, 2019

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 1
Build artifacts

@cmonr cmonr added ready for merge and removed needs: CI labels Mar 28, 2019

cmonr added a commit to cmonr/mbed-os that referenced this pull request Mar 28, 2019

Merge pull request ARMmbed#9904 from michalpasztamobica/greentea_pack…
…et_size_fix

Handle oversized packets in tcp and udp socket tests.

@cmonr cmonr merged commit 53920a6 into ARMmbed:master Mar 28, 2019

21 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARMC5 Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/greentea-test Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9096 cycles (-1152 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Mar 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.