NULL pointer deference causing Picoquic server to crash

[QUICTester]

Hi, when testing commit d2f01093, we discover NULL pointer deference that causes a crash on the Picoquic server.

Sending the following frame sequence as a client will crash the server: 1) Ping frame 2) Crypto frame containing Client Hello message 3) ACK frame without padding that acknowledges the Initial packet (Server Hello message) from the server 3) ACK frame that acknowledges the Handshake packet (Certificate, Certificate Verify, Finished messages) from the server

Root cause: Picoquic does not correctly handles the Initial retransmission queue. Picoquic’s method of emptying the Initial retransmission queue in picoquic_implicit_handshake_ack() does not follow the order in which the packets were added. Some packets will remain in the retransmission queue after picoquic_implicit_handshake_ack() is called. This will trigger a retransmission callback even when the Initial key was discarded, causing NULL pointer deference when the server tries to encrypt the Initial packet for retransmission.

In picoquic/sender.c:picoquic_implicit_handshake_ack(), p_next should be p->next_packet instead of p->previous_packet.

[huitema]

I think something similar was fixed recently. Does the crash repeat on the latest version of picoquic?

[huitema]

Clarification question about the exact test sequence. I understand it as:

1. Test Client sends an Initial packet containing a Ping frame, padded to required length
2. Test Client sends an Initial packet with a Crypto frame containing the Client Hello message
3. Test Client waits for Initial packet from server containing the Server Hello message
4. Test Client sends an Initial packet containing an ACK frame without padding that acknowledges the Initial packet (Server Hello message) from the server
5. Test Client sends an Handshake packet containing an ACK frame that acknowledges the Handshake packet (Certificate, Certificate Verify, Finished messages) from the server

Is this correct?

[huitema]

Further analysis:

• using p_previous_packet in picoquic_implicit_handshake_ack is actually correct. Quirky, but verified.
• receiving an Initial ACK of the server hello is already tested in the interop runner -- multiple implementations do that.
• the server deletes the Initial context after receiving a valid handshake packet from the client, and deletes the Handshake context when the connection moves to the ready stage.

This suggests that there is some kind of race condition happening. Can you clarify the timing of the test packets relative to server states?

[huitema]

Here is the qlog visualization of my trial at reproducing the bug:

Start with an initial ping, then the client sends the client hello and waits for the server hello. That server hello arrives at T=12.00. (The graph says 1.0, but there is a transmission delay in the simulation.) The client cannot send handshake packets immediately, but by the time they arrive the sender has finished sending a whole flight. The additional packets are processed without a glitch.

[QUICTester]

Yes, your understanding of the test sequence is correct.

This has been solved in the latest commit. Sorry, I made a mistake. The correct fix should be p->previous_packet. The previous code with p->next will only empty the first packet in the retransmission queue, causing a retransmission callback after the implicit handshake ack (if there are more than two packets in the retransmission queue) and leading to NULL pointer deference when trying to get the discarded Initial encryption key.

Yes, there is a rare race condition in the server; if the time interval between the Ping frame (first message) and the Crypto frame carrying the Client Hello message is long enough (we set it to ~0.3s seconds during testing), the crash will not happen. Otherwise, the crash happens when we set the time interval to ~0.1s on our machine.

[huitema]

Resolved as no repro.

[huitema]

PR #1529 verifies that the issue was fixed. It also renames the indices in the queue of packets pending ACK or retransmission, because the root cause of the previous issue was largely due to name confusion between "newest", "oldest", "next" and "previous".