Tun (valgrind and route issue)

[caeies]

This PR add some various fixes:

• most important: fixes a wrong call to route setup after the tun_setup.
• fixes several issues from valgrind (memory leaks, use after free, garbage writting).
• add a sleep after login failure in the persistent != 0 case to avoid locking the account on wrong password (should we reask password or force exit in such case ?).

Targeted to enhance #1048 .

NOTE: still missing:

• compilation flags to allow MacOS and BSD compilation (without the tun interface).
• still some leaks due to RC are laying around, investigation still in progress.: remaining leaks are due to the way the shutdown is processed. So we can probably ignore them as they are not happening during the running time.

[DimitriPapadopoulos]

Could you rebase to solve conflicts? Probably caused by Coverity fixes, sorry about it.

[caeies]

Could you rebase to solve conflicts? Probably caused by Coverity fixes, sorry about it.

Done. But take care your last coverity fix adds a warning on (and I'm with a -Werror build):

diff --git a/src/userinput.c b/src/userinput.c
index 7e54c32..8af3e10 100644
--- a/src/userinput.c
+++ b/src/userinput.c
@@ -370,7 +370,6 @@ void read_password(const char *pinentry, const char *hint,
 char *read_from_stdin(size_t count)
 {
        char *buf;
-       char *output;
        ssize_t bytes_read;
 
        assert(count < SIZE_MAX - 1);

[caeies]

And by the way, I confirm that there's a race condition between the ppp set route and the WARN: Removing wrong route to vpn server... sometime the INFO: Interface pppVPN is UP is triggered without it, leaving the tunnel stuck forever (which is curious because the echo request should trigger the tunnel down.

Best regards.

[DimitriPapadopoulos]

I would rather move 2c5c9a8 to a new PR against the master branch, to discuss this separately.

Do any of the commits here, especially the Valgrind findings, apply to the master branch, or only to the tun branch? Again, I would rather move those that apply to the master branch to a separate PR against the master branch. and release a new version before merging the tun branch.

[caeies]

I would rather move 2c5c9a8 to a new PR against the master branch, to discuss this separately.

Do any of the commits here, especially the Valgrind findings, apply to the master branch, or only to the tun branch? Again, I would rather move those that apply to the master branch to a separate PR against the master branch. and release a new version before merging the tun branch.

Yeah agreed. see #1306 #1305 and #1304 . thanks.

[DimitriPapadopoulos]

Make it a size_t instead of a ssize_t, since packet->len is also a size_t?

[caeies]

well they are negative operations on these values (because hdlc_bufsize and len (not packet->len) are of the same type) , so not sure it is 100% safe in the current code to do so.

At minimum it would require to check the size of the output packet in pppd_read function and drop all packets of a size < 2 if I get it correctly.

And honestly speaking this is present in the whole file, because most of the time these values contains the return value of the read operations like (for example look at pppd_read around lines 1112).

So let me check more deeply if it is "easily" doable or not :) (and I'm not sure this has to be present in this branch anyway, as this code was the previous one and I just moved it "like this" as it was done in previous patch).

Best regards.

[caeies]

Ok, as I have reduced the scope of the variable, it's easy to do, so I did it. But clearly, the whole code should be reviewed to handle this properly in all parts...

[caeies]

by the way, take care that the tun branch is far behind the current master (so I rebased this one on the coverity_scan instead of the origin/tun one).