Fix what review found in the per-path probe

Six things, from a review of the branch.

QuicPacketBuilderCleanup was called without QuicPacketBuilderFinalize in
two branches, and it opens with CXPLAT_DBG_ASSERT(SendData == NULL). By
then QuicPacketBuilderPrepareForPathMtuDiscovery has succeeded, so
SendData is live: the assert fires in debug and the send data and its
datagram leak in release. The truncation branch is reachable rather than
theoretical -- a peer advertising the RFC-legal minimum
max_udp_payload_size of 1200 takes it on the first probe of every
connection, and since no packet is sent nothing re-arms the probe, so that
path's discovery is dead for the connection's life. Finalize handles a
packet with no frames exactly right, undoing the header and rolling the
packet number back, so it is the one-line fix both branches wanted.

MTU probes stopped being congestion controlled. QUIC_CONN_SEND_FLAG_DPLPMTUD
is not in QUIC_CONN_SEND_FLAGS_BYPASS_CC, so while these were built in the
send loop a full cwnd deferred them. Building them per path skipped that:
only Path->Allowance, the amplification limit, was checked, and a full-size
ack-eliciting probe went out over cwnd and into BytesInFlight. RFC 8899
wants these congestion controlled. QuicPacketBuilderHasAllowance is checked
once the builder is initialized, which is where SendAllowance is set.

A probe skipped for the amplification limit lost its flag while keeping
its per-path bit. Under the old design the flag and the state were the same
thing so nothing was lost; now nothing re-arms it -- no packet was sent, so
loss detection has nothing to discard, and the search-complete timeout only
looks at finished searches. The flag goes back up, as it already does when
the route is not ready.

QUIC_PARAM_CONN_LOCAL_ADDRESS was missing the Paths[0] exemption that
QuicConnActivatePath has and that the documentation states. Paths[0]
reaches that branch not active whenever the peer has sent PATH_BACKUP, so
an application naming its local address was refused while ACTIVATE_PATH for
the same address succeeded.

loss_detection.c dereferenced PathID->Path without a null check on the
lines this branch already touches. A QUIC_PATHID outlives its QUIC_PATH, so
a status frame still in flight when its path is removed reaches here;
connection.c guards its own uses the same way.

docs/Settings.md and the comment above QuicConnPathMeetsRequiredDatagramLength
still said a held-back path is never probed and can never improve. That was
true of P2 and is what P3 removes. An application following the old text
would pick a needlessly small requirement.

Not covered by a test: the truncation branch. The advertised
max_udp_payload_size comes from the socket's local MTU, which is 65536 on
loopback, so no test peer can advertise a value low enough to reach it.

Author: masa-koz

docs/Settings.md

@@ -235,11 +235,11 @@ It exists for applications that promise their own callers a datagram size that n
 
 **A payload length, not an MTU.** The two are not the same question. A path's datagram capacity is derived from its MTU, its address family and its connection ID length, and the same MTU carries twenty fewer bytes over IPv6 than over IPv4. A connection that moves from an IPv4 path to an IPv6 path of identical MTU loses those twenty bytes; comparing MTUs would not notice. The comparison here is against `QuicCalculateDatagramLength` for the path in question, the same arithmetic that produces the `MaxSendLength` reported by `QUIC_CONNECTION_EVENT_DATAGRAM_STATE_CHANGED`, so the natural value to set is the `MaxSendLength` the application last saw.
 
-**Choose it against `MinimumMtu`, not against what the connection currently carries.** A path is created with `Path->Mtu` equal to `MinimumMtu` and, as below, is never probed while it is held back, so a requirement above what `MinimumMtu` yields is one no newly added path can ever satisfy. Setting the requirement to the `MaxSendLength` in hand is therefore only safe while the active path has not grown past `MinimumMtu`; with `MaximumMtu` above it the active path climbs, and a requirement taken from it would keep every subsequent path out of use for the life of the connection.
+**A newly added path starts at `MinimumMtu` and has to be measured up to the requirement.** It is not held at that size: a validated path is probed even while it is held back, so it climbs towards `MaximumMtu` and is admitted once it can carry the required length. What the requirement costs is time, not reachability. A requirement above what `MaximumMtu` yields is the one that can never be satisfied.
 
 Any value is accepted, including such a one. There is nothing useful to bound it against: capacity depends on family and connection ID length as well as MTU, none of them settled when the parameter is usually set. A requirement nothing meets holds every path out of use, which the path statistics make visible.
 
-**The requirement filters; it does not drive.** A path is judged on the MTU it has already measured. Nothing here raises a path's MTU, and msquic does not probe a path it is not sending on — path MTU discovery only ever advances the path `QuicConnChoosePath` returns, which is an active one. A path short of the requirement therefore stays short of it. That is deliberate: an application that finds a candidate wanting is expected to drop it, not to wait for it to improve.
+**The requirement filters; it does not drive.** A path is judged on the MTU it has already measured, and setting a requirement does not ask for any particular size to be reached. It does not stop the path being measured either: a validated path that is being held back is probed on the path itself, with a padded `PATH_CHALLENGE` rather than the usual PING, so it goes on converging while out of the rotation. Nothing announces the moment it becomes wide enough, so an application that wants to know watches `QUIC_PARAM_CONN_PATH_STATISTICS`.
 
 What happens to such a path depends on whether multipath was negotiated.
 

src/core/connection.c

@@ -7633,11 +7633,11 @@ QuicConnAddPath(
 // Whether a path already carries the datagram payload length the application
 // requires of paths it sends on.
 //
-// The path's MTU is used as measured -- nothing here raises it. A path short of
-// the requirement stays short of it: msquic does not probe a path it is not
-// sending on, so the requirement is a filter on what may be used, not a target
-// something climbs towards. An application that finds a candidate wanting is
-// expected to drop it.
+// The path's MTU is used as measured -- nothing here raises it, and setting a
+// requirement does not ask for any size to be reached. It does not stop the
+// path being measured either: QuicSendPathMtuProbes probes a validated path
+// that is being held back, so it goes on converging and is admitted once it
+// can carry the length. The requirement is a filter on what may be used.
 //
 _IRQL_requires_max_(PASSIVE_LEVEL)
 static
@@ -8478,7 +8478,14 @@ QuicConnParamSet(
                     // carries less through an older API is still moving onto
                     // it.
                     //
-                    if (!QuicConnPathMeetsRequiredDatagramLength(Connection, Path)) {
+                    // Paths[0] is exempt, as it is in QuicConnActivatePath: it
+                    // is what QuicConnChoosePath falls back to whether or not
+                    // anything is active, so refusing it achieves nothing. It
+                    // reaches here not being active because a received
+                    // PATH_BACKUP clears IsActive on it like any other path.
+                    //
+                    if (Path != &Connection->Paths[0] &&
+                        !QuicConnPathMeetsRequiredDatagramLength(Connection, Path)) {
                         Status = QUIC_STATUS_INVALID_STATE;
                         break;
                     }

src/core/loss_detection.c

@@ -981,7 +981,13 @@ QuicLossDetectionRetransmitFrames(
                 &FatalError);
             CXPLAT_DBG_ASSERT(!FatalError);
             if (PathID != NULL) {
-                if (!PathID->Path->IsActive &&
+                //
+                // The path can be gone while a frame describing it is still in
+                // flight -- a QUIC_PATHID outlives its QUIC_PATH -- so this is
+                // guarded the way connection.c guards its own uses.
+                //
+                if (PathID->Path != NULL &&
+                    !PathID->Path->IsActive &&
                     Packet->Frames[i].PATH_BACKUP.Sequence + 1 == PathID->StatusSendSeq) {
                     //
                     // The flag on its own is not enough. QuicSendWriteFrames
@@ -1007,7 +1013,13 @@ QuicLossDetectionRetransmitFrames(
                 &FatalError);
             CXPLAT_DBG_ASSERT(!FatalError);
             if (PathID != NULL) {
-                if (PathID->Path->IsActive &&
+                //
+                // The path can be gone while a frame describing it is still in
+                // flight -- a QUIC_PATHID outlives its QUIC_PATH -- so this is
+                // guarded the way connection.c guards its own uses.
+                //
+                if (PathID->Path != NULL &&
+                    PathID->Path->IsActive &&
                     Packet->Frames[i].PATH_AVAILABLE.Sequence + 1 == PathID->StatusSendSeq) {
                     //
                     // The flag on its own is not enough. QuicSendWriteFrames

src/core/send.c

@@ -1542,8 +1542,20 @@ QuicSendPathMtuProbes(
     for (uint8_t i = 0; i < Connection->PathsCount; ++i) {
 
         QUIC_PATH* Path = &Connection->Paths[i];
-        if (!Path->SendMtuProbe ||
-            Path->Allowance < QUIC_MIN_SEND_ALLOWANCE) {
+        if (!Path->SendMtuProbe) {
+            continue;
+        }
+
+        //
+        // Amplification limited. The flag goes back up rather than the path
+        // being dropped: SendMtuProbe stays set either way, and nothing else
+        // re-arms it -- no packet was sent, so loss detection has nothing to
+        // discard, and the search-complete timeout only looks at paths whose
+        // search has finished. Without this the probe waits for some unrelated
+        // path to raise the flag again.
+        //
+        if (Path->Allowance < QUIC_MIN_SEND_ALLOWANCE) {
+            Send->SendFlags |= QUIC_CONN_SEND_FLAG_DPLPMTUD;
             continue;
         }
 
@@ -1573,6 +1585,19 @@ QuicSendPathMtuProbes(
         }
         _Analysis_assume_(Builder.Metadata != NULL);
 
+        //
+        // A probe is a full-size ack-eliciting packet and RFC 8899 has it
+        // congestion controlled like any other. It used to be, for free: the
+        // flag is not in QUIC_CONN_SEND_FLAGS_BYPASS_CC, so the send loop this
+        // was lifted out of dropped it while cwnd was full and picked it up on
+        // the next flush. Sending per path means checking that here instead.
+        // The flag goes back up so that next flush still happens.
+        //
+        if (!QuicPacketBuilderHasAllowance(&Builder)) {
+            Send->SendFlags |= QUIC_CONN_SEND_FLAG_DPLPMTUD;
+            continue;
+        }
+
         if (!QuicPacketBuilderPrepareForPathMtuDiscovery(&Builder)) {
             continue;
         }
@@ -1597,6 +1622,7 @@ QuicSendPathMtuProbes(
                 Intended,
                 (uint16_t)Builder.Datagram->Length);
             Path->SendMtuProbe = FALSE;
+            QuicPacketBuilderFinalize(&Builder, TRUE);
             QuicPacketBuilderCleanup(&Builder);
             continue;
         }
@@ -1637,6 +1663,7 @@ QuicSendPathMtuProbes(
                     // written, so this is not expected to be reachable.
                     //
                     Path->SendMtuProbe = FALSE;
+                    QuicPacketBuilderFinalize(&Builder, TRUE);
                     QuicPacketBuilderCleanup(&Builder);
                     continue;
                 }