Merge pull request #1325 from jimklimov/libusb-signed-char

LibUSB related code - revise for bit-maths with signed char

Author: jimklimov

drivers/blazer_usb.c

@@ -684,7 +684,7 @@ void upsdrv_initups(void)
 		 * This should allow automatic application of the workaround */
 		ret = usb_get_string(udev, 0, 0, (usb_ctrl_charbuf)tbuf, sizeof(tbuf));
 		if (ret >= 4) {
-			langid = tbuf[2] | (tbuf[3] << 8);
+			langid = (unsigned char)tbuf[2] | ((unsigned char)tbuf[3] << 8);
 			upsdebugx(1, "First supported language ID: 0x%x (please report to the NUT maintainer!)", langid);
 		}
 	}

drivers/libshut.c

@@ -334,7 +334,7 @@ static int shut_control_msg(
 
 /* Data portability */
 /* realign packet data according to Endianess */
-#define BYTESWAP(in) (((in & 0xFF) << 8) + ((in & 0xFF00) >> 8))
+#define BYTESWAP(in) ((((uint16_t)in & 0x00FF) << 8) + (((uint16_t)in & 0xFF00) >> 8))
 static void align_request(struct shut_ctrltransfer_s *ctrl)
 {
 #if (defined (WORDS_BIGENDIAN)) && (WORDS_BIGENDIAN)
@@ -893,7 +893,7 @@ static unsigned char shut_checksum(
 	unsigned char chk=0;
 
 	for(i=0; i<bufsize; i++)
-		chk^=buf[i];
+		chk ^= (unsigned char)buf[i];
 
 	upsdebugx (4, "shut_checksum: %02x", chk);
 	return chk;

drivers/libusb0.c

@@ -377,7 +377,7 @@ static int libusb_open(usb_dev_handle **udevp,
 
 				upsdebug_hex(3, "HID descriptor, method 1", buf, 9);
 
-				rdlen1 = (uint8_t)buf[7] | ((uint8_t)buf[8] << 8);
+				rdlen1 = ((uint8_t)buf[7]) | (((uint8_t)buf[8]) << 8);
 			}
 
 			if (rdlen1 < -1) {
@@ -407,7 +407,7 @@ static int libusb_open(usb_dev_handle **udevp,
 				) {
 					p = (usb_ctrl_char *)&iface->extra[i];
 					upsdebug_hex(3, "HID descriptor, method 2", p, 9);
-					rdlen2 = (uint8_t)p[7] | ((uint8_t)p[8] << 8);
+					rdlen2 = ((uint8_t)p[7]) | (((uint8_t)p[8]) << 8);
 					break;
 				}
 			}

drivers/libusb1.c

@@ -141,7 +141,9 @@ static int nut_libusb_open(libusb_device_handle **udevp,
 #if (defined HAVE_LIBUSB_DETACH_KERNEL_DRIVER) || (defined HAVE_LIBUSB_DETACH_KERNEL_DRIVER_NP)
 	int retries;
 #endif
-	int rdlen1, rdlen2; /* report descriptor length, method 1+2 */
+	/* libusb-1.0 usb_ctrl_charbufsize is uint16_t and we
+	 * want the rdlen vars signed - so taking a wider type */
+	int32_t rdlen1, rdlen2; /* report descriptor length, method 1+2 */
 	USBDeviceMatcher_t *m;
 	libusb_device **devlist;
 	ssize_t	devcount = 0;
@@ -159,7 +161,7 @@ static int nut_libusb_open(libusb_device_handle **udevp,
 
 	/* report descriptor */
 	unsigned char	rdbuf[MAX_REPORT_SIZE];
-	int		rdlen;
+	int32_t		rdlen;
 
 	/* libusb base init */
 	if (libusb_init(NULL) < 0) {
@@ -425,7 +427,7 @@ static int nut_libusb_open(libusb_device_handle **udevp,
 
 			upsdebug_hex(3, "HID descriptor, method 1", buf, 9);
 
-			rdlen1 = buf[7] | (buf[8] << 8);
+			rdlen1 = ((uint8_t)buf[7]) | (((uint8_t)buf[8]) << 8);
 		}
 
 		if (rdlen1 < -1) {
@@ -450,7 +452,7 @@ static int nut_libusb_open(libusb_device_handle **udevp,
 			if (i+9 <= if_desc->extra_length && if_desc->extra[i] >= 9 && if_desc->extra[i+1] == 0x21) {
 				p = &if_desc->extra[i];
 				upsdebug_hex(3, "HID descriptor, method 2", p, 9);
-				rdlen2 = p[7] | (p[8] << 8);
+				rdlen2 = ((uint8_t)p[7]) | (((uint8_t)p[8]) << 8);
 				break;
 			}
 		}

drivers/nutdrv_qx.c

@@ -3058,7 +3058,7 @@ void	upsdrv_initups(void)
 			ret = usb_get_string(udev, 0, 0,
 				(usb_ctrl_charbuf)tbuf, sizeof(tbuf));
 			if (ret >= 4) {
-				langid = tbuf[2] | (tbuf[3] << 8);
+				langid = ((uint8_t)tbuf[2]) | (((uint8_t)tbuf[3]) << 8);
 				upsdebugx(1,
 					"First supported language ID: 0x%x "
 					"(please report to the NUT maintainer!)",

drivers/riello_usb.c

@@ -95,9 +95,9 @@ static int cypress_setfeatures()
 
 	/* Write features report */
 	ret = usb_control_msg(udev, USB_ENDPOINT_OUT + USB_TYPE_CLASS + USB_RECIP_INTERFACE,
-								0x09,						/* HID_REPORT_SET = 0x09 */
-								0 + (0x03 << 8),		/* HID_REPORT_TYPE_FEATURE */
-								0, (usb_ctrl_charbuf) bufOut, 0x5, 1000);
+		0x09,				/* HID_REPORT_SET = 0x09 */
+		0 + (0x03 << 8),		/* HID_REPORT_TYPE_FEATURE */
+		0, (usb_ctrl_charbuf) bufOut, 0x5, 1000);
 
 	if (ret <= 0) {
 		upsdebugx(3, "send: %s", ret ? nut_usb_strerror(ret) : "error");
@@ -197,7 +197,7 @@ static int Get_USB_Packet(uint8_t *buffer)
 	}
 
 	/* copy to buffer */
-	size = inBuf[0] & 0x07;
+	size = (unsigned char)(inBuf[0]) & 0x07;
 	if (size)
 		memcpy(buffer, &inBuf[1], size);
 

drivers/tripplite_usb.c

@@ -743,10 +743,11 @@ static int soft_shutdown(void)
 static int hard_shutdown(void)
 {
 	int ret;
-	char buf[256], cmd_N[]="N\0x", cmd_K[] = "K\0";
+	unsigned char buf[256], cmd_N[]="N\0x", cmd_K[] = "K\0";
 
-	cmd_N[2] = offdelay;
-	cmd_N[1] = offdelay >> 8;
+	/* FIXME: Assumes memory layout / endianness? */
+	cmd_N[2] = (unsigned char)(offdelay & 0x00FF);
+	cmd_N[1] = (unsigned char)(offdelay >> 8);
 	upsdebugx(3, "hard_shutdown(offdelay=%d): N", offdelay);
 
 	ret = send_cmd(cmd_N, sizeof(cmd_N), buf, sizeof(buf));

tests/getvaluetest.c

@@ -10,6 +10,7 @@
  *
  * Copyright (C)
  *      2021    Nick Briggs <[email protected]>
+ *      2022    Jim Klimov <[email protected]>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -34,6 +35,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include "hidtypes.h"
+#include "usb-common.h"
 #include "common.h"
 
 void GetValue(const unsigned char *Buf, HIDData_t *pData, long *pValue);
@@ -125,6 +127,106 @@ static int RunBuiltInTests(char *argv[]) {
 			exitStatus = 1;
 		}
 	}
+
+	/* Emulate rdlen calculations in libusb{0,1}.c or
+	 * langid calculations in nutdrv_qx.c; in these
+	 * cases we take two bytes (cast from usb_ctrl_char
+	 * type, may be signed depending on used API version)
+	 * from the protocol buffer, and build a platform
+	 * dependent representation of a two-byte word.
+	 */
+	usb_ctrl_char	bufC[2];
+	signed char	bufS[2];
+	unsigned char	bufU[2];
+	int rdlen;
+
+	/* Example from issue https://github.com/networkupstools/nut/issues/1261
+	 * where resulting length 0x01a9 should be "425" but ended up "-87" */
+	bufC[0] = (usb_ctrl_char)0xa9;
+	bufC[1] = (usb_ctrl_char)0x01;
+
+	bufS[0] = (signed char)0xa9;
+	bufS[1] = (signed char)0x01;
+
+	bufU[0] = (unsigned char)0xa9;
+	bufU[1] = (unsigned char)0x01;
+
+	/* Check different conversion methods and hope current build CPU,
+	 * C implementation etc. do not mess up bit-shifting vs rotation,
+	 * zeroing high bits, int type width extension et al. If something
+	 * is mismatched below, the production NUT code may need adaptations
+	 * for that platform to count stuff correctly!
+	 */
+	printf("\nTesting bit-shifting approaches used in codebase to get 2-byte int lengths from wire bytes:\n");
+	printf("(Expected correct value is '425', incorrect '-87' or other)\n");
+
+#define REPORT_VERDICT(expected) { if (expected) { printf(" - PASS\n"); } else { printf(" - FAIL\n"); exitStatus = 1; } }
+
+	rdlen = bufC[0] | (bufC[1] << 8);
+	printf(" * reference: no casting, usb_ctrl_char :\t%d\t(depends on libusb API built against)", rdlen);
+	REPORT_VERDICT (rdlen == 425 || rdlen == -87)
+
+	rdlen = bufS[0] | (bufS[1] << 8);
+	printf(" * reference: no casting, signed char   :\t%d\t(expected '-87' here)", rdlen);
+	REPORT_VERDICT (rdlen == -87)
+
+	rdlen = bufU[0] | (bufU[1] << 8);
+	printf(" * reference: no casting, unsigned char :\t%d\t(expected '425')", rdlen);
+	REPORT_VERDICT (rdlen == 425)
+
+
+	rdlen = (uint8_t)bufC[0] | ((uint8_t)bufC[1] << 8);
+	printf(" * uint8_t casting, usb_ctrl_char :\t%d", rdlen);
+	REPORT_VERDICT (rdlen == 425)
+
+	rdlen = (uint8_t)bufS[0] | ((uint8_t)bufS[1] << 8);
+	printf(" * uint8_t casting, signed char   :\t%d", rdlen);
+	REPORT_VERDICT (rdlen == 425)
+
+	rdlen = (uint8_t)bufU[0] | ((uint8_t)bufU[1] << 8);
+	printf(" * uint8_t casting, unsigned char :\t%d", rdlen);
+	REPORT_VERDICT (rdlen == 425)
+
+
+	rdlen = ((uint8_t)bufC[0]) | (((uint8_t)bufC[1]) << 8);
+	printf(" * uint8_t casting with parentheses, usb_ctrl_char :\t%d", rdlen);
+	REPORT_VERDICT (rdlen == 425)
+
+	rdlen = ((uint8_t)bufS[0]) | (((uint8_t)bufS[1]) << 8);
+	printf(" * uint8_t casting with parentheses, signed char   :\t%d", rdlen);
+	REPORT_VERDICT (rdlen == 425)
+
+	rdlen = ((uint8_t)bufU[0]) | (((uint8_t)bufU[1]) << 8);
+	printf(" * uint8_t casting with parentheses, unsigned char :\t%d", rdlen);
+	REPORT_VERDICT (rdlen == 425)
+
+
+	rdlen = ((uint16_t)(bufC[0]) & 0x00FF) | (((uint16_t)(bufC[1]) & 0x00FF) << 8);
+	printf(" * uint16_t casting with 8-bit mask, usb_ctrl_char :\t%d", rdlen);
+	REPORT_VERDICT (rdlen == 425)
+
+	rdlen = ((uint16_t)(bufS[0]) & 0x00FF) | (((uint16_t)(bufS[1]) & 0x00FF) << 8);
+	printf(" * uint16_t casting with 8-bit mask, signed char   :\t%d", rdlen);
+	REPORT_VERDICT (rdlen == 425)
+
+	rdlen = ((uint16_t)(bufU[0]) & 0x00FF) | (((uint16_t)(bufU[1]) & 0x00FF) << 8);
+	printf(" * uint16_t casting with 8-bit mask, unsigned char :\t%d", rdlen);
+	REPORT_VERDICT (rdlen == 425)
+
+
+	rdlen = 256 * (uint8_t)(bufC[1]) + (uint8_t)(bufC[0]);
+	printf(" * uint8_t casting with multiplication, usb_ctrl_char :\t%d", rdlen);
+	REPORT_VERDICT (rdlen == 425)
+
+	rdlen = 256 * (uint8_t)(bufS[1]) + (uint8_t)(bufS[0]);
+	printf(" * uint8_t casting with multiplication, signed char   :\t%d", rdlen);
+	REPORT_VERDICT (rdlen == 425)
+
+	rdlen = 256 * (uint8_t)(bufU[1]) + (uint8_t)(bufU[0]);
+	printf(" * uint8_t casting with multiplication, unsigned char :\t%d", rdlen);
+	REPORT_VERDICT (rdlen == 425)
+
+
 	return (exitStatus);
 }