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

sdf::findFile does not sanitize input filename, when given a path instead of a file name. #572

Open
aaronchongth opened this issue May 19, 2021 · 2 comments
Labels
bug Something isn't working

Comments

@aaronchongth
Copy link
Collaborator

aaronchongth commented May 19, 2021

Environment

  • OS Version: Ubuntu 20.04
  • Source build, branch sdf11

Description

  • Expected behavior:

When calling sdf::findFile with test_model/model.sdf as the input _filename, instead of a base file name (e.g. model.sdf), it does not throw any exceptions and returns the full path /path/to/test_model/model.sdf.

  • Actual behavior:

In Windows machines however, we will produce a path like so C:/somewhere\\path\\to\\models\\test_model/model.sdf, due to the lack of path sanitization.

Output

This was how it looked like during the Windows CI build for the lines https://github.com/osrf/sdformat/blob/sdf11/test/sdf/includes.sdf#L17-L20,

17: [ RUN      ] ElementTracing.includes
17: C:\Jenkins\workspace\sdformat-ci-pr_any-windows7-amd64\ws\sdformat\test\integration\element_tracing.cc(283): error: Expected equality of these values:
17:   modelFilePath
17:     Which is: "C:/Jenkins/workspace/sdformat-ci-pr_any-windows7-amd64/ws/sdformat\\test\\integration\\model\\test_model\\model.sdf"
17:   overrideModelWithFileElem->FilePath()
17:     Which is: "C:/Jenkins/workspace/sdformat-ci-pr_any-windows7-amd64/ws/sdformat\\test\\integration\\model\\test_model/model.sdf"
17: [  FAILED  ] ElementTracing.includes (201 ms)
@azeey
Copy link
Collaborator

azeey commented May 20, 2021

I thought the issue was actually in sdf::findFile when it's used to find the included file: https://github.com/osrf/sdformat/blob/e5ca000c1934b59302362ef85016ae20af501ed0/src/parser.cc#L1103-L1106

In your example, doesn't sdf::Element::FilePath() return C:/somewhere\\path\\to\\models\\test_model/model.sdf?

@aaronchongth
Copy link
Collaborator Author

Yup, it did. Sorry I got confused myself for a moment. I just did some basic tests, and it is as you mentioned, it was sdf::findFile in SDFImpl.hh, https://github.com/osrf/sdformat/blob/sdf11/include/sdf/SDFImpl.hh#L82-L99 that does not sanitize the path.

I will make the changes to the issue accordingly, sorry for the confusion.

@aaronchongth aaronchongth changed the title TestFile and SourceFile in test_config.hh appends paths without sanitization sdf::findFile does not sanitize input filename, when given a path instead of a file name. May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants