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.

Sign up for GitHub

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

Jump to bottom

compress_path() doesn't normalize some paths correctly #1041

Closed
jsiwek opened this issue Jun 30, 2020 · 3 comments · Fixed by #1050
Closed

compress_path() doesn't normalize some paths correctly #1041

jsiwek opened this issue Jun 30, 2020 · 3 comments · Fixed by #1050
Assignees
@timwoj
Labels
Area: Scripting Type: Bug 🐛 Unexpected behavior or output.
Milestone
3.2.0

Comments

@jsiwek
Copy link
Contributor

jsiwek commented Jun 30, 2020

Probably related to what @dopheide-esnet reported on Slack:

print compress_path("./../foo");
print compress_path("././../foo");

Yields the wrong results:

foo
./foo

We already have path normalization implemented in C++ here:

zeek/src/util.cc

Line 1603 in a5a51de

string normalize_path(std::string_view path)

And think it works correctly in this case, so would suggest not bothering to fix the script-layer version of compress_path(): just change it to a BIF calling normalize_path().
@jsiwek jsiwek added Type: Bug 🐛 Unexpected behavior or output. Area: Scripting labels Jun 30, 2020
@dopheide-esnet
Copy link
Contributor

Here's an example from a cluster:
loaded_scripts.log:
...
/usr/local/zeek/spool/installed-scripts-do-not-touch/site/react/load.bro
/usr/local/zeek/spool/installed-scripts-do-not-touch/site/react/main.bro
/usr/local/zeek/spool/installed-scripts-do-not-touch/site/react/gridftp.bro
/usr/local/zeek/spool/installed-scripts-do-not-touch/site/react/conn-bulk.bro
/usr/local/zeek/spool/installed-scripts-do-not-touch/site/react/conn-bulk.zeek
/usr/local/zeek/spool/installed-scripts-do-not-touch/site/react/ssh.bro
/usr/local/zeek/spool/installed-scripts-do-not-touch/site/react/arista.bro
...
(Note that it does get the ident 'level' correct in this case.)

[zeek@bro-east1 site]$ ls conn-bulk.zeek
conn-bulk.zeek
[zeek@bro-east1 site]$ head -n 2 react/conn-bulk.bro
@load ./main
@load ../conn-bulk.zeek

@rsmmr rsmmr added this to the 3.2.0 milestone Jul 6, 2020
@rsmmr rsmmr assigned rsmmr and timwoj and unassigned rsmmr Jul 6, 2020
timwoj added a commit that referenced this issue Jul 6, 2020
@timwoj
GH-1041: Move compress_path to a bif that uses normalize_path
f2a299d
timwoj added a commit that referenced this issue Jul 6, 2020
@timwoj
GH-1041: Move compress_path to a bif that uses normalize_path
560ee0c
@timwoj
Copy link
Member

timwoj commented Jul 6, 2020

@dopheide-esnet I just added 560ee0c, do you want to see if that fixes your issue?

@timwoj timwoj mentioned this issue Jul 6, 2020
Merged
@dopheide-esnet
Copy link
Contributor

Yeah, initial test looks great.

@jsiwek jsiwek mentioned this issue Jul 7, 2020
Open
jsiwek added a commit that referenced this issue Jul 7, 2020
@jsiwek
Merge remote-tracking branch 'origin/topic/timw/1041-compress-path'
0948361 
* origin/topic/timw/1041-compress-path:
  GH-1041: Move compress_path to a bif that uses normalize_path
0xxon added a commit that referenced this issue Jul 9, 2020
@0xxon
Merge remote-tracking branch 'origin/master' into topic/johanna/table…
e1a45d3 
…-changes

* origin/master: (47 commits)
  scan.l: Remove "constant" did_module_restore logic
  Fix FreeBSD CI script to install right SWIG package
  Update submodule(s)
  GH-928: use realpath() instead of inode to de-duplicate scripts
  Update submodule(s)
  GH-1040: Add zero-indexed version of str_split
  Fix WhileStmt to call Stmt(Tag) ctor
  GH-1041: Move compress_path to a bif that uses normalize_path
  Update submodule(s)
  Update submodule(s)
  Update submodule(s)
  Fix --enable-mobile-ipv6 build
  Fix namespace of GetCurrentLocation() to zeek::detail
  Add backtrace() and print_backtrace()
  Rename BroString files to ZeekString
  Update NEWS entry with note about class renames
  Rename BroObj to Obj
  Rename BroString to zeek::String
  Move Func up to zeek namespace, rename BroFunc to ScriptFunc
  Mark global val_mgr as deprecated and fix uses of it to use namespaced version
  ...
jsiwek pushed a commit that referenced this issue Jul 24, 2020
@timwoj @jsiwek
GH-1041: Move compress_path to a bif that uses normalize_path
9cd03b3 
(cherry picked from commit 560ee0c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@timwoj timwoj

Labels
Area: Scripting Type: Bug 🐛 Unexpected behavior or output.
Projects
None yet
Milestone
3.2.0
Development

Successfully merging a pull request may close this issue.

GH-1041: Move compress_path to a bif that uses normalize_path zeek/zeek
4 participants
@jsiwek @rsmmr @timwoj @dopheide-esnet