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

Fix a unicode parsing error #4310

Merged
merged 2 commits into from Apr 25, 2018
Merged

Fix a unicode parsing error #4310

merged 2 commits into from Apr 25, 2018

Conversation

obelisk
Copy link
Contributor

@obelisk obelisk commented Apr 21, 2018

There was an error where having a query with a path like C:\users, osquery thought that was a unicode character sers after \u.

This adds a check for double backslashes and skips them if found.

What would happen before with the new testcase:

➜  osquery git:(master) ✗ ./build/darwin/osquery/osquery_tests --gtest_filter="ConversionsTests.test_unicode_unescape"
Note: Google Test filter = ConversionsTests.test_unicode_unescape
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from ConversionsTests
[ RUN      ] ConversionsTests.test_unicode_unescape
WARNING: Logging before InitGoogleLogging() is written to STDERR
W0421 11:44:52.044927 2431210368 conversions.h:212] Unescaping a string with length: 28 failed at: 3
/Users/obelisk/Documents/git_repos/osquery/osquery/core/tests/conversions_tests.cpp:74: Failure
      Expected: unescapeUnicode(test.first)
      Which is: ""
To be equal to: test.second
      Which is: "c:\\\\users\\\\obelisk\\\\file.txt"
[  FAILED  ] ConversionsTests.test_unicode_unescape (1 ms)
[----------] 1 test from ConversionsTests (1 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (1 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] ConversionsTests.test_unicode_unescape

 1 FAILED TEST

@obelisk obelisk added this to the 3.3.0 milestone Apr 21, 2018
@obelisk obelisk self-assigned this Apr 21, 2018
@obelisk obelisk requested a review from muffins April 21, 2018 18:50
@facebook-github-bot facebook-github-bot added the cla signed Automated label: Pull Request author has signed the osquery CLA label Apr 21, 2018
@facebook-github-bot
Copy link

@obelisk has updated the pull request.

@facebook-github-bot
Copy link

@obelisk has updated the pull request.

@osqueryer
Copy link

👎 The commit 48c6055 (Job results: 1348) failed one or more tests (FreeBSD).

@osqueryer
Copy link

🙅 The commit 87b3402 (Job results: 3393) failed the code audit or documentation test.

@osqueryer
Copy link

👎 The commit 87b3402 (Job results: 6906) failed one or more tests (Linux).

@osqueryer
Copy link

👎 The commit 87b3402 (Job results: 3796) failed one or more tests (Windows).

@osqueryer
Copy link

👎 The commit 87b3402 (Job results: 3797) failed one or more tests (Windows).

@osqueryer
Copy link

👎 The commit 87b3402 (Job results: 6907) failed one or more tests (Linux).

@facebook-github-bot
Copy link

@obelisk has updated the pull request. View: changes

@osqueryer
Copy link

👎 The commit cca927e (Job results: 6910) failed one or more tests (Linux).

@osqueryer
Copy link

👎 The commit cca927e (Job results: 6911) failed one or more tests (Linux).

}
return Status();
return Status{0};
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: why the initialization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason

Copy link
Contributor

Choose a reason for hiding this comment

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

Mind changing it back for consistency with the code base? :)

Copy link
Contributor

@muffins muffins left a comment

Choose a reason for hiding this comment

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

LGTM, just one nit.

@facebook-github-bot
Copy link

@obelisk has updated the pull request.

@osqueryer
Copy link

👎 The commit f375deb (Job results: 6944) failed one or more tests (Linux).

@osqueryer
Copy link

👎 The commit f375deb (Job results: 6945) failed one or more tests (Linux).

@obelisk obelisk merged commit c646139 into osquery:master Apr 25, 2018
trizt pushed a commit to trizt/osquery that referenced this pull request May 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cla signed Automated label: Pull Request author has signed the osquery CLA configuration distributed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants