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: peek panics #371

Merged
merged 2 commits into from
May 6, 2024
Merged

Fix: peek panics #371

merged 2 commits into from
May 6, 2024

Conversation

TilakMaddy
Copy link
Collaborator

@TilakMaddy TilakMaddy commented Apr 25, 2024

Sometimes for node properties offset, len can have values like -1

That's why it's important we check the bounds before we return the string

Fix for #369

@TilakMaddy TilakMaddy marked this pull request as ready for review April 25, 2024 15:01
@alexroan
Copy link
Contributor

Waiting for Hackathon submissions to conclude before addressing this PR 👍

Copy link
Contributor

@alexroan alexroan left a comment

Choose a reason for hiding this comment

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

Could you add tests to check for these negative values please @TilakMaddy ?

Any time we fix something that's breaking, we should introduce tests at the same time :)

@TilakMaddy
Copy link
Collaborator Author

TilakMaddy commented May 1, 2024

I made a mistake in PR readme. I don't think it's because of negative values (because we are deserializing it to usize). It has to be something else. Let me re run this without the checks on https://github.com/smartcontractkit/ccip/tree/ccip-develop/contracts to find out what values they were (which causes index out of bounds panic)

@alexroan
Copy link
Contributor

alexroan commented May 6, 2024

@TilakMaddy is this still relevant ?

@TilakMaddy
Copy link
Collaborator Author

@TilakMaddy is this still relevant ?

Yes Alex, we should get this PR in because it's important to check that index lies within bounds before accessing array element at that index

@alexroan alexroan merged commit 499669a into dev May 6, 2024
20 checks passed
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

Successfully merging this pull request may close these issues.

None yet

2 participants