[defect]: dictionary.freeradius FreeRADIUS-Stats-Server-State doesn't match home_state_t

[matthubb]

What type of defect/bug is this?

Unexpected behaviour (obvious or verified by project member)

How can the issue be reproduced?

Commit 162aa91f71c4657d3944b80621af568d76fea8aa introduced a backwards-incompatible change to home_state_t typedef enum and dictionary.freeradius does not reflect the changes.

After this change, server-status responses including FreeRADIUS VSA FreeRADIUS-Stats-Server-State will be misreported by clients / utilities (e.g. radclient, wireshark) using the current dictionary.freeradius.

The enum values for home_state_t changed from:

0 - HOME_STATE_ALIVE
1 - HOME_STATE_ZOMBIE
2 - HOME_STATE_IS_DEAD
3 - HOME_STATE_UNKNOWN

To:

0 - HOME_STATE_UNKNOWN
1 - HOME_STATE_ALIVE
2 - HOME_STATE_ZOMBIE
3 - HOME_STATE_IS_DEAD
4 - HOME_STATE_CONNECTION_FAIL

But dictionary.freeradius stayed as:

VALUE   FreeRADIUS-Stats-Server-State   Alive                   0
VALUE   FreeRADIUS-Stats-Server-State   Zombie                  1
VALUE   FreeRADIUS-Stats-Server-State   Dead                    2
VALUE   FreeRADIUS-Stats-Server-State   Idle                    3

Therefore server response encoding HOME_STATE_ALIVE according to the changed realms.h will be decoded as Zombie by a client according to dictionary.freeradius.

Log output from the FreeRADIUS daemon

Hopefully not required as the discrepancy between `realms.h` and `dictionary.freeradius` is clear.

Relevant log output from client utilities

No response

Backtrace from LLDB or GDB

No response

[alandekok]

Thanks. I've pushed some fixes. Please verify.

[matthubb]

There are state comparisons in src/main/process.c that will need reworking as HOME_STATE_IS_DEAD is considered ordinally in multiple locations.

e.g. https://github.com/FreeRADIUS/freeradius-server/blob/b46a217a4640f86e7c08497adee9edcfe1603a3d/src/main/process.c#L3124

https://github.com/FreeRADIUS/freeradius-server/blob/b46a217a4640f86e7c08497adee9edcfe1603a3d/src/main/process.c#L3659

[alandekok]

Those comparisons didn't change when the enum was changed. So they should be fine.

[matthubb]

The HOME_STATE_UNKNOWN will now be numerically greater than HOME_STATE_IS_DEAD. So this would change the evaluation of the following when home->state == HOME_STATE_UNKNOWN. Resulting in behaviour change.

        /*
         *  The home server is alive (or may be alive).
         *  Send the packet to the IP.
         */
        if (home->state < HOME_STATE_IS_DEAD) goto do_home;

https://github.com/FreeRADIUS/freeradius-server/blob/b46a217a4640f86e7c08497adee9edcfe1603a3d/src/main/process.c#L3124

[alandekok]

Again, please read what I write. That comparison didn't change when the enum changed. So when the enum changed, there was behavior change.

Putting the enum back restores the previous behavior. And fixes the bug.

If you do git annotate on the lines which do these checks, you'll see they were last changed in 2018/2020. So it is not a behavior change to restore behavior which was tested and working in 2020.

[matthubb]

The commit 162aa91f71c4657d3944b80621af568d76fea8aa that introduced this change to the enum, also changed process.c:

         *
         *  If it's zombie, we mark it alive immediately.
         */
-       if ((home->state == HOME_STATE_IS_DEAD) &&
+       if ((home->state >= HOME_STATE_IS_DEAD) &&

[matthubb]

There are further numeric comparisons of the state value which may have changed behaviour as a result of the enum re-ordering. Changing each to a series of equality checks would remove the now incorrect order significance of the enum.

$ git checkout c493836d2a72d2ebb29e5a2d5f45f3e033ea0094
Previous HEAD position was b46a217... resync.  Helps with #4570
HEAD is now at c493836... check for multiple "down" states.  Fixes #4790
$ ack 'home(?:_server)?->state [^=]'
src/main/process.c
3126:           if (home->state < HOME_STATE_IS_DEAD) goto do_home;
3174:           if (home->state < HOME_STATE_IS_DEAD) goto do_home;
3661:           if ((home->state >= HOME_STATE_IS_DEAD) &&
4082:           if ((home->state >= HOME_STATE_IS_DEAD) ||
4562:       (request->home_server->state >= HOME_STATE_IS_DEAD) ||

src/main/realms.c
2650:           if (home->state >= HOME_STATE_IS_DEAD) {
2815:                   if ((home->state >= HOME_STATE_IS_DEAD) &&

[alandekok]

There are further numeric comparisons of the state value which may have changed behaviour as a result of the enum re-ordering.

As I had said... look at git annotate. If those comparisons are from BEFORE the enum changes, they're fine.

If you think they're not fine, then supply patch.

[matthubb]

PRs for both v3.0.x and v3.2.x branches submitted to avoid potential behaviour changes after the enum order was reverted.

[alandekok]

... to avoid potential behaviour changes after the enum order was reverted.

I have to ask here whether you're reading anything I'm writing.

The checks for < HOME_SERVER_IS_DEAD and >= HOME_SERVER_IS_DEAD are explicitly dependent on the order of the enum. When the enum order was changed, the checks became incorrect. When the enum order was fixed, the checks became correct again.

I have no idea why you think the checks which have not changed for 4 years are now magically incorrect, when the enum has reverted to the same order as from 4 years ago. If the checks were correct 4 years ago, they are now correct again.

Your "fixes" also miss a dead state of ADMIN_DOWN. So the fixes are in fact worse than the existing code.