hidparser.c: GetValue() can return incorrect result when compiled for 64-bit environment

[nbriggs]

I've discovered, with the attached test case (rename the .txt to .c), using
testgetvalue.txt, code copied from hidparser.c, that extracting a value from a report:

  const unsigned char Buf[5] = {0x8d, 0xFF, 0xFF, 0xFF, 0xFF};
  HIDData_t Data = {.LogMax=2147483647, .LogMin = -1, .Size = 32};
  long Value;

with GetValue(&Buf, &Data, &Value), independent of whether the processor is big-endian or little-endian, produces Value = -1 when compiled in an ILP32 environment and Value = 0 when compiled in an LP64 environment.

I'll track down the problem and propose a fix.

[nbriggs]

In GetValue --

        /* figure out how many bits are significant */
        range = pData->LogMax - pData->LogMin + 1;
        if (range <= 0) {
                /* makes no sense, give up */
                *pValue = value;
                return;
        }

If you're in an ILP32 environment, range (long) will be < 0 if there was overflow in the calculation, which there is in this case of Min=-1, Max=2147483647, and so it will never do any checking and just return the value.
I think there are other things wrong with the logic, but this explains why in ILP32 environments it returns the expected value, but in LP64 it presses on and subsequently gets it wrong.

[nbriggs]

I've got a revised implementation for GetValue, but won't be able to submit it until next week.
The main difference is that it uses the absolute magnitude of the LogMin and LogMax values, rather than the range, to determine how much of the input gets masked off. Are there test cases capturing input from some of the less well conforming UPSs?

[jimklimov]

Sorry for the delay. Unfortunately, currently there are not many tests present, and those that are - are mostly for code quality/syntax. There was a long-standing plan to mock some device activities (so dummy-ups appeared) to test the upsd server and its clients, but probably nothing in place for testing against real hardware - including workarounds for known-broken hardware (and the chain of OS drivers, libs, etc. leading to that) especially as there is no persistent pool of HW for the project... just some team members or users and contributors reporting results with what they have on hand.

[blecher-at]

While working on #616 I noticed the following issue caused by this change.
Without the changes to drivers/hidparser.c, I see 0x1709 converted to 232,7 and 0x14 to 20, which is correct

0.315012 [D3] Report[get]: (3 bytes) => 0f 17 09
0.315152 [D1] Path: UPS.Input.Voltage, Type: Feature, ReportID: 0x0f, Offset: 0, Size: 16, Value: 232.7
0.229773 [D3] Report[get]: (2 bytes) => 13 14
0.229936 [D1] Path: UPS.Output.PercentLoad, Type: Feature, ReportID: 0x13, Offset: 0, Size: 8, Value: 20
0.228399 [D3] Report[get]: (3 bytes) => 1c f6 01
0.228577 [D1] Path: UPS.Input.Frequency, Type: Feature, ReportID: 0x1c, Offset: 0, Size: 16, Value: 50.2
revert_1023.txt

And after the changes made here, I see wrong values of -0.1, -1 respectively

0.309297 [D3] Report[get]: (3 bytes) => 0f 17 09
0.309590 [D1] Path: UPS.Input.Voltage, Type: Feature, ReportID: 0x0f, Offset: 0, Size: 16, Value: -0.1
0.229773 [D3] Report[get]: (2 bytes) => 13 14
0.225024 [D1] Path: UPS.Output.PercentLoad, Type: Feature, ReportID: 0x13, Offset: 0, Size: 8, Value: -1
issue_1023.txt

[jimklimov]

Thanks for the report about possible regression, I've posted a PR to help investigate this. THX: #1138