GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — deps: update chromium zlib 1.2.12 by fabiomcarneiro · Pull Request #42571 · nodejs/node

Conversation

@fabiomcarneiro

@fabiomcarneiro fabiomcarneiro commented Apr 1, 2022

Copy link
Copy Markdown

Updates chromium zlib to 1.2.12
kept zlib.gyp build file.

https://chromium.googlesource.com/chromium/src/third_party/zlib/+/faff052b6b6edcd6dd548513fe44ac0941427bf0

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lin72h and bricss reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@fabiomcarneiro
deps: update chromium zlib 1.2.12
c0a30fc
@nodejs-github-bot nodejs-github-bot added dependencies PRs that add, update, or configure Node.js dependencies. needs-ci PRs that need a full CI run. zlib Issues and PRs related to the zlib module and its compression dependencies. labels Apr 1, 2022
@fabiomcarneiro
fabiomcarneiro marked this pull request as draft April 1, 2022 18:02
mscdex
mscdex reviewed Apr 1, 2022
View reviewed changes
Comment thread deps/zlib/zconf.h
/* This include does prefixing as below, but with an updated set of names. Also
* sets up export macros in component builds. */
//#include "chromeconf.h"
#include "chromeconf.h"

@mscdex mscdex Apr 1, 2022

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This change still needs to be backed out IIRC. Either that, or you might try adding defining CHROMIUM_ZLIB_NO_CHROMECONF in the gyp file to see if that works. I commented this out in my original PR that added the Chromium zlib implementation because it was breaking zlib tests.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mscdex
mscdex reviewed Apr 1, 2022
View reviewed changes
Comment thread deps/zlib/zlib.gyp
}],
],
}
} No newline at end of file

@mscdex mscdex Apr 1, 2022

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Unnecessary change

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Trott Trott Apr 2, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'm OK with stripping out trailing spaces in this PR rather than leaving them or doing it in a separate PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mscdex

mscdex commented Apr 1, 2022

Copy link
Copy Markdown
Contributor

By the looks of it, the gyp will probably need to be updated to match the changes made to the build.gn file, especially for any optimization-related changes.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos

targos commented Apr 4, 2022

Copy link
Copy Markdown
Member

Feel free to look at what I did in https://github.com/targos/node/tree/update-zlib-2 to fix the gyp config.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Ilmarinen100

Ilmarinen100 commented Apr 5, 2022

Copy link
Copy Markdown

am I correct to assume nodejs is vulnerable to CVE-2018-25032 until this is integrated/released?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mscdex

mscdex commented Apr 5, 2022

Copy link
Copy Markdown
Contributor

@Ilmarinen100 #31201 (comment)
Ilmarinen100 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@LearnerOrLearnerr

LearnerOrLearnerr commented Apr 20, 2022

Copy link
Copy Markdown

What would be the plan to close CVE-2018-25032 for the latest node LTS versions so that security scanning tools stop reporting the zlib implementation in node as vulnerable? I understand from the linked comments in #31201 that Chromium zlib which is being used is not vulnerable, but how to update nodejs such that this manual review of the situation and analysis is not required?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nschonni nschonni mentioned this pull request Apr 20, 2022
Fix for CVE-2018-25032: Update the zlib version bundled with Node.js nodejs/security-wg#792
Closed
@targos targos mentioned this pull request Aug 17, 2022
Is maintaining-zlib document up-to-date? #44254
Closed
@Neustradamus

Neustradamus commented Oct 15, 2022

Copy link
Copy Markdown

Note there is now a 1.2.13 with CVE-2022-37434 fix.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott
Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49
@lpinca

lpinca commented Nov 17, 2022

Copy link
Copy Markdown
Member

Superseded by #45387.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lpinca lpinca closed this Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Trott Trott Trott left review comments
+1 more reviewer
@mscdex mscdex mscdex left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

dependencies PRs that add, update, or configure Node.js dependencies. needs-ci PRs that need a full CI run. zlib Issues and PRs related to the zlib module and its compression dependencies.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@fabiomcarneiro @mscdex @targos @Ilmarinen100 @LearnerOrLearnerr @Neustradamus @lpinca @Trott @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.