Fix double-prefixing in log module when using explicit relative path with -d #1591
Conversation
…ive path with -d CDir::CheckPathPrefix is called twice, both in OnLoad, and again in PutLog. Normally the second is a no-op, but if the module path is explicitly relative (e.g. to "."), It will incorrectly add the prefix twice.
Codecov Report
@@ Coverage Diff @@
## master #1591 +/- ##
==========================================
+ Coverage 37.44% 37.48% +0.03%
==========================================
Files 127 127
Lines 31023 31022 -1
Branches 93 93
==========================================
+ Hits 11617 11628 +11
+ Misses 19357 19345 -12
Partials 49 49
Continue to review full report at Codecov.
|
|
What if I'm talking to |
|
ZNC handles Without the patch, sPath on :295 has the value: With the patch, sPath has the values: These are both using the default logpath for user modules, I can patch this too while I'm here. I propose "." and ".." be substituted with "log.dot" and "log.dotdot" if they appear by themselves in a path element.
|
|
If the problem is with relative path in -d, other parts of ZNC can behave
poorly with such path. In that case not log module should be fixed, but -d
flag: it should convert relative path to absolute path upon startup.
2018-08-15 14:55 GMT+01:00 Ryan <notifications@github.com>:
… ZNC handles .. poorly with or without the patch.
Without the patch, sPath on :295 has the value:
./users/WugTest/moddata/log/users/WugTest/moddata/log/2018-08-15.log
With the patch, sPath has the values:
./users/WugTest/moddata/log/TestNetwork/../2018-08-15.log
These are both using the default logpath for user modules,
$NETWORK/$WINDOW/%Y-%m-%d.log.
The only difference between ordinary behavior and this special case is
that CheckPathPrefix elides the .. path element by removing it and the
prior one, it doesn't stop the undesirable filesystem traversal.
I can patch this too while I'm here. I propose "." and ".." be substituted
with "log.dot" and "log.dotdot" if they appear by themselves in a path
element.
- if they aren't by themselves, they'll be left alone
- $WINDOW's replacement contains a literal dot to prevent collision
with nicknames, which never do
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1591 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAT15OjmjmVnHcAnuBCDGKCO8CbjY1uuks5uRChQgaJpZM4VuTkY>
.
|
|
I disagree. If something behaves badly when you give it a relative path, then I think ensuring all paths are absolute just masks possible bugs. I think CDir::CheckPathPrefix is poorly named, poorly documented, and its implementation very inelegant. If a function like that were included now in a pull request, you would surely reject it. I think a much better solution would be to replace it with a new function whose behavior is more strongly defined. |
|
Well, both. -d should be fixed, and CDir::CheckPathPrefix should be fixed.
2018-08-15 15:37 GMT+01:00 Ryan <notifications@github.com>:
I disagree. If something behaves badly when you give it a relative path,
then I think ensuring all paths are absolute just masks possible bugs.
Are you willing to provide test cases for every possible ZNC codepath which
happens to use file paths? Ensuring all paths are absolute is more
self-contained change which fixes not only issue you discovered, but also
future issues with relative paths.
… I think CDir::CheckPathPrefix is poorly named, poorly documented, and its
implementation very inelegant. If a function like that were included now in
a pull request, you would surely reject it. I think a much better solution
would be to replace it with a new function whose behavior is more strongly
defined.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1591 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAT15NIUyHje4k-s7qrGRbdg2bqDzdzHks5uRDIPgaJpZM4VuTkY>
.
|
I've been thinking about it and looking into the impact. This particular issue is pretty self contained because unlike all of the other callsites (of which there are only 5), CheckPathPrefix is called in the following way:
ChangeDir handles two relative paths by appending them (into a new relative path), thus the doubling. A more complete solution would be to change CheckPathPrefix to check the prefix before calling ChangeDir, but then the second CheckPathPrefix call in log.cpp is always a no-op so it should still be removed. |
|
I have a build now that replaces CDir::CheckPathPrefix with 2 new functions, CFile::GetRelativeFile and CFile::Contains, that do the same thing, but generalized to relative paths. CFile::GetRelativeFile in conjunction with another new function CFile::Canonize can also probably replace CDir::ChangeDir. The only thing I'm not sure about is whether to kill them completely, or leave them around but deprecated, since the python and perl bindings use them, and removing them will break any python/perl scripts that depend on them. I'll clean it up and push it to a new branch tonight. |
Any progress? |
CDir::CheckPathPrefix is called twice, both in OnLoad, and again in PutLog. Normally
the second is a no-op, but if the module path is explicitly relative (e.g. to "."),
It will incorrectly add the prefix twice.