gazebosim / sdformat

Simulation Description Format (SDFormat) parser and description files.
http://sdformat.org
Apache License 2.0
165 stars 93 forks source link

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

Open aaronchongth opened 3 years ago

aaronchongth commented 3 years ago

Environment

Description

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.

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 commented 3 years ago

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 commented 3 years ago

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.