Skip to content

[release/6.0] Use "read" to fetch misaligned 64bit values from bundle header - #63519

Merged
jeffschwMSFT merged 3 commits into
release/6.0from
backport/pr-63431-to-release/6.0
Jan 13, 2022
Merged

[release/6.0] Use "read" to fetch misaligned 64bit values from bundle header#63519
jeffschwMSFT merged 3 commits into
release/6.0from
backport/pr-63431-to-release/6.0

Conversation

@github-actions

@github-actionsgithub-actionsBot commented Jan 7, 2022

Copy link
Copy Markdown
Contributor

Backport of #63431 to release/6.0

Fixes: #62273

Customer Impact

The problem was reported by a customer. Single file does not work on ARM32 devices configured to terminate processes on misaligned memory access (happens on some Raspbian systems, unclear how often they're configured in this way).

Testing

Regular test pass.

Manually tested the app built with singlefilehost that contains the fix on Raspbian (bullseye arm32). The app runs regardless of /proc/cpu/alignment settings.

Without the fix, when /proc/cpu/alignment is set to 4 (fault), the app crashes on misaligned reads while processing the bundle headers.

This is a regression introduced when we added support for compression. With compression, we introduced versioned fields into the header, so the header cannot be memcopied all-at-once and the code was changed to read individual fields from memmapped header, which introduced misaligned reads.

Note that setting /proc/cpu/alignment to fault is not typically the default setting. With other settings the kernel can trap the failures and silently handle misaligned reads, so apps run.
However, when configured to fail, no singlefile app would run. In such mode it is a regression from 5.0.

Risk

Low. Instead of direct reads of int64 values form mapped header, we call memcpy, which is alignment-aware.

@ghostghost added the area-Single-File label Jan 7, 2022
@ghost

ghost commented Jan 7, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @agocke, @vitek-karas, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #63431 to release/6.0

/cc @VSadov

Customer Impact

Testing

Risk

Author:github-actions[bot]
Assignees:-
Labels:

area-Single-File

Milestone:-

@janvorlijanvorli left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@VSadovVSadov self-assigned this Jan 7, 2022
@danmoseley

Copy link
Copy Markdown
Contributor

OpenDevices01 fix is just now ported.
Networking test failures on Win 11 has port PR open.

@VSadov

Copy link
Copy Markdown
Member

This change only impacts the singlefilehost binary, so I assume the System.Net.Security.Tests failures in regular tests on win-x64 cannot be from this change.

@danmoseley

Copy link
Copy Markdown
Contributor

Indeed, our comments crossed. I do not know about the ios failures. I can re-run.

@danmoseleydanmoseley reopened this Jan 8, 2022
@danmoseley

Copy link
Copy Markdown
Contributor

Should we consider adjusting one of our CI test configurations to set /proc/cpu/alignment to fault? Perhaps it might catch a bug in future.

@VSadov

Copy link
Copy Markdown
Member

set /proc/cpu/alignment to fault

Seems like a good idea.
Misaligned access is generally unexpected. Even silent handling via kernel emulation is a perf loss, which can add up depending on a scenario.

It would need to be on linux-arm32, I think.

@jeffschwMSFTjeffschwMSFT left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. We should take for consideration in 6.0.x

@danmoseley

Copy link
Copy Markdown
Contributor

set /proc/cpu/alignment to fault

Seems like a good idea. Misaligned access is generally unexpected. Even silent handling via kernel emulation is a perf loss, which can add up depending on a scenario.

It would need to be on linux-arm32, I think.

I'll let you open the issue if you think it's worthwhile, as you have more context.

@leecowleecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jan 11, 2022
@leecowleecow added this to the 6.0.3 milestone Jan 11, 2022
@agocke

Copy link
Copy Markdown
Member

Issue created #63638

@jeffschwMSFT
jeffschwMSFT merged commit fe7335c into release/6.0Jan 13, 2022
@mmitche

Copy link
Copy Markdown
Member

@jeffschwMSFT This missed the check-in window. Is it critical for 6.0.2?

@agocke

Copy link
Copy Markdown
Member

We only have customer who reported this — probably fine to get it in the next patch

@jeffschwMSFT

Copy link
Copy Markdown
Member

I was talking with @safern to coordinate merge.

@safern
safern deleted the backport/pr-63431-to-release/6.0 branch January 13, 2022 19:03
@ghostghost locked as resolved and limited conversation to collaborators Feb 13, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Servicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@danmoseley@VSadov@agocke@mmitche@jeffschwMSFT@janvorli@leecow