Re: [pptp-devel] pptp connection dies in pptp-linux_1.9.* Works fine in pptp-linux_1.8.0-1

[Christoph B. <sou...@ma...>]

James Cameron wrote...

> 1.  upgrade to 1.10.0 which was released in January, although none of
> the patches between 1.9.0 and 1.10.0 seem to me to be related to your
> problem,

Debian maintainer here ...

Any change here was fairly surprising since technically 1.9.0+ds-2 is
1.10.0 as it contains all the patches. That's why I haven't done another
upload yet although it's on the agenda.

> 2.  check what the Debian or Ubuntu developers changed in the package,
> and remove those changes, or;

Checking that requires (slight) knowledge of Debian packaging, so a
hint, there was a change in how modifications were organized.

Up to and including 1.8.0-1 you'd examine the tree after unpacking
(dpkg-source -x *.dsc) but don't be fooled by debian/patches/ in these
versions though, it's not used. You'll find several changes, especially
all this "missing window" handling already exists before it was added
upstream in 1.9.0

>From 1.9.0+ds-1, just look into the files in debian/patches/, they
can also be found at

    https://sources.debian.org/patches/pptp-linux/1.9.0+ds-2/

So, after a first glance I saw no reason why the Debian changes caused
this regression. Of the many changes, two stick out a bit, though:

For quite a while I have been staring at pqueue.c:

@@ -421,8 +426,9 @@ int decaps_gre (int fd, callback_t callback, int cl)
                 seq, seq_recv + 1);
         stats.rx_underwin++;
     /* sequence number too high, is it reasonably close? */
-    } else if ( (missing_window == -1) || 
-                               (seq < seq_recv + missing_window || WRAPPED(seq, seq_recv + missing_window)) ) {
+    } else if ( (seq < seq_recv + missing_window ||
+                 WRAPPED(seq, seq_recv + missing_window)) ||
+               (missing_window == -1) ) {
        stats.rx_buffered++;
         if ( log_level >= 1 )
             log("%s packet %d (expecting %d, lost or reordered)",

... but doubt it has any effect - with missing_window == -1 the latter
(and current) form might result in underruns but this shouldn't do harm,
it's just visual.

Another change is pqueue.c:

@@ -137,6 +149,7 @@ int pqueue_add (u_int32_t seq, unsigned char *packet, int packlen) {
     if (point->seq == seq) {
       // queue already contains this packet
       warn("discarding duplicate packet %d", seq);
+      pq_freelist_add(point);
       return -1;
     }
     if (point->seq > seq) {

This was brought by 503ceb0 ("pqueue: close a memory leak") and never
backported in Debian (but probably should by yours truly). The obvious
assumption was there is no proper cleanup when re-using a blob but it
seems to be done right.


3. Find a third option :)

> 4.  use git bisect to find which patch introduced the problem between
> 1.8.0 and 1.9.0.

Dmitry, do you have knowledge in compiling from source and re-building
Debian packages? Since your situation is hard to create, we cannot test
from outside.

Another thing, when using 1.8.0-1 you might see increasing memory usage
of the pptp process, can you confirm?

    Christoph