Skip to content

gh-88116: Avoid undefined behavior when decoding varints in code objects #94375

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

Merged
merged 2 commits into from
Jun 28, 2022

Conversation

pablogsal
Copy link
Member

@pablogsal pablogsal commented Jun 28, 2022

It seems like we'll fail to correctly decode a varint between INT_MAX
and UINT_MAX or if a signed varint is > INT_MAX / 2 or < INT_MIN / 2 as
this is written today. In fact, it might left shift a 1 into the sign
bit, which would be UB.

Indeed, this test program shows the issue:


int main()
{
    int val = 2147483647;
    unsigned int shift = 1;
    std::cout << (val << shift) << std::endl;
}

As this prints -2.

…e objects

 It seems like we'll fail to correctly decode a varint between INT_MAX
 and UINT_MAX or if a signed varint is > INT_MAX / 2 or < INT_MIN / 2 as
 this is written today. In fact, it might left shift a 1 into the sign
 bit, which would be UB.

 Indeed, this test program shows the issue:

```

int main()
{
    int val = 2147483647;
    unsigned int shift = 1;
    std::cout << (val << shift) << std::endl;
}
```

As this prints -2.
@pablogsal pablogsal requested a review from markshannon as a code owner June 28, 2022 11:40
@pablogsal pablogsal added the 3.11 only security fixes label Jun 28, 2022
@pablogsal pablogsal added the needs backport to 3.11 only security fixes label Jun 28, 2022
Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

Looks good.

In theory, the numbers being encoded should always be less than INT_MAX / 2, but no harm in being on the safe side.

@pablogsal pablogsal merged commit c485ec0 into python:main Jun 28, 2022
@pablogsal pablogsal deleted the gh-88116-2 branch June 28, 2022 13:24
@miss-islington
Copy link
Contributor

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 28, 2022
…e objects (pythonGH-94375)

(cherry picked from commit c485ec0)

Co-authored-by: Pablo Galindo Salgado <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Jun 28, 2022
@bedevere-bot
Copy link

GH-94387 is a backport of this pull request to the 3.11 branch.

miss-islington added a commit that referenced this pull request Jun 28, 2022
…cts (GH-94375)

(cherry picked from commit c485ec0)

Co-authored-by: Pablo Galindo Salgado <[email protected]>
gvanrossum pushed a commit to gvanrossum/cpython that referenced this pull request Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.11 only security fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants