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 crash with PPP interface with nil dst address #294

Merged
merged 2 commits into from
Dec 21, 2022

Conversation

lmbarros
Copy link
Contributor

@lmbarros lmbarros commented Apr 11, 2022

balenaEngine initialization would fail on devices connected to the
network via PPP (Point-to-Point Protocol) and with a nil destination
address (0.0.0.0):

panic: runtime error: invalid memory address or nil pointer dereference

This commit updates the netlink dependency to a fork where we
cherry-picked the correction. This correction wasn't available in any
stable release of netlink, so we opted to use this fork instead of
relying on a beta netlink version. We can obsolete our fork once Moby
starts using a netlink version that includes the fix.

@lmbarros lmbarros self-assigned this Apr 11, 2022
@ghost
Copy link

ghost commented Apr 11, 2022

Your landr site preview has been successfully deployed to https://landr-balena-os-repo-balena-engine-preview-294.netlify.app

Deployed with Landr 9.4.14

@alexgg
Copy link
Contributor

alexgg commented Dec 13, 2022

@lmbarros could we cherry-pick vishvananda/netlink#665 for the time being?

@lmbarros
Copy link
Contributor Author

@alexgg I see I mentioned cherry picking myself, but that's not easily done. netlink is a dependency, not code we own. (We do keep a vendored copy of netlink within our repo -- changing it directly could technically work, but would be really bad practice and asking for trouble in the future.)

@lmbarros
Copy link
Contributor Author

We would need to fork netlink, cherry pick into our fork and point the dependency to the fork. It's doable, but I don't know... doesn't appear to be much simpler than pointing to the beta netlink and going through a round of Engine tests.

@alexgg
Copy link
Contributor

alexgg commented Dec 14, 2022

hey @lmbarros using a beta version of netlink feels like looking for trouble - if it was production ready it wouldn't be beta. I understand forking a dependency is cumbersome, but once netlink releases a stable version we can just drop it. And having the engine crash when using primary cellular connectivity is something we need to address.
Do you want to take it to an arch call maybe to discuss pros/cons?

@lmbarros
Copy link
Contributor Author

@alexgg , not saying this is the right approach, but in their next-release branch Moby is already using the beta netlink for some time.

@alexgg
Copy link
Contributor

alexgg commented Dec 14, 2022

@alexgg , not saying this is the right approach, but in their next-release branch Moby is already using the beta netlink for some time.

@lmbarros but we also wouldn't use Moby's next release in production, would we?

balenaEngine initialization would fail on devices connected to the
network via PPP (Point-to-Point Protocol) and with a nil destination
address (0.0.0.0):

    panic: runtime error: invalid memory address or nil pointer dereference

This commit updates the netlink dependency to a fork where we
cherry-picked the correction. This correction wasn't available in any
stable release of netlink, so we opted to use this fork instead of
relying on a beta netlink version. We can obsolete our fork once Moby
starts using a netlink version that includes the fix.

Fixes #292

Signed-off-by: Leandro Motta Barros <leandro@balena.io>
Change-type: patch
@lmbarros lmbarros changed the title Update netlink Fix crash with PPP interface with nil dst address Dec 20, 2022
During the migration to Flowzone (the balena CI system) we brought
several changes from Moby's HEAD Dockerfile. Among those, we replaced
the `frozen-images` base image to a debian:bullseye-slim. This was a
mistake: the slim image doesn't contain the `setcap` binary we need by
the `TestBuildUserNamespaceValidateCapabilitiesAreV2` integration test.

So, this PR simply brings back the full-blown `debian:bullseye`.

Signed-off-by: Leandro Motta Barros <leandro@balena.io>
Change-type: patch
@lmbarros lmbarros marked this pull request as ready for review December 20, 2022 21:54
@lmbarros lmbarros requested a review from alexgg December 20, 2022 21:55
@lmbarros
Copy link
Contributor Author

Hey @alexgg ! This is the netlib update (with the fix cherry-picked into a temporary netlib fork, as we agreed).

I slipped in a second commit fixing an integration test failure introduced by the migration to Flowzone.

Copy link
Contributor

@alexgg alexgg left a comment

Choose a reason for hiding this comment

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

lgtm

@lmbarros lmbarros merged commit afda00c into master Dec 21, 2022
@lmbarros lmbarros deleted the lmb/update-netlink branch December 21, 2022 11:56
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.

2 participants