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

Basic/WithRebindPaddingArgs.RebindAddrPadded failed with assert #4330

Open
1 of 4 tasks
ami-GS opened this issue May 30, 2024 · 2 comments
Open
1 of 4 tasks

Basic/WithRebindPaddingArgs.RebindAddrPadded failed with assert #4330

ami-GS opened this issue May 30, 2024 · 2 comments
Milestone

Comments

@ami-GS
Copy link
Contributor

ami-GS commented May 30, 2024

Describe the bug

Windows got assert by Basic/WithRebindPaddingArgs.RebindAddrPadded/*

[Microsoft-Quic][ lib] ASSERT, D:\a\msquic\msquic\src\core\send.c:957 - Builder->Metadata->FrameCount > PrevFrameCount || RanOutOfRoom || (CxPlatGetAllocFailDenominator() != 0).

image

Affected OS

  • Windows
  • Linux
  • macOS
  • Other (specify below)

Additional OS information

No response

MsQuic version

main

Steps taken to reproduce bug

needs automation

Expected behavior

pass

Actual outcome

[Microsoft-Quic][ lib] ASSERT, D:\a\msquic\msquic\src\core\send.c:957 - Builder->Metadata->FrameCount > PrevFrameCount || RanOutOfRoom || (CxPlatGetAllocFailDenominator() != 0).

Additional details

By comparing passed case (with Ubuntu),

  • Indicating QUIC_CONNECTION_EVENT_PEER_ADDRESS_CHANGED twice
  • client send a lot of PING Frames

image
These are arrived at once with second QUIC_CONNECTION_EVENT_PEER_ADDRESS_CHANGED indication after the first QUIC_CONNECTION_EVENT_PEER_ADDRESS_CHANGED indication.

This is first indication
image

This is second indication
image

This is the assert after the second indication
[Microsoft-Quic][ lib] ASSERT, D:\a\msquic\msquic\src\core\send.c:957 - Builder->Metadata->FrameCount > PrevFrameCount || RanOutOfRoom || (CxPlatGetAllocFailDenominator() != 0).

@ami-GS
Copy link
Contributor Author

ami-GS commented May 30, 2024

Maybe, the cause of 2 QUIC_CONNECTION_EVENT_PEER_ADDRESS_CHANGED indication is that these PING frames are arrived after unregistering packet hook? so QUIC_CONNECTION_EVENT_PEER_ADDRESS_CHANGED happens to change back to original peer address?

image

Then the second QUIC_CONNECTION_EVENT_PEER_ADDRESS_CHANGED happens with receiving PING frames

Not sure about the ASSERT yet

@nibanks
Copy link
Member

nibanks commented May 30, 2024

This is the assert after the second indication
[Microsoft-Quic][ lib] ASSERT, D:\a\msquic\msquic\src\core\send.c:957 - Builder->Metadata->FrameCount > PrevFrameCount || RanOutOfRoom || (CxPlatGetAllocFailDenominator() != 0).

This assert generally means that for some reason we thought we needed to call QuicSendWriteFrames to put some stuff in a packet, but when we ran through the logic, we ended up not doing anything. This usually indicates a bug in our logic. The complexity comes from the fact that we have related logic in two places:

  1. We have logic that tries to decide if we need to send anything in a particular packet type, in QuicSendFlush.
  2. In QuicSendWriteFrames we have the actual logic to write data to a packet.

Rarely, we have a bug that indicates these two are somehow out of sync. I'm wondering if somehow we need to send a PING frame for one thing, but then path probing kicks in (because there is a UDP port change) and we end up sending the PING in a different code path instead (where we might need to send it for both places?)....

@nibanks nibanks added this to the Future milestone Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants