Fix Namespace loading in PyYAML 5.4.x #6673

chris-boson · 2021-03-25T09:54:33Z

What does this PR do?

This fixes Namespace loading with PyYAML 5.4.x, which was restricted due to a security vulnerability.

Related issues:
#5606
#5619

Before submitting

Was this discussed/approved via a GitHub issue? (not for typos and docs)
Did you read the contributor guideline, Pull Request section?
Did you make sure your PR does only one thing, instead of bundling different changes together?
Did you make sure to update the documentation with your changes? (if necessary)
Did you write any new necessary tests? (not for typos and docs)
Did you verify new and existing tests pass locally with your changes?
Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

Is this pull request ready for review? (if not, please submit in draft mode)
Check that all items from Before submitting are resolved
Make sure the title is self-explanatory and the description concisely explains the PR
Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 🙃

kaushikb11 · 2021-03-25T09:59:59Z

pytorch_lightning/core/saving.py

@@ -350,7 +350,7 @@ def load_hparams_from_yaml(config_yaml: str, use_omegaconf: bool = True) -> Dict
        return {}

    with fs.open(config_yaml, "r") as fp:
-        hparams = yaml.load(fp, Loader=yaml.UnsafeLoader)
+        hparams = yaml.load(fp, Loader=yaml.Loader)


@chris-boson Why replace this?

Actually I think this does just default to UnsafeLoader, but it doesn't seem to cause problems with pyyaml 5.4.x functionality wise.
I think if we want to use SafeLoader we need to add our own Loader and add constructors yaml/pyyaml#482 (comment)

Similar to this https://stackoverflow.com/a/52586430

we are fine with declare own Loader, @chris-boson mind send a suggestion?

Borda · 2021-03-25T10:12:39Z

@chris-boson pls target master as default branch

codecov · 2021-03-25T11:17:10Z

Codecov Report

Merging #6673 (bf2f161) into master (df579a8) will decrease coverage by 4%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #6673    +/-   ##
=======================================
- Coverage      91%     87%    -4%     
=======================================
  Files         200     200            
  Lines       12929   12929            
=======================================
- Hits        11780   11222   -558     
- Misses       1149    1707   +558

chris-boson · 2021-03-25T12:40:43Z

@Borda Are we ok with just removing PyYAML!=5.4.x from requirements and continue using UnsafeLoading? What were the concerns with excluding this version? I think in terms of safety that would be equivalent to using PyYAML<5.4.

Alternatively we need to add our own SafeLoader and constructors #6673 (comment).

omry · 2021-03-25T17:15:21Z

requirements.txt

@@ -4,7 +4,7 @@ numpy>=1.16.6
 torch>=1.4
 future>=0.17.1  # required for builtins in setup.py
 # pyyaml>=3.13
-PyYAML>=5.1, !=5.4.*  # OmegaConf requirement >=5.1
+PyYAML>=5.1  # OmegaConf requirement >=5.1


OmegaConf 2.0 and newer declares it's version dependency on pyyaml so this line should no longer been needed (At some point in the far past it didn't, which was the reason for adding this in the first place).

https://github.com/omry/omegaconf/blob/2.0_branch/requirements/base.txt

Borda · 2021-04-08T08:51:14Z

I think in terms of safety that would be equivalent to using PyYAML<5.4

we do not want any future versions like 6.x or so... so rather ban specific version if needed

Borda · 2021-04-12T07:07:34Z

@chris-boson how is it going here?

chris-boson · 2021-04-12T15:28:25Z

@chris-boson how is it going here?

@Borda I added a yaml loader for Namespace objects, we would need to add constructors for other objects we want to load safely (one of the tests is failing)

carmocca · 2021-04-12T16:08:36Z

So to use SafeLoader we would need to add constructors for every object serialized?

Should we just use the UnsafeLoader then? AFAIK this was the case before, and we can't guarantee what will be saved to hparams

chris-boson · 2021-04-12T16:51:39Z

So to use SafeLoader we would need to add constructors for every object serialized?

Should we just use the UnsafeLoader then? AFAIK this was the case before, and we can't guarantee what will be saved to hparams

Yeah, at least it wouldn't be any worse than it was before. So effectively we'd just take out the PyYAML!=5.4.* from requirements

carmocca · 2021-04-12T17:40:05Z

Exactly, @Borda, do you agree?

Borda · 2021-04-12T18:09:03Z

So effectively we'd just take out the PyYAML!=5.4.* from requirements

but isn't that change in PyYAML for the future, is there an expectation that 5.5 will be fine?

chris-boson · 2021-04-12T19:14:43Z

So effectively we'd just take out the PyYAML!=5.4.* from requirements

but isn't that change in PyYAML for the future, is there an expectation that 5.5 will be fine?

We can put <=5.4.1?

carmocca · 2021-05-03T16:46:14Z

@chris-boson what's blocking this?

Borda · 2021-05-04T21:59:46Z

@chris-boson what's blocking this?

seems like your approval :]

Borda

can we update our YAML usage to support also newer versions?

chris-boson requested review from awaelchli, Borda, carmocca, justusschock, SeanNaren, tchaton and williamFalcon as code owners March 25, 2021 09:54

kaushikb11 reviewed Mar 25, 2021

View reviewed changes

Borda changed the base branch from release/1.2.x to master March 25, 2021 10:12

Borda requested review from ananyahjha93, edenlightning, SkafteNicki and teddykoker as code owners March 25, 2021 10:12

mergify bot added the has conflicts label Mar 25, 2021

chris-boson force-pushed the enhance/pyyaml54 branch from b07843b to 02d2dd9 Compare March 25, 2021 11:04

mergify bot removed the has conflicts label Mar 25, 2021

chris-boson marked this pull request as draft March 25, 2021 11:14

omry reviewed Mar 25, 2021

View reviewed changes

chris-boson force-pushed the enhance/pyyaml54 branch 3 times, most recently from f29491e to 2eab087 Compare April 1, 2021 09:17

Borda added the bug Something isn't working label Apr 8, 2021

Borda added this to the 1.2.x milestone Apr 8, 2021

Borda force-pushed the enhance/pyyaml54 branch from 2eab087 to 8c2833b Compare April 8, 2021 08:53

chris-boson force-pushed the enhance/pyyaml54 branch from 8c2833b to 487b327 Compare April 12, 2021 14:51

chris-boson force-pushed the enhance/pyyaml54 branch from 487b327 to 9a499cc Compare April 13, 2021 17:27

Borda modified the milestones: 1.2.x, 1.3 Apr 18, 2021

chris-boson force-pushed the enhance/pyyaml54 branch from 9a499cc to 7bb5ba2 Compare April 29, 2021 12:19

chris-boson added 3 commits April 29, 2021 14:52

Fix Namespace loading in PyYAML 5.4.x

25afa61

Remove OmegaConf reference from PyYAML requirements

34040f8

Max allowed version for pyyaml

5990971

chris-boson force-pushed the enhance/pyyaml54 branch from 7bb5ba2 to 5990971 Compare April 29, 2021 12:52

ananthsub mentioned this pull request Apr 30, 2021

Allow PyYAML 5.4 #7291

Closed

awaelchli linked an issue May 2, 2021 that may be closed by this pull request

Allow PyYAML 5.4 #7291

Closed

awaelchli approved these changes May 2, 2021

View reviewed changes

Merge branch 'master' into enhance/pyyaml54

1d0922d

awaelchli marked this pull request as ready for review May 4, 2021 10:58

awaelchli added the ready PRs ready to be merged label May 4, 2021

Borda enabled auto-merge (squash) May 4, 2021 21:59

Borda approved these changes May 4, 2021

View reviewed changes

Borda requested review from kaushikb11 and removed request for teddykoker May 4, 2021 22:02

ananthsub approved these changes May 4, 2021

View reviewed changes

Merge branch 'master' into enhance/pyyaml54

bf2f161

Borda merged commit 763a9a9 into Lightning-AI:master May 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Namespace loading in PyYAML 5.4.x #6673

Fix Namespace loading in PyYAML 5.4.x #6673

chris-boson commented Mar 25, 2021 •

edited by Borda

Loading

kaushikb11 Mar 25, 2021

chris-boson Mar 25, 2021

chris-boson Mar 25, 2021

tchaton Apr 8, 2021

Borda Apr 8, 2021

Borda commented Mar 25, 2021

codecov bot commented Mar 25, 2021 •

edited

Loading

chris-boson commented Mar 25, 2021

omry Mar 25, 2021 •

edited

Loading

Borda commented Apr 8, 2021

Borda commented Apr 12, 2021

chris-boson commented Apr 12, 2021

carmocca commented Apr 12, 2021

chris-boson commented Apr 12, 2021

carmocca commented Apr 12, 2021

Borda commented Apr 12, 2021

chris-boson commented Apr 12, 2021

carmocca commented May 3, 2021

Borda commented May 4, 2021 •

edited

Loading

Borda left a comment

Fix Namespace loading in PyYAML 5.4.x #6673

Fix Namespace loading in PyYAML 5.4.x #6673

Conversation

chris-boson commented Mar 25, 2021 • edited by Borda Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

kaushikb11 Mar 25, 2021

Choose a reason for hiding this comment

chris-boson Mar 25, 2021

Choose a reason for hiding this comment

chris-boson Mar 25, 2021

Choose a reason for hiding this comment

tchaton Apr 8, 2021

Choose a reason for hiding this comment

Borda Apr 8, 2021

Choose a reason for hiding this comment

Borda commented Mar 25, 2021

codecov bot commented Mar 25, 2021 • edited Loading

Codecov Report

chris-boson commented Mar 25, 2021

omry Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

Borda commented Apr 8, 2021

Borda commented Apr 12, 2021

chris-boson commented Apr 12, 2021

carmocca commented Apr 12, 2021

chris-boson commented Apr 12, 2021

carmocca commented Apr 12, 2021

Borda commented Apr 12, 2021

chris-boson commented Apr 12, 2021

carmocca commented May 3, 2021

Borda commented May 4, 2021 • edited Loading

Borda left a comment

Choose a reason for hiding this comment

chris-boson commented Mar 25, 2021 •

edited by Borda

Loading

codecov bot commented Mar 25, 2021 •

edited

Loading

omry Mar 25, 2021 •

edited

Loading

Borda commented May 4, 2021 •

edited

Loading