Nut upsdev - Jan 2008 - usbhid-ups bug

Hi all,

greetings and a happy new year! I am writing as a user, rather than as
an ex-developer. Usbhid-ups contains a fatal bug that was apparently
introduced in r1210 (I did a binary search to find the exact
revision). Arjen, you wrote "correct me if I'm wrong", so it seems
that's what I am doing. 

------------------------------------------------------------------------
r1210 | adkorte-guest | 2008-01-02 16:34:26 -0400 (Wed, 02 Jan 2008) | 3 lines

Parsing continues if the HID path for a report ends in 0x00000000
(these will not be made available). Commonly used as placeholders.

It looks like (but correct me if I'm wrong) that reports for which the
HID path ends in 0x00000000 are placeholders only. By ignoring them
already during the parsing of the report descriptor, they will not end
up in the list of pData elements in the parsed report descriptor, so
we don't have to ignore them later on (with every interrupt report we
receive). This saves both time and complexity.
------------------------------------------------------------------------

Symptom: when connecting to an APC Back-UPS 500, everything works fine
until r1209; from r1210, I get lots of error messages such as

Can't retrieve Report 22: Value too large for defined data type
HIDGetDataValue: Value too large for defined data type

For reference, attached are five files:

r1209 and r1210: respective output of '/usr/bin/usbhid-ups -DD -a apc'
u1209 and u1210: respective output of 'upsc apc'
descriptor-apc.hex: the APC's report descriptor dump

Further notes: it seems (see r1209) that
UPS.PowerSummary.PresentStatus.00000000

is indeed something meaningful for the APC driver. I am not sure
why. It holds the status info. Comparing the outputs of "upsc", r1210
wrongly thinks the device is on battery.

Moreover, when running r1215, my dmesg fills up with error messages like this:

usb 3-2: usbfs: USBDEVFS_CONTROL failed cmd usbhid-ups rqt 161 rq 1 len 3 ret
-75

I don't know if this is the same error or not. It seems that some
ioctl fails. If I remember correctly, 75 is the error code for "Value
too large for defined data type", so probably yes.

I also get a "protocol error" when trying -k; again I didn't have
time
to check if that error appears in the same revision or not.

I am sorry that I don't have time to try to fix this myself; I have
already taken several hours off from child care today to try to
collect the above information (and I am off work to be a full-time
parent right now). Please let me know if you need more info or
testing, and I'll do my best.

-- Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: nut-debug.tgz
Type: application/octet-stream
Size: 5007 bytes
Desc: gzip compressed data, from Unix
Url :
http://lists.alioth.debian.org/pipermail/nut-upsdev/attachments/20080111/8c039816/attachment.obj

> Symptom: when connecting to an APC Back-UPS 500, everything works fine
> until r1209; from r1210, I get lots of error messages such as
>
> Can't retrieve Report 22: Value too large for defined data type
> HIDGetDataValue: Value too large for defined data type
Ouch, you don't have to go any further, I found the mistake. The
'placeholder' reports shouldn't be removed at the place they are
removed
now, since they are used to calculate the lenght of the full report (the
last part of Parse_ReportDesc() in 'drivers/hidparser.c'). If (like in
your case) they make up the remainder of a report (and nothing useful is
after them), it will effectively trim the length of the report to the used
data only. This is wrong, since we won't allocate enough room in the
report buffer then, which will cause the failures you're seeing.
> For reference, attached are five files:
>
> r1209 and r1210: respective output of '/usr/bin/usbhid-ups -DD -a
apc'
> u1209 and u1210: respective output of 'upsc apc'
> descriptor-apc.hex: the APC's report descriptor dump
>
> Further notes: it seems (see r1209) that
> UPS.PowerSummary.PresentStatus.00000000
>
> is indeed something meaningful for the APC driver. I am not sure
> why. It holds the status info. Comparing the outputs of "upsc",
r1210
> wrongly thinks the device is on battery.
If we miscalculate the lenght of the reports (which r1210 will cause), you
can expect all kinds of problems. Thanks to your detailed problem
analysis, this won't be difficult to fix.

Best regards, Arjen
-- 
Eindhoven - The Netherlands
Key fingerprint - 66 4E 03 2C 9D B5 CB 9B  7A FE 7E C1 EE 88 BC 57