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

deps: cherry-pick 078e1dc from upstream c-ares #23815

Closed
wants to merge 1 commit into from
Closed

deps: cherry-pick 078e1dc from upstream c-ares #23815

wants to merge 1 commit into from

Conversation

bnoordhuis
Copy link
Member

Original commit message:

Prevent changing name servers while queries are outstanding

Changing name servers doesn't work, per c-ares/c-ares#41.
Better to return an error code than to crash.

Refs: c-ares/c-ares#41
Refs: c-ares/c-ares#191
Refs: #894

@bnoordhuis
deps: cherry-pick 078e1dc from upstream c-ares
836c45a 
Original commit message:

    Prevent changing name servers while queries are outstanding

    Changing name servers doesn't work, per c-ares/c-ares#41.
    Better to return an error code than to crash.

Refs: c-ares/c-ares#41
Refs: c-ares/c-ares#191
Refs: nodejs#894
@tniessen
Copy link
Member

I think we added our own set of patches to avoid this, not sure whether we need to / should change anything in our own implementation. @XadillaX and @addaleax were involved if I recall this correctly.

refack
refack approved these changes Oct 22, 2018
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

RSLGTM

@refack refack added the cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. label Oct 22, 2018
@nodejs-github-bot
Copy link
Collaborator

@bnoordhuis build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1334/pipeline

@addaleax
Copy link
Member

@tniessen 💯 b3678df should have fully addressed this on our side, so I don’t think this cherry-pick changes anything for us.

@bnoordhuis
Copy link
Member Author

I agree, this is just extra hardening.

@addaleax
Copy link
Member

@bnoordhuis I’ve bumped the c-ares mailing list thread about a new release yesterday because there was talk about that 2 weeks ago, and today 1.15.0 was release – if you want, feel free to turn this PR into a full update?

@bnoordhuis
Copy link
Member Author

Yes, I saw. I had planned to pick up this change with 1.14.1 but since that got delayed, I decided to cherry-pick this commit. I'll close this PR and open a new one upgrading to 1.15.0.

(Aside: kinda nice how I got c-ares/c-ares#228 in right before the cut-off.)

@bnoordhuis bnoordhuis closed this Oct 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
cares Issues and PRs related to the c-ares dependency or the cares_wrap binding.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@bnoordhuis @tniessen @nodejs-github-bot @addaleax @refack @jasnell @cjihrig