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

yaml.load(f) to yaml.load(f, Loader=yaml.FullLoader) #11910

Merged
merged 2 commits into from
Jul 10, 2019
Merged

yaml.load(f) to yaml.load(f, Loader=yaml.FullLoader) #11910

merged 2 commits into from
Jul 10, 2019

Conversation

AlexisTM
Copy link
Contributor

@AlexisTM AlexisTM commented Apr 25, 2019

Describe problem solved by the proposed pull request

The widely used yaml has changed the API and forces to choose a Loader to avoid attacks from a yaml file. This made many application shouting the following:

/home/alexis/Tools/PX4.upstream/msg/tools/uorb_rtps_classifier.py:97: YAMLLoadWarning: calling yaml.load() without Loader=... is deprecated, as the default Loader is unsafe. Please read https://msg.pyyaml.org/load for full details.

Describe your preferred solution

I am proposing to use of FullLoader to keep the Yaml specs functionalities.

Describe possible alternatives

We can choose between the following loaders:

  • BaseLoader: Only loads the most basic YAML
  • SafeLoader: Loads a subset of the YAML language, safely. This is recommended for loading untrusted input.
  • FullLoader: Loads the full YAML language. Avoids arbitrary code execution. This is currently (PyYAML 5.1) the default loader called by yaml.load(input) (after issuing the warning).
  • UnsafeLoader (also called Loader for backwards compatability): The original Loader code that could be easily exploitable by untrusted data input.

@AlexisTM AlexisTM changed the title yaml.load(f) to yaml.load(f, Loader=yaml.SafeLoader) yaml.load(f) to yaml.load(f, Loader=yaml.FullLoader) Apr 25, 2019
@dagar
Copy link
Member

dagar commented Apr 25, 2019

Thanks @AlexisTM. Can you check the Jenkins failures?

@TSC21
Copy link
Member

TSC21 commented Jun 2, 2019

@AlexisTM can you please rebase and check why Jenkins is failing? Thanks

@AlexisTM
Copy link
Contributor Author

AlexisTM commented Jun 3, 2019

Sorry for the delay. Thanks @TSC21 for the reminder.

@AlexisTM
Copy link
Contributor Author

AlexisTM commented Jun 4, 2019

The SITL test failed (performance wise)

@AlexisTM
Copy link
Contributor Author

AlexisTM commented Jun 5, 2019

[worse sentence]: I don't have the error, do we have to enforce the pyyaml version higher for the building process?

+ make validate_module_configs
Traceback (most recent call last):
  File "/tmp/jenkins/workspace/PX4_Firmware_PR-11910/Tools/validate_yaml.py", line 49, in <module>
    schema = load_yaml_file(schema_file)
  File "/tmp/jenkins/workspace/PX4_Firmware_PR-11910/Tools/validate_yaml.py", line 43, in load_yaml_file
    return yaml.load(stream, Loader=yaml.FullLoader)
AttributeError: 'module' object has no attribute 'FullLoader'
Makefile:438: recipe for target 'validate_module_configs' failed
make: *** [validate_module_configs] Error 123
script returned exit code 2

Copy link
Member

@bkueng bkueng left a comment

Choose a reason for hiding this comment

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

CI looks fine (up to an unrelated AppVeyor failure), let's merge.

@bkueng bkueng merged commit 453ecfe into PX4:master Jul 10, 2019
@dagar
Copy link
Member

dagar commented Jul 11, 2019

@AlexisTM @bkueng this is breaking the main Jenkins pipeline when it runs in master.

Screen Shot 2019-07-10 at 8 21 02 PM

@AlexisTM
Copy link
Contributor Author

@dagar I suspect the build machines to use old dependencies where the safe load of pyyaml was not yet implemented.

The other possibility would be to use yaml.safe_load(stream) instead of yaml.load(f, Loader=yaml.FullLoader) which might have been implemented before the overload of the load function.

@bkueng
Copy link
Member

bkueng commented Jul 11, 2019

@AlexisTM exactly, I did it in #12461.

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.

4 participants