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.

Sign up for GitHub

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

Jump to bottom

libcontainer: relax getenv_int sanity check #3489

Merged
merged 1 commit into from
Jun 1, 2022
Merged

libcontainer: relax getenv_int sanity check #3489

merged 1 commit into from
Jun 1, 2022

Conversation

eriksjolund
Copy link
Contributor

Remove upper bound in integer sanity check
to not restrict the number of socket-activated
sockets passed in.

Closes #3488

Signed-off-by: Erik Sjölund erik.sjolund@gmail.com

Is this a correct way to fix the bug? (I am not familiar with runc)

@rhatdan
Copy link
Contributor

rhatdan commented May 26, 2022

LGTM

kolyshkin
kolyshkin previously approved these changes May 27, 2022
View reviewed changes
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

@kolyshkin kolyshkin added the backport/1.1-todo A PR in main branch which needs to be backported to release-1.1 label May 27, 2022
@kolyshkin
Copy link
Contributor

@eriksjolund can you please open a backport of this one to release-1.1 branch? This way we will see the fix released much sooner.

@eriksjolund eriksjolund force-pushed the relax_sanity_check_in_getenv_int branch from e2de218 to 00af87d Compare May 27, 2022 05:34
@eriksjolund eriksjolund mentioned this pull request May 27, 2022
Merged
@eriksjolund
Copy link
Contributor Author

Sure.
I'm not quite sure of which procedure to follow. Here is what I just did.

  1. Rebase this PR to the latest in main and then force-push it.
  2. git switch release-1.1
  3. git checkout -b 1.1-backport-3489
  4. git cherry-pick -x -s 00af87da4fd7debb161ecce0ab662883ee8c3835
  5. git push -u eriksjolund 1.1-backport-3489
  6. Create another PR [1.1] libcontainer: relax getenv_int sanity check #3494

cyphar
cyphar previously approved these changes Jun 1, 2022
View reviewed changes
Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

LGTM. I have no memory of this check. Very strange.

@eriksjolund @cyphar
libcontainer: relax getenv_int sanity check
03a210d 
Remove upper bound in integer sanity check
to not restrict the number of socket-activated
sockets passed in.

Closes opencontainers#3488

Signed-off-by: Erik Sjölund <erik.sjolund@gmail.com>
@cyphar cyphar force-pushed the relax_sanity_check_in_getenv_int branch from 00af87d to 03a210d Compare June 1, 2022 03:54
@kolyshkin kolyshkin added backport/1.0-done A PR in main branch which has been backported to release-1.0 and removed backport/1.1-todo A PR in main branch which needs to be backported to release-1.1 labels Jun 1, 2022
kolyshkin
kolyshkin reviewed Jun 1, 2022
View reviewed changes
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

kolyshkin
kolyshkin approved these changes Jun 1, 2022
View reviewed changes
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

@kolyshkin
Copy link
Contributor

LGTM. I have no memory of this check. Very strange.

That was my silly attempt in trying to unify the code that parses the log level with the code that parses the fd value.

@kolyshkin kolyshkin added this to the 1.2.0 milestone Jun 1, 2022
@kolyshkin kolyshkin requested review from cyphar and thaJeztah June 1, 2022 18:00
thaJeztah
thaJeztah approved these changes Jun 1, 2022
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah merged commit a51ea7c into opencontainers:main Jun 1, 2022
@cyphar
Copy link
Member

cyphar commented Jun 2, 2022

That was my silly attempt in trying to unify the code that parses the log level with the code that parses the fd value.

Ah, fair enough -- socket activation is not something we test for very thoroughly so you can't really be blamed for missing that it would break.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kolyshkin kolyshkin kolyshkin approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@cyphar cyphar Awaiting requested review from cyphar

Assignees
No one assigned
Labels
backport/1.0-done A PR in main branch which has been backported to release-1.0
Projects
None yet
Milestone
1.2.0
Development

Successfully merging this pull request may close these issues.

Socket activation with 3 sockets fails
6 participants
@eriksjolund @rhatdan @kolyshkin @cyphar @thaJeztah @ray372922