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.

Sign up for GitHub

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

Jump to bottom

DyadicBilinearQuarterDownsampler_sse fails to preserve xmm7, can cause undefined behavior #3585

Closed
randomascii opened this issue Oct 21, 2022 · 0 comments
Closed

DyadicBilinearQuarterDownsampler_sse fails to preserve xmm7, can cause undefined behavior #3585

randomascii opened this issue Oct 21, 2022 · 0 comments

Comments

@randomascii
Copy link
Contributor

The calling convention for 64-bit processes on Windows is documented here:

https://learn.microsoft.com/en-us/cpp/build/x64-calling-convention?view=msvc-170

and it says:

"The x64 ABI considers registers RBX, RBP, RDI, RSI, RSP, R12, R13, R14, R15, and XMM6-XMM15 nonvolatile. They must be saved and restored by a function that uses them."

However DyadicBilinearQuarterDownsampler_sse uses xmm7 but does not preserve and restore it. This can cause arbitrary bugs in calling code that assumes that xmm7 will be preserved. This is a possible cause of crashes and memory corruption in Chrome. See crbug.com/1218384 for a long-running discussion, especially the more recent comments.

This appears to be a serious bug, but the fix should be fairly straightforward.
randomascii added a commit to randomascii/openh264 that referenced this issue Oct 21, 2022
@randomascii
Save xmm7 in DyadicBilinearQuarterDownsampler_sse
6cd6fde 
Static analysis of chrome.dll showed that xmm7 was being used but not
preserved in this function. The Windows calling convention requires
that xmm7 be preserved so this change adds the necessary "PUSH_XMM 8"
and POP_XMM directives to fix this.

This fixes issue cisco#3585. This may fix a bug in Chrome but that is
unknown.
randomascii added a commit to randomascii/openh264 that referenced this issue Oct 21, 2022
@randomascii
Save xmm7 in DyadicBilinearQuarterDownsampler_sse
c659bf8 
Static analysis of chrome.dll showed that xmm7 was being used but not
preserved in this function. The Windows calling convention requires
that xmm7 be preserved so this change adds the necessary "PUSH_XMM 8"
and POP_XMM directives to fix this.

This fixes issue cisco#3585. This may fix a bug in Chrome but that is
unknown.
@randomascii randomascii mentioned this issue Oct 21, 2022
Merged
jrmuizel pushed a commit that referenced this issue Oct 25, 2022
@randomascii @jrmuizel
Save xmm7 in DyadicBilinearQuarterDownsampler_sse
db95667 
Static analysis of chrome.dll showed that xmm7 was being used but not
preserved in this function. The Windows calling convention requires
that xmm7 be preserved so this change adds the necessary "PUSH_XMM 8"
and POP_XMM directives to fix this.

This fixes issue #3585. This may fix a bug in Chrome but that is
unknown.
Yuhengwe1 pushed a commit to Yuhengwe1/openh264 that referenced this issue Feb 20, 2023
@randomascii @Yuhengwe1
Save xmm7 in DyadicBilinearQuarterDownsampler_sse
24a19f8 
Static analysis of chrome.dll showed that xmm7 was being used but not
preserved in this function. The Windows calling convention requires
that xmm7 be preserved so this change adds the necessary "PUSH_XMM 8"
and POP_XMM directives to fix this.

This fixes issue cisco#3585. This may fix a bug in Chrome but that is
unknown.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@jrmuizel @randomascii