Codecov Report

Merging#1591 intomaster willincrease coverage by0.03%.
The diff coverage isn/a.

@@            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.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last updatee2a9647...6f961ad. Read thecomment docs.

What if I'm talking to.. ?

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

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.

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'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:

sPath = CheckPathPrefix(GetSavePath(), CheckPathPrefix(GetSavePath(), ...))

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.

I'll clean it up and push it to a new branch tonight.

Any progress?