-
Notifications
You must be signed in to change notification settings - Fork 700
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
Escape unix socket group in unit tests #1554
Conversation
Signed-off-by: secwall <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #1554 +/- ##
============================================
- Coverage 70.94% 70.78% -0.16%
============================================
Files 120 120
Lines 65044 65044
============================================
- Hits 46143 46039 -104
- Misses 18901 19005 +104 |
Agree, don't think we need to backport it. |
@@ -568,7 +568,8 @@ if {$::verbose} { | |||
set tempFileId [open $tempFileName w] | |||
set group [dict get [file attributes $tempFileName] -group] | |||
if {$group != ""} { | |||
start_server [list tags {"repl external:skip"} overrides [list unixsocketgroup $group unixsocketperm 744]] { | |||
set escaped_group "\"[string map {"\\" "\\\\"} $group]\"" | |||
start_server [list tags {"repl external:skip"} overrides [list unixsocketgroup $escaped_group unixsocketperm 744]] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be more correct to do the escaping inside start_server
so it works everywhere and for all options?
(proc start_server
is defined in tests/support/server.tcl.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably yes. Do we have a list of all string options that we should escape?
Could we try to escape any string option if we don't have such list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. :) I think some investigation is needed to find out the best solution here. Now it's already merged so let's not worry too much about it.
In some cases unix groups could have whitespace and/or
\
in them.One example is my workstation. It's a MacOS in an Active Directory domain. So my user has group
LD\Domain Users
.Running
make test
onunstable
and8.0
branches fails with:I'm not sure if we need to fix this in 8.0. But it seems that it should be fixed in unstable.