Fix data corruption in delayed mini-conn packets

[sabitov-kirill]

This PR fixes a bug (see #593) where packets delayed by a mini-connection (due to missing keys) retained pointers to volatile memory provided by the caller of lsquic_engine_packet_in. This resulted in data corruption when the caller reused or freed the buffer before the packet was processed by the full connection.

Fix

imico_maybe_delay_processing now calls lsquic_conn_copy_and_release_pi_data. This allocates a persistent buffer for the packet data and sets the PI_OWN_DATA flag, ensuring the data remains valid until the packet is processed or destroyed.

Testing

• Added a new unit test tests/test_mini_conn_delay.c that reproduces the corruption by mocking the decryption delay and verifying the data integrity.
• Verified that lsquic_packet_in_put correctly frees the allocated memory when the packet is eventually processed in lsquic_ietf_full_conn_server_new.

[dtikhonov]

Thank you for the bug report and the patch, @sabitov-kirill! It is surprising that this bug has lingered for so long.

Because memory allocation can fail, this logic should be changed:

Try allocating and copying. If that fails, throw the packet out. If successful, proceed as before.

[dtikhonov]

This function call can fail, need to check return value and take appropriate action.

[sabitov-kirill]

Agreed, my bad. Fixed in e41fc86. However, am I correct that we do not need to bother about free as long as PI_OWN_DATA is raised and reference count is set correctly?

[dtikhonov]

That's right, freeing happens when the reference count goes to zero. In lsquic_engine.c, the reference count is decremented after the "packet in" call:

    conn->cn_if->ci_packet_in(conn, packet_in);                                 
#if LSQUIC_CONN_STATS                                             
    engine->busy.pin_conn = conn;                                           
#endif                                                                   
    QLOG_PACKET_RX(lsquic_conn_log_cid(conn), packet_in, packet_in_data, packet_in_size);
    lsquic_packet_in_put(&engine->pub.enp_mm, packet_in);              // <---------- this decrements refcnt            

If the count goes to zero, this is called:

void                                            
lsquic_mm_put_packet_in (struct lsquic_mm *mm,                 
                                        struct lsquic_packet_in *packet_in)     
{                                                                 
    assert(0 == packet_in->pi_refcnt);                                      
    if (packet_in->pi_flags & PI_OWN_DATA)                               
    {                                                                                    
        lsquic_mm_put_packet_in_buf(mm, packet_in->pi_data, packet_in->pi_data_sz);
    }                                                                         
                                                                              
#if LSQUIC_USE_POOLS                                                     
    TAILQ_INSERT_HEAD(&mm->free_packets_in, packet_in, pi_next);     
#else                                                          
    lsquic_malo_put(packet_in);                                                    
#endif                                                      
}                                                                         

[dtikhonov]

I just looked at the original mini-conn code ("original" as in "Google QUIC") and it does the right thing!

    else if ((packet_in->pi_flags & PI_OWN_DATA) ||                                      
            0 == lsquic_conn_copy_and_release_pi_data(&mc->mc_conn,                
                                                mc->mc_enpub, packet_in))     
    {                                                                         
        assert(packet_in->pi_flags & PI_OWN_DATA);                       
        LSQ_INFO("could not decrypt packet: defer");                 
        mc->mc_deferred_packnos |= MCONN_PACKET_MASK(packet_in->pi_packno);
        MCHIST_APPEND(mc, MCHE_UNDECR_DEFER);                                      
        return PRP_DEFER;                                   
    }                                                                     

Furthermore, it appears that lsquic_conn_copy_and_release_pi_data() was written just for this scenario!

@sabitov-kirill, I am going to ask you to do one more modification to your PR: Check whether PI_OWN_DATA is set, just like it is done in the code above. This will make this code future-proof.

[Copilot]

Pull request overview

This PR fixes a critical data corruption bug in the IETF mini-connection implementation where delayed packets retained pointers to volatile caller-provided memory buffers. The fix ensures packet data is copied to persistent storage before queuing, preventing corruption when the original buffer is reused or freed.

Key Changes:

• Modified imico_maybe_delay_processing to copy packet data before delaying packets, matching the pattern used in the GQUIC mini-connection implementation
• Added a new unit test to verify the fix by simulating decryption delays and validating data integrity

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/liblsquic/lsquic_mini_conn_ietf.c	Added data copying logic to prevent corruption when delaying packets that lack the PI_OWN_DATA flag
tests/test_mini_conn_delay.c	New test that reproduces the bug by mocking decryption delays and verifying packet data integrity after buffer modification
tests/CMakeLists.txt	Added mini_conn_delay to the list of tests to build

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Memory leak: The test creates a mini connection but never destroys it. The conn object and its associated resources (including the delayed packet with copied data) are not cleaned up before the test exits. Consider calling conn->cn_if->ci_destroy(conn) before lsquic_mm_cleanup.

Suggested change

	conn->cn_if->ci_destroy(conn);

[Copilot]

Missing copyright header. Other test files in this repository include a copyright notice at the top (e.g., /* Copyright (c) 2017 - 2022 LiteSpeed Technologies Inc. See LICENSE. */). Consider adding the appropriate copyright header to maintain consistency with the codebase.