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

fix: Avoid invalid access into random region in memory in OperatorUtils #12253

Closed

Conversation

yuandagits
Copy link
Contributor

Summary:
There are 2 bugs in processEncodedFilterResults:

  1. We may call bits::isBitSet(values, index) and then rows.isValid(i), this means we may be passing in an invalid index to bits::isBitSet(values, index). We need to call rows.isValid(i) before bits::isBitSet(values, index). Because index may give a bogus value if it is not valid

  2. We may access an invalid value in index via auto index = indices[i]; if i is not valid.

Fix is to check if the row is valid before processing

Differential Revision: D69095863

Summary:
There are 2 bugs in processEncodedFilterResults:

1. We may call bits::isBitSet(values, index) and then rows.isValid(i), this means we may be passing in an invalid index to `bits::isBitSet(values, index)`. We need to call `rows.isValid(i)` before `bits::isBitSet(values, index)`. Because index may give a bogus value if it is not valid 

2. We may access an invalid value in index via `auto index = indices[i];` if i is not valid.

Fix is to check if the row is valid before processing

Differential Revision: D69095863
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 4, 2025
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D69095863

Copy link

netlify bot commented Feb 4, 2025

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 474b926
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/67a1a92c1149c10008e4db0c

Copy link
Contributor

@spershin spershin left a comment

Choose a reason for hiding this comment

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

Nice catch!

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 48d1bbf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants