test: Add more unit test coverage to s2n_send.c #3347

lundinc2 · 2022-06-06T16:59:13Z

Resolved issues:

None

Description of changes:

This PR adds more unit test coverage to s2n_send.c.

Call-outs:

Address any potentially confusing code. Is there code added that needs to be cleaned up later? Is there code that is missing because it’s still in development?

Testing:

How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?
N/A

Is this a refactor change? If so, how have you proved that the intended behavior hasn't changed?
N/A

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

lrstewart · 2022-06-08T05:31:15Z

tests/unit/s2n_send_test.c

+        EXPECT_SUCCESS(s2n_connection_set_ctx(conn, (void*)&total_bytes_to_send));
+        EXPECT_SUCCESS(s2n_connection_set_send_cb(conn, s2n_counted_send_fn));
+        EXPECT_SUCCESS(s2n_connection_set_send_ctx(conn, (void*) conn));


Couldn't you just put total_bytes_to_send in the send context and avoid the extra indirection?

I need to check conn->current_user_data_consumed to get the amount of user data sent in the callback :/. I can't rely on the len in the callback because it's encrypted.

lrstewart · 2022-06-08T05:34:11Z

tests/unit/s2n_send_test.c

+        EXPECT_EQUAL(conn->current_user_data_consumed + written, conn->active_application_bytes_consumed);
+


This line might need a comment, bc I'm not sure the variable names describe what's happening well. Is conn->current_user_data_consumed the remaining data to write?

If the variable name isn't great but would be a pain to change, you could write an EXPECT macro that describes what you're looking for. Like maybe "EXPECT_WRITTEN(written)"?

I'll rewrite this to make it clearer.

conn->active_application_bytes_consumed is the total amount of data that has been processed in s2n_send. It resets to 0 w/e the dynamic record thresholds are hit if they are enabled. In this case it is not enabled so we can use it to track the amount of bytes that have been sent to the socket (successful + unsuccessful).

conn->current_user_data_consumed is tracking the amount of data that has been successfully flushed.

In the case of a partial write conn->current_user_data_consumed is set to the amount of bytes that were not written, and then we return the total amount of bytes that we were able to send.

So this check is to make sure that written + current_user_data_consumed would be equal to the total amount of bytes we have put in the conn->out stuffer, but current_user_data_consumed tracking what we still need to send.

lrstewart · 2022-06-08T05:46:32Z

tests/unit/s2n_send_test.c

+        /* Subsequent writes should only be decreasing. */
+        s2n_custom_send_fn_called = false;
+        EXPECT_FAILURE_WITH_ERRNO(s2n_send(conn, large_data.data, written - large_data.size, &blocked), S2N_ERR_SEND_SIZE);


I'm not sure what this is testing. Isn't written - large_data.size negative? Or are you relying on it overflowing an unsigned parameter? But then the test name talks about a "smaller buffer".

I'm checking this conditional

s2n-tls/tls/s2n_send.c

Line 136 in ac56bca

S2N_ERROR_IF(conn->current_user_data_consumed > total_size, S2N_ERR_SEND_SIZE);

.

Basically if s2n_send has already sent more than the total size of the send it implies an error in the user logic because the message should've been flushed and conn->current_user_data_consumed should be moved back to 0!

I'll add a comment to clear this up.

I wrote this test this way because I think it will be the most common way a user may trip up on this

lrstewart · 2022-06-08T05:49:58Z

tests/unit/s2n_send_test.c

+
+    /* Until we hit the dynamic record resize threshold we expect that the records are divisible
+     * by the s2n_record_min_write_payload_size. */


Can you expand on this? Why is this the expected behavior, and why is it important that this is true?

Will do 👍

lrstewart · 2022-06-08T05:51:59Z

tests/unit/s2n_send_test.c

+    uint16_t min_payload_size = 0;
+    POSIX_GUARD_RESULT(s2n_record_min_write_payload_size(conn, &min_payload_size));
+    EXPECT_EQUAL(1398, min_payload_size);


Where does "1398" come from?

I'll add a comment to say as much but this is here because this number is the max amount of bytes that can be stuffed into a single ethernet frame.

So when dynamic mode is used it will use this as the max write size until the bytes written threshold is reached.

I have this check here so if for some reason this value changes the test case can be revisited (Will add a comment for this as well)

lrstewart · 2022-06-08T05:57:43Z

tests/unit/s2n_send_test.c

+
+    /* s2n_send partial writes are flushed in proceeding sends */


Nit, I don't think "proceeding" is what you mean here. How about, "the next send", "later sends" or "subsequent sends".

Okay will update this to "subsequent sends"

tests/unit/s2n_send_test.c

lrstewart · 2022-06-08T06:03:17Z

tests/unit/s2n_send_test.c

+
+        /* Assume we have a partial write of sizeof(test_data) - 1 that was successful.
+         * The subsequent socket write failed with EAGAIN, and control has returned to the application.*/


Why can we assume this? Doesn't s2n_partial_socket_send_fn write len-3?

Ah oops, will update this comment. At one time it was at len-1, and I forgot to update this comment when I changed it

lrstewart · 2022-06-08T06:06:46Z

tests/unit/s2n_send_test.c

+        EXPECT_SUCCESS(s2n_connection_set_send_cb(conn, s2n_send_always_passes_fn));
+        EXPECT_SUCCESS(s2n_send(conn, test_data, sizeof(test_data), &blocked));
+        EXPECT_TRUE(s2n_custom_send_fn_called);
+        EXPECT_EQUAL(conn->current_user_data_consumed, 0);
+        EXPECT_EQUAL(blocked, S2N_NOT_BLOCKED);


How does this prove that we flushed the previous partial write?

I assumed EXPECT_EQUAL(conn->current_user_data_consumed, 0); would be enough, but you are right! I will add another check that there is no data available in the out stuffer.

lrstewart · 2022-06-13T17:55:46Z

tests/unit/s2n_send_test.c

+    EXPECT_SUCCESS(s2n_stuffer_wipe(&conn->out));
+    EXPECT_SUCCESS(s2n_stuffer_write_bytes(&conn->out, buf, len));
+


Why is your send callback touching internal connection buffers? A customer's send function couldn't do that, since s2n_connection is opaque.

Wouldn't this modify the behavior of s2n_send in unexpected ways that invalidate the test? Are you sure that you aren't papering over a bug?

Same for s2n_send_mitigates_beast_fn, s2n_dynamic_record_sizing_fn, and s2n_byte_limit_send_fn

Oh I've forgotten why I introduced these. Removing them.

tests/unit/s2n_send_test.c

lrstewart · 2022-06-13T18:04:13Z

tests/unit/s2n_send_test.c

+    /* Allocate 32KB so it takes multiple records to send complete data. */
+    EXPECT_SUCCESS(s2n_alloc(&large_data, 1 << 15));


Don't we have a constant somewhere representing the maximum record size? Could we use that here rather than "<<15"?

This integer is larger than the maximum record size so the message is chunked across multiple records.

I'll move this into a macro

lrstewart · 2022-06-13T18:06:49Z

tests/unit/s2n_send_test.c

+
+    /* s2n_flush will send attempt to send the out stuffer before closing a connection. */


Do you mean "will send" or "will attempt to send"? This is kind of important-- there's a big different between making one best-effort attempt then closing vs blocking until the send completes, and this test doesn't distinguish since your send always succeeds.

Updated to "will send"

lrstewart · 2022-06-13T18:15:11Z

tests/unit/s2n_send_test.c

+    /* s2n_send enforces RE-ENTRENCY. */
+    {


This is actually one of the few s2n_send tests we already have ;) https://github.com/aws/s2n-tls/blob/main/tests/unit/s2n_send_test.c#L71-L92

Ah oops, removed!

tests/unit/s2n_send_test.c

Co-authored-by: Lindsay Stewart <[email protected]>

lrstewart · 2022-08-07T03:37:30Z

#3409

lundinc2 added the s2n-core team label Jun 7, 2022

lundinc2 requested a review from lrstewart June 7, 2022 23:48

lrstewart reviewed Jun 8, 2022

View reviewed changes

lundinc2 force-pushed the s2n-send-testing branch from 32af85a to e6c0825 Compare June 9, 2022 18:19

lrstewart reviewed Jun 13, 2022

View reviewed changes

lundinc2 and others added 12 commits June 14, 2022 08:44

test: Add more unit test coverage to s2n_send.c

4d0f59a

Compiler command difference with CI.

1b1998f

Fix CI build failure.

b072ff3

Fix CI.

a150c8e

Fix CI.

e7e2f5d

Add quic conditional.

ee791b2

Fix CI.

b16e7c5

Apply suggestions from code review

bc3e9d4

Co-authored-by: Lindsay Stewart <[email protected]>

PR feedback.

e781160

Update tests/unit/s2n_send_test.c

25fe92e

Co-authored-by: Lindsay Stewart <[email protected]>

PR feedback.

f80d7b8

Remove record test.

fca30c2

lundinc2 force-pushed the s2n-send-testing branch from a8e1e0f to fca30c2 Compare June 14, 2022 15:44

Fix CI.

9012467

lundinc2 requested review from lrstewart and maddeleine June 15, 2022 17:46

lrstewart closed this Aug 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Add more unit test coverage to s2n_send.c #3347

test: Add more unit test coverage to s2n_send.c #3347

lundinc2 commented Jun 6, 2022

lrstewart Jun 8, 2022

lundinc2 Jun 8, 2022

lrstewart Jun 8, 2022

lundinc2 Jun 8, 2022

lrstewart Jun 8, 2022

lundinc2 Jun 8, 2022

lrstewart Jun 8, 2022

lundinc2 Jun 8, 2022

lrstewart Jun 8, 2022

lundinc2 Jun 8, 2022

lrstewart Jun 8, 2022

lundinc2 Jun 8, 2022

lrstewart Jun 8, 2022

lundinc2 Jun 8, 2022

lrstewart Jun 8, 2022

lundinc2 Jun 8, 2022

lrstewart Jun 13, 2022

lundinc2 Jun 14, 2022

lrstewart Jun 13, 2022

lundinc2 Jun 14, 2022

lrstewart Jun 13, 2022

lundinc2 Jun 14, 2022

lrstewart Jun 13, 2022

lundinc2 Jun 14, 2022

lrstewart commented Aug 7, 2022

		EXPECT_EQUAL(conn->current_user_data_consumed + written, conn->active_application_bytes_consumed);


		/* Until we hit the dynamic record resize threshold we expect that the records are divisible
		* by the s2n_record_min_write_payload_size. */


		/* s2n_send partial writes are flushed in proceeding sends */


		/* Assume we have a partial write of sizeof(test_data) - 1 that was successful.
		* The subsequent socket write failed with EAGAIN, and control has returned to the application.*/

		EXPECT_SUCCESS(s2n_stuffer_wipe(&conn->out));
		EXPECT_SUCCESS(s2n_stuffer_write_bytes(&conn->out, buf, len));

		/* Allocate 32KB so it takes multiple records to send complete data. */
		EXPECT_SUCCESS(s2n_alloc(&large_data, 1 << 15));


		/* s2n_flush will send attempt to send the out stuffer before closing a connection. */

test: Add more unit test coverage to s2n_send.c #3347

test: Add more unit test coverage to s2n_send.c #3347

Conversation

lundinc2 commented Jun 6, 2022

Resolved issues:

Description of changes:

Call-outs:

Testing:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrstewart commented Aug 7, 2022