Opened 3 years ago

Closed 3 years ago

#1448 closed Bug / Defect (fixed)

--inactive bytes not working

Reported by: Gert Döring Owned by: Gert Döring
Priority: major Milestone: release 2.5.6
Component: Generic / unclassified Version: OpenVPN 2.5.4 (Community Ed)
Severity: Not set (select this one, unless your'e a OpenVPN developer) Keywords: msvc, --inactive, overflow
Cc: stipa, Gert Döring

Description

Server pushes

inactive 900 107374182400

which, according to the manpage, means "if there is no activity in 900s *or* "exit if less than bytes of combined in/out traffic are produced on the tun/tap device in n seconds". This is "10Gbyte in 15 minutes".

OpenVPN 2.5.x on Linux (all versions) do not abort here, neither do mingw-built Windows versions (up to 2.5.3).

MSVC built Windows versions (2.5.4 and up) do exit after 900s with

2022-02-04 09:28:53 us=781000 Inactivity timeout (--inactive), exiting

which seem to be the correct thing to do.

Now the interesting question is "why is MSVC-built windows behaving differently, and where is the real bug?"...

Change History (5)

comment:1 Changed 3 years ago by Gert Döring

The magic of integer overflows at work here.

static int
positive_atoi(const char *str)
{
    const int i = atoi(str);
    return i < 0 ? 0 : i;
}

...
    else if (streq(p[0], "inactive") && p[1] && !p[3])
    {
        VERIFY_PERMISSION(OPT_P_TIMER);
        options->inactivity_timeout = positive_atoi(p[1]);
        if (p[2])
        {
            options->inactivity_minimum_bytes = positive_atoi(p[2]);
        }

... and this number is too big for an "int", so it overflows. On Linux, the result of the overflow seems to be "0" (and on MinGW-built Windows), so it does "nothing". On MSVC built, the result is (231)-1 (214748364), which is about 2Gbyte - hard to achieve in 15minutes normally, so "it does as instructed".

Now, what to do about it? Go for a long for option parsing? And then what? Stay with a "long" (and update all the code paths to "long") or warn/abort if there is an integer overlflow, keeping the existing behaviour but making clear what happens?

Last edited 3 years ago by Gert Döring (previous) (diff)

comment:3 Changed 3 years ago by Gert Döring

Owner: set to Gert Döring
Status: newaccepted

comment:4 Changed 3 years ago by Gert Döring

Keywords: --inactive overflow added; inactive removed
Milestone: release 2.5.6

commit cae1a7fcf14e6ded34ab5a1e8842c3034cc89608 (master)
commit 1e573aa9b31d9270bd43d8c5a448314508a3311f (release/2.5)
Author: Gert Doering
Date: Fri Feb 4 12:42:01 2022 +0100

Repair --inactive with 'bytes' argument larger 2Gbytes.

Fix will be in 2.5.6 (... breaking all the Linux installations as well!) and in 2.6.0

comment:5 Changed 3 years ago by Gert Döring

Resolution: fixed
Status: acceptedclosed
Note: See TracTickets for help on using tickets.