Skip to content

apple dns: fix crash on invalid dns name#16028

Merged
htuch merged 6 commits intomainfrom
fix-dns-issue
Apr 26, 2021
Merged

apple dns: fix crash on invalid dns name#16028
htuch merged 6 commits intomainfrom
fix-dns-issue

Conversation

@junr03
Copy link
Member

@junr03 junr03 commented Apr 16, 2021

Commit Message: apple dns: fix crash on invalid dns name.
Additional Description: The release assert in question was ahead of verifying the return code.
Risk Level: low - most (possibly all) envoy production usage does not happen on apple hardware.
Testing: added new test that repro.

Fixes #16021

Signed-off-by: Jose Nino jnino@lyft.com

Jose Nino added 2 commits April 15, 2021 21:26
Signed-off-by: Jose Nino <jnino@lyft.com>
fmt
Signed-off-by: Jose Nino <jnino@lyft.com>
@junr03 junr03 requested a review from htuch April 16, 2021 13:45
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!
/wait

// This callback should never be executed.
FAIL();
}));

Copy link
Member

Choose a reason for hiding this comment

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

?

return;
}

RELEASE_ASSERT(interface_index == 0,
Copy link
Member

Choose a reason for hiding this comment

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

Should this be ENVOY_BUG? I.e. it's fully recoverable.

Jose Nino added 2 commits April 20, 2021 12:36
Signed-off-by: Jose Nino <jnino@lyft.com>
fmt
Signed-off-by: Jose Nino <jnino@lyft.com>
Jose Nino added 2 commits April 21, 2021 14:10
fmt
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
@junr03
Copy link
Member Author

junr03 commented Apr 26, 2021

@htuch this is ready.

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@htuch htuch merged commit f40e279 into main Apr 26, 2021
@htuch htuch deleted the fix-dns-issue branch April 26, 2021 23:54
@travisgroth
Copy link

Hi - will this make it into a patch for 1.17? If not, what release will it be included in? Thanks!

@junr03
Copy link
Member Author

junr03 commented May 3, 2021

No, we only have security patches with designated CVEs. This will make it into the next point release: 1.19

@travisgroth
Copy link

It looks like this should qualify for backporting https://github.com/envoyproxy/envoy/blob/main/RELEASES.md#stable-releases

Stability fixes backported from the main branch (anything that can result in a crash, including crashes triggered by a trusted control plane).

Justification:

  • crash on a maintained release (EOL next year)
  • easy to trigger in routine usage
  • small change

gokulnair pushed a commit to gokulnair/envoy that referenced this pull request May 6, 2021
The release assert in question was ahead of verifying the return code.

Risk Level: low - most (possibly all) envoy production usage does not happen on apple hardware.
Testing: added new test that repro.

Fixes envoyproxy#16021
Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Gokul Nair <gnair@twitter.com>
@travisgroth
Copy link

/backport

@repokitteh-read-only repokitteh-read-only bot added the backport/review Request to backport to stable releases label May 12, 2021
@travisgroth
Copy link

/cc @PiotrSikora can I get this reviewed for backport to maintained releases? I believe 1.17+.

@phlax
Copy link
Member

phlax commented May 25, 2021

cc @dmitri-d

mattklein123 pushed a commit that referenced this pull request Jun 4, 2021
Commit Message: apple dns - fix interface issue with localhost lookup
Additional Description: deleting ENVOY_BUG statement, as localhost dns resolution renders a valid non-zero interface index.
Risk Level: low
Testing: fixed previously existing test that did not have a run block.

Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
@alyssawilk alyssawilk added backport/approved Approved backports to stable releases and removed backport/review Request to backport to stable releases labels Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/approved Approved backports to stable releases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Crash on macOS with invalid DNS name

5 participants