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 a multiple-pointers-to-single-loop-variable bug in EgressNetworkPolicy #12045

Merged

Conversation

danwinship
Copy link
Contributor

The EgressNetworkPolicy code was doing the same thing as in https://github.com/openshift/ose/pull/420 where we were trying to store pointers to various objects in an array but ended up storing multiple pointers to the same object instead.

(I guess I was imagining [and maybe others were too since this bug passed code review twice] that since policy was a local variable, but &policy survives after the function returns, then that must mean that & in this context copies the local variable to a new heap object. But in fact, all that happens is that the compiler sees that &policy needs to survive after the function returns, and so allocates policy on the heap rather than the stack to begin with, and so &policy is the same address each time through the loop.)

Fixes #12038.

@openshift/networking PTAL

@smarterclayton
Copy link
Contributor

I assume you're going to open a PR against release-1.4 as well?

Copy link
Contributor

@knobunc knobunc left a comment

Choose a reason for hiding this comment

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

LGTM

@knobunc
Copy link
Contributor

knobunc commented Nov 28, 2016

[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to bbb79e3

Copy link

@pravisankar pravisankar left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11732/) (Base Commit: 9b1d477)

1 similar comment
@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11732/) (Base Commit: 9b1d477)

@knobunc
Copy link
Contributor

knobunc commented Nov 28, 2016

[merge]

@knobunc
Copy link
Contributor

knobunc commented Nov 28, 2016

@danwinship as @smarterclayton suggested (and I am sure you are planning to do), this should go on the 1.4 branch too.

@knobunc
Copy link
Contributor

knobunc commented Nov 29, 2016

[merge] flaked on #11662

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to bbb79e3

@openshift-bot
Copy link
Contributor

openshift-bot commented Nov 29, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11787/) (Base Commit: daef9e7) (Image: devenv-rhel7_5436)

@openshift-bot openshift-bot merged commit 0b1d66c into openshift:master Nov 29, 2016
@danwinship danwinship deleted the egress-network-policy-cache branch November 30, 2016 14:22
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.

5 participants