Rework MODE/RPL_CHANMODEIS handling for trailing args #1661

linuxdaemon · 2019-05-22T20:31:03Z

Some servers may send a colon even if the last parameter doesn't need
it, currently this leads to issues with permission/mode tracking, as the
core doesn't handle the colon properly.

This fix replaces reconstructing the parameter string with just passing a vector of the relevant parameters to CChan::SetModes() and adds overrides for CChan::SetModes() and CChan::ModeChange() that accept the vector instead.

Fixes #1637

H7-25 · 2019-05-23T16:06:08Z

There's always some problem :
with znc: #asd has modes: +:nt
with adirc directly : #asd has modes: +nt

and also :

IdleKick sets mode: +v :BrAvE`OuT

ChanServ sets mode: +qo BrAvEOuT :BrAvEOuT

linuxdaemon · 2019-05-23T18:19:43Z

@H7-25 Is this with my changes or without?

H7-25 · 2019-05-23T19:05:41Z

@linuxdaemon sorry i'm wrong, i hadn't chekout the right branch, i've try just now but i've an error compiling it :

[ 3%] Building CXX object src/CMakeFiles/znclib.dir/Chan.cpp.o
/home/network/znc-patched/src/Chan.cpp: In member function ‘void CChan::ModeChange(const CString&, const CNick*)’:
/home/network/znc-patched/src/Chan.cpp:438:5: error: expected ‘,’ or ‘;’ before ‘vsModes’
vsModes.push_back(sModeArg);
^
src/CMakeFiles/znclib.dir/build.make:211: set di istruzioni per l'obiettivo "src/CMakeFiles/znclib.dir/Chan.cpp.o" non riuscito
make[2]: *** [src/CMakeFiles/znclib.dir/Chan.cpp.o] Errore 1
CMakeFiles/Makefile2:369: set di istruzioni per l'obiettivo "src/CMakeFiles/znclib.dir/all" non riuscito
make[1]: *** [src/CMakeFiles/znclib.dir/all] Errore 2
Makefile:116: set di istruzioni per l'obiettivo "all" non riuscito
make: *** [all] Errore 2

H7-25 · 2019-05-23T19:35:29Z

@linuxdaemon it crash a first client connection attempt

linuxdaemon · 2019-05-23T21:20:37Z

@H7-25 should be fixed

H7-25 · 2019-05-23T21:22:43Z

@linuxdaemon look work fine, i'm using it just now for my network staff i let you know if i find issue

codecov · 2019-05-23T21:41:14Z

Codecov Report

Merging #1661 into master will increase coverage by 0.24%.
The diff coverage is 79.16%.

@@            Coverage Diff             @@
##           master    #1661      +/-   ##
==========================================
+ Coverage   38.31%   38.56%   +0.24%     
==========================================
  Files         125      125              
  Lines       30148    30189      +41     
  Branches       93       93              
==========================================
+ Hits        11551    11641      +90     
+ Misses      18548    18499      -49     
  Partials       49       49

Impacted Files	Coverage Δ
include/znc/Chan.h	`49.05% <ø> (+1.88%)`	⬆️
src/Message.cpp	`95.65% <100%> (+0.27%)`	⬆️
include/znc/Message.h	`82.14% <100%> (+0.43%)`	⬆️
src/IRCSock.cpp	`63.62% <50%> (+0.57%)`	⬆️
src/Chan.cpp	`60.16% <75.86%> (+5.54%)`	⬆️
src/FileUtils.cpp	`49.39% <0%> (-0.49%)`	⬇️
src/ZNCString.cpp	`78.2% <0%> (+0.19%)`	⬆️
src/Modules.cpp	`60.17% <0%> (+0.8%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d17cdc7...7c20839. Read the comment docs.

codecov · 2019-05-23T21:41:14Z

Codecov Report

Merging #1661 into master will increase coverage by 0.01%.
The diff coverage is 44.44%.

@@            Coverage Diff             @@
##           master    #1661      +/-   ##
==========================================
+ Coverage   37.23%   37.24%   +0.01%     
==========================================
  Files         127      127              
  Lines       31052    31084      +32     
  Branches       93       93              
==========================================
+ Hits        11561    11577      +16     
- Misses      19442    19458      +16     
  Partials       49       49

Impacted Files	Coverage Δ
include/znc/Chan.h	`47.16% <ø> (ø)`	⬆️
include/znc/Message.h	`81.92% <100%> (+0.22%)`	⬆️
src/Chan.cpp	`51.68% <24.13%> (-2.94%)`	⬇️
src/IRCSock.cpp	`62.95% <50%> (ø)`	⬆️
src/Message.cpp	`95.1% <90.9%> (-0.27%)`	⬇️
src/HTTPSock.cpp	`41.73% <0%> (-0.81%)`	⬇️
src/FileUtils.cpp	`49.39% <0%> (-0.49%)`	⬇️
include/znc/Csocket.h	`50.33% <0%> (+0.67%)`	⬆️
src/Utils.cpp	`67.9% <0%> (+2.19%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7c5eec...288380e. Read the comment docs.

DarthGandalf · 2019-05-29T07:30:19Z

This should go to master, not to 1.7.x. It's too big change to be considered there

DarthGandalf · 2019-05-29T07:35:15Z

include/znc/Chan.h

@@ -77,7 +77,9 @@ class CChan : private CCoreTranslationMixin {

    // Modes
    void SetModes(const CString& s);
+    void SetModes(const VCString& vsModes);


Signature of this method indicates that every mode is in a separate string. How are parameters passed? e.g. which nick got +o and which mask got +b?

This is a vector of {"+oosvb", "Op1", "Op2", "VoicedUser", "BannedUser!*@*"}, acting the same as SetModes(const CString&) but taking the arguments directly from the message instead of concating them and re-splitting them inside of SetModes as is currently done.

Could you add a comment explaining that?

Also, since the first element is special, probably it belongs to a separate parameter

DarthGandalf · 2019-05-29T07:37:12Z

Any tests?

include/znc/Message.h

DarthGandalf · 2019-05-29T07:42:35Z

src/Chan.cpp

-    CString sModeArg = sModes.Token(0);
-    CString sArgs = sModes.Token(1, true);
+void CChan::ModeChange(const VCString& vsModes, const CNick* pOpNick) {
+    CString sModeArg = vsModes.at(0);


Can vsModes be empty?

H7-25 · 2019-06-10T08:49:55Z

I'm using this patch for my network staff. I confirm that there's always some problem with mode parsing,
some examples :

· ChanServ sets mode: +qo Simos :Simos
· IdleKick sets mode: +v :Horus
· Simos sets mode: +b :tester!@

This using Adiirc + ZNC (with @linuxdaemon patch)

After a while (it can run for same days) ZNC crash, i've to enable core file generation but i've not found how.

DarthGandalf · 2019-06-10T08:59:51Z

@H7-25 https://wiki.znc.in/Debugging

H7-25 · 2019-06-10T11:05:20Z

Debug enabled i'll share gdb back trace asap.

linuxdaemon · 2019-06-12T15:58:44Z

Any tests?

Not currently, I will work on those when I can get back around to this

Some servers may send a colon even if the last parameter doesn't need it, currently this leads to issues with permission/mode tracking, as the core doesn't handle the colon properly. This fix replaces reconstructing the parameter string with just passing a vector of the relevant parameters to CChan::SetModes() and adds overrides for CChan::SetModes() and CChan::ModeChange() that accept the vector instead.

The mode character string will be passed as the first argument, while the mode parameters are sent as a VCString as the second parameter

H7-25 · 2019-06-12T20:42:58Z

@linuxdaemon , can't compile :

[ 0%] Built target copy_csocket_h
Scanning dependencies of target modpython_swig
[ 0%] Generating modpython_biglib.cpp, znc_core.py
/home/network/znc-patched/include/znc/Config.h:0: Warning 362: operator= ignored
/home/network/znc-patched/include/znc/Message.h:EOF: Error: Syntax error in input(3).
modules/modpython/CMakeFiles/modpython_swig.dir/build.make:62: set di istruzioni per l'obiettivo "modules/modpython/modpython_biglib.cpp" non riuscito
make[2]: *** [modules/modpython/modpython_biglib.cpp] Errore 1
CMakeFiles/Makefile2:3584: set di istruzioni per l'obiettivo "modules/modpython/CMakeFiles/modpython_swig.dir/all" non riuscito
make[1]: *** [modules/modpython/CMakeFiles/modpython_swig.dir/all] Errore 2
Makefile:116: set di istruzioni per l'obiettivo "all" non riuscito
make: *** [all] Errore 2

linuxdaemon · 2019-06-12T21:53:27Z

@H7-25 Please can you set your locale to C or en_*, build again, and include the English log if it errors again? Sorry, I can't read Italian.

H7-25 · 2019-06-12T21:58:20Z

@linuxdaemon sorry i was not thinking to locale

[ 0%] Built target copy_csocket_h
[ 0%] Generating modpython_biglib.cpp, znc_core.py
/home/network/znc-patched/include/znc/Config.h:0: Warning 362: operator= ignored
/home/network/znc-patched/include/znc/Message.h:EOF: Error: Syntax error in input(3).
modules/modpython/CMakeFiles/modpython_swig.dir/build.make:62: recipe for target 'modules/modpython/modpython_biglib.cpp' failed
make[2]: *** [modules/modpython/modpython_biglib.cpp] Error 1
CMakeFiles/Makefile2:3584: recipe for target 'modules/modpython/CMakeFiles/modpython_swig.dir/all' failed
make[1]: *** [modules/modpython/CMakeFiles/modpython_swig.dir/all] Error 2
Makefile:116: recipe for target 'all' failed
make: *** [all] Error 2

linuxdaemon · 2019-06-12T22:07:45Z

@H7-25 I'm unable to replicate that issue, did you run make clean before rebuilding?

H7-25 · 2019-06-12T22:08:43Z

@linuxdaemon yes i've try

H7-25 · 2019-06-13T07:43:14Z

@linuxdaemon, compiled, i've had to make a new git clone, it wasn't like something after latest pull.

H7-25 · 2019-06-16T11:59:58Z

@linuxdaemon there's still some issue :

Quote get +Y when join a channel :

linuxdaemon · 2019-06-16T20:51:28Z

@H7-25 Unable to replicate, but I have added tests for that case just in case. That might be a bug with your client

H7-25 · 2019-06-16T20:55:55Z

@linuxdaemon, sorry, i confirm is a textual issue, not znc

linuxdaemon · 2019-06-17T16:58:53Z

@DarthGandalf any other changes needed?

include/znc/Message.h

include/znc/Chan.h

src/Chan.cpp

linuxdaemon · 2019-07-31T23:54:03Z

As far as I can tell, this is ready to go. I'm unsure why CI failed, but the tests passed locally just fine

include/znc/Message.h

DarthGandalf · 2019-08-07T07:50:30Z

test/MessageTest.cpp

+    EXPECT_THAT(CMessage("CMD").GetParamsSplit(1, 10), ContainerEq(VCString()));
+    EXPECT_THAT(CMessage("CMD").GetParamsSplit(-1, 10), ContainerEq(VCString()));
+
+    EXPECT_THAT(CMessage("CMD p1 :p2 p3").GetParamsSplit(0), ContainerEq(VCString({"p1", "p2 p3"})));


ElementsAre("p1", "p2 p2") would be simpler; and the same for the rest of of this test

DarthGandalf · 2019-08-07T07:50:30Z

test/MessageTest.cpp

+    EXPECT_THAT(CMessage("CMD p1 :p2 p3").GetParamsSplit(-1), ContainerEq(VCString()));
+
+    EXPECT_THAT(CMessage("CMD p1 :p2 p3").GetParamsSplit(0, 0), ContainerEq(VCString()));
+    EXPECT_THAT(CMessage("CMD p1 :p2 p3").GetParamsSplit(1, 0), ContainerEq(VCString()));


IsEmpty()

DarthGandalf · 2019-08-07T07:56:03Z

And no, I didn't see any notification that there is a new commit after my last comments. Please say something here, otherwise it's not known that something was changed.

linuxdaemon · 2019-08-08T06:59:18Z

All comments have been addressed

Some servers may send a colon even if the last parameter doesn't need it, currently this leads to issues with permission/mode tracking, as the core doesn't handle the colon properly. This fix replaces reconstructing the parameter string with just passing a vector of the relevant parameters to CChan::SetModes() and adds overrides for CChan::SetModes() and CChan::ModeChange() that accept the vector instead. Clean up uses of old CModeMessage::GetModes() (cherry picked from commit 9536945)

linuxdaemon changed the base branch from master to 1.7.x May 24, 2019

DarthGandalf reviewed May 29, 2019

View changes

include/znc/Message.h Show resolved Hide resolved

Loading

DarthGandalf reviewed May 29, 2019

View changes

linuxdaemon changed the base branch from 1.7.x to master Jun 12, 2019

linuxdaemon added 3 commits Jun 12, 2019

Fix missing semicolon

ab676fc

Fix parameter index logic

c274126

linuxdaemon force-pushed the fix-mode-string-parse branch from 288380e to c274126 Jun 12, 2019

linuxdaemon added 5 commits Jun 12, 2019

Make SetModes/ModeChange accept explicit mode char arg

1996d48

The mode character string will be passed as the first argument, while the mode parameters are sent as a VCString as the second parameter

Document new mode handling methods

e6a12d2

Split mode string and mode params in CModeMessage methods

ad68189

Add test cases for new CModeMessage methods

2a18b3c

Add tests for GetParamsSplit

9d4e50d

Add tests for status mode handling

020c17b

DarthGandalf reviewed Jun 21, 2019

View changes

include/znc/Message.h Show resolved Hide resolved

Loading

include/znc/Chan.h Show resolved Hide resolved

Loading

src/Chan.cpp Show resolved Hide resolved

Loading

src/Chan.cpp Show resolved Hide resolved

Loading

Mark old Chan mode functions as deprecated

edb7c48

DarthGandalf reviewed Aug 7, 2019

View changes

linuxdaemon and others added 4 commits Aug 7, 2019

Refactor test cases

69c8313

Expand comments for new methods

c508fdf

Clean up uses of old CModeMessage::GetModes()

db57bc5

Fix MessageTest cases

7c20839

DarthGandalf merged commit 9536945 into znc:master Aug 8, 2019
1 of 2 checks passed

Loading

DarthGandalf mentioned this pull request Nov 4, 2019

"attempt to increment a past-the-end iterator" #1684

Closed

ghost mentioned this pull request Mar 28, 2020

ZNC topic bug #1708

Closed

znc / znc Public

Rework MODE/RPL_CHANMODEIS handling for trailing args #1661

Rework MODE/RPL_CHANMODEIS handling for trailing args #1661

linuxdaemon commented May 22, 2019

H7-25 commented May 23, 2019 •

edited

Loading

linuxdaemon commented May 23, 2019

H7-25 commented May 23, 2019 •

edited

Loading

H7-25 commented May 23, 2019

linuxdaemon commented May 23, 2019

H7-25 commented May 23, 2019

codecov bot commented May 23, 2019 •

edited

Loading

codecov bot commented May 23, 2019

DarthGandalf commented May 29, 2019

DarthGandalf May 29, 2019

linuxdaemon Jun 12, 2019

DarthGandalf Jun 12, 2019

DarthGandalf commented May 29, 2019

DarthGandalf May 29, 2019

H7-25 commented Jun 10, 2019

DarthGandalf commented Jun 10, 2019

H7-25 commented Jun 10, 2019

linuxdaemon commented Jun 12, 2019

H7-25 commented Jun 12, 2019

linuxdaemon commented Jun 12, 2019

H7-25 commented Jun 12, 2019

linuxdaemon commented Jun 12, 2019

H7-25 commented Jun 12, 2019

H7-25 commented Jun 13, 2019

H7-25 commented Jun 16, 2019

linuxdaemon commented Jun 16, 2019

H7-25 commented Jun 16, 2019

linuxdaemon commented Jun 17, 2019

linuxdaemon commented Jul 31, 2019

DarthGandalf Aug 7, 2019

DarthGandalf Aug 7, 2019

DarthGandalf commented Aug 7, 2019

linuxdaemon commented Aug 8, 2019

znc / znc Public

Rework MODE/RPL_CHANMODEIS handling for trailing args #1661

Rework MODE/RPL_CHANMODEIS handling for trailing args #1661

Conversation

linuxdaemon commented May 22, 2019

H7-25 commented May 23, 2019 • edited Loading

linuxdaemon commented May 23, 2019

H7-25 commented May 23, 2019 • edited Loading

H7-25 commented May 23, 2019

linuxdaemon commented May 23, 2019

H7-25 commented May 23, 2019

codecov bot commented May 23, 2019 • edited Loading

Codecov Report

codecov bot commented May 23, 2019

Codecov Report

DarthGandalf commented May 29, 2019

DarthGandalf May 29, 2019

Choose a reason for hiding this comment

linuxdaemon Jun 12, 2019

Choose a reason for hiding this comment

DarthGandalf Jun 12, 2019

Choose a reason for hiding this comment

DarthGandalf commented May 29, 2019

DarthGandalf May 29, 2019

Choose a reason for hiding this comment

H7-25 commented Jun 10, 2019

DarthGandalf commented Jun 10, 2019

H7-25 commented Jun 10, 2019

linuxdaemon commented Jun 12, 2019

H7-25 commented Jun 12, 2019

linuxdaemon commented Jun 12, 2019

H7-25 commented Jun 12, 2019

linuxdaemon commented Jun 12, 2019

H7-25 commented Jun 12, 2019

H7-25 commented Jun 13, 2019

H7-25 commented Jun 16, 2019

linuxdaemon commented Jun 16, 2019

H7-25 commented Jun 16, 2019

linuxdaemon commented Jun 17, 2019

linuxdaemon commented Jul 31, 2019

DarthGandalf Aug 7, 2019

Choose a reason for hiding this comment

DarthGandalf Aug 7, 2019

Choose a reason for hiding this comment

DarthGandalf commented Aug 7, 2019

linuxdaemon commented Aug 8, 2019

H7-25 commented May 23, 2019 •

edited

Loading

H7-25 commented May 23, 2019 •

edited

Loading

codecov bot commented May 23, 2019 •

edited

Loading