On 17.10.2015 14:52, Marcus Meissner wrote:
1327477 Wrong sizeof argument
Signed-off-by: Marcus Meissner marcus@jet.franken.de
dlls/hidclass.sys/device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c index e7e7e11..dcc7d3c 100644 --- a/dlls/hidclass.sys/device.c +++ b/dlls/hidclass.sys/device.c @@ -277,7 +277,7 @@ static DWORD CALLBACK hid_device_thread(void *args) packet->reportId = 0;
irp = IoBuildDeviceIoControlRequest(IOCTL_HID_GET_INPUT_REPORT,
device, NULL, 0, packet, sizeof(packet), TRUE, events[0],
device, NULL, 0, packet, sizeof(*packet)+ext->preparseData->caps.InputReportByteLength, TRUE, events[0], &irp_status); irpsp = IoGetNextIrpStackLocation(irp);
This looks wrong, you have to allocate a separate buffer, and then copy it (like in the code below). I don't know why some of these buffers are HEAP_ZERO_MEMORY though, and others not? Adding Aric, he might want to review this part again, especially since Coverity detected more issues in this code (unnecessary assignment of "rc" for example).
On Sat, Oct 17, 2015 at 03:31:24PM +0200, Sebastian Lackner wrote:
On 17.10.2015 14:52, Marcus Meissner wrote:
1327477 Wrong sizeof argument
Signed-off-by: Marcus Meissner marcus@jet.franken.de
dlls/hidclass.sys/device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c index e7e7e11..dcc7d3c 100644 --- a/dlls/hidclass.sys/device.c +++ b/dlls/hidclass.sys/device.c @@ -277,7 +277,7 @@ static DWORD CALLBACK hid_device_thread(void *args) packet->reportId = 0;
irp = IoBuildDeviceIoControlRequest(IOCTL_HID_GET_INPUT_REPORT,
device, NULL, 0, packet, sizeof(packet), TRUE, events[0],
device, NULL, 0, packet, sizeof(*packet)+ext->preparseData->caps.InputReportByteLength, TRUE, events[0], &irp_status); irpsp = IoGetNextIrpStackLocation(irp);This looks wrong, you have to allocate a separate buffer, and then copy it (like in the code below). I don't know why some of these buffers are HEAP_ZERO_MEMORY though, and others not? Adding Aric, he might want to review this part again, especially since Coverity detected more issues in this code (unnecessary assignment of "rc" for example).
something seems wrong though here, yes.
rc issue is "CID 1327478 Unused value"
Ciao, Marcus
On 10/17/15 8:34 AM, Marcus Meissner wrote:
On Sat, Oct 17, 2015 at 03:31:24PM +0200, Sebastian Lackner wrote:
On 17.10.2015 14:52, Marcus Meissner wrote:
1327477 Wrong sizeof argument
Signed-off-by: Marcus Meissner marcus@jet.franken.de
dlls/hidclass.sys/device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c index e7e7e11..dcc7d3c 100644 --- a/dlls/hidclass.sys/device.c +++ b/dlls/hidclass.sys/device.c @@ -277,7 +277,7 @@ static DWORD CALLBACK hid_device_thread(void *args) packet->reportId = 0;
irp = IoBuildDeviceIoControlRequest(IOCTL_HID_GET_INPUT_REPORT,
device, NULL, 0, packet, sizeof(packet), TRUE, events[0],
device, NULL, 0, packet, sizeof(*packet)+ext->preparseData->caps.InputReportByteLength, TRUE, events[0], &irp_status); irpsp = IoGetNextIrpStackLocation(irp);This looks wrong, you have to allocate a separate buffer, and then copy it (like in the code below). I don't know why some of these buffers are HEAP_ZERO_MEMORY though, and others not? Adding Aric, he might want to review this part again, especially since Coverity detected more issues in this code (unnecessary assignment of "rc" for example).
something seems wrong though here, yes.
rc issue is "CID 1327478 Unused value"
Ciao, Marcus
What is happening here is that the HID_XFER_PACKET is not a variable length structure, it takes a pointer to the buffer. I am taking a bit of a shortcut and instead of allocating the HID_XFER_PACKET structure and a separate buffer, I am allocating 1 block of memory that is both, the buffer just comes after the HID_XFER_PACKET structure.
I am pretty sure Marcus' original patch is unnecessary. I am not surprised that there are issues in this code area though. It when through a lot of revision and tweaking as I was getting things working and that often results in cruft.
-aric
On 19 October 2015 at 15:04, Aric Stewart aric@codeweavers.com wrote:
What is happening here is that the HID_XFER_PACKET is not a variable length structure, it takes a pointer to the buffer. I am taking a bit of a shortcut and instead of allocating the HID_XFER_PACKET structure and a separate buffer, I am allocating 1 block of memory that is both, the buffer just comes after the HID_XFER_PACKET structure.
But it should still be "sizeof(*packet)" instead of "sizeof(packet)", right?
On 10/19/15 8:27 AM, Henri Verbeet wrote:
On 19 October 2015 at 15:04, Aric Stewart aric@codeweavers.com wrote:
What is happening here is that the HID_XFER_PACKET is not a variable length structure, it takes a pointer to the buffer. I am taking a bit of a shortcut and instead of allocating the HID_XFER_PACKET structure and a separate buffer, I am allocating 1 block of memory that is both, the buffer just comes after the HID_XFER_PACKET structure.
But it should still be "sizeof(*packet)" instead of "sizeof(packet)", right?
Thanks! This is correct. Patch coming soon.
-aric
On 19.10.2015 15:04, Aric Stewart wrote:
On 10/17/15 8:34 AM, Marcus Meissner wrote:
On Sat, Oct 17, 2015 at 03:31:24PM +0200, Sebastian Lackner wrote:
On 17.10.2015 14:52, Marcus Meissner wrote:
1327477 Wrong sizeof argument
Signed-off-by: Marcus Meissner marcus@jet.franken.de
dlls/hidclass.sys/device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c index e7e7e11..dcc7d3c 100644 --- a/dlls/hidclass.sys/device.c +++ b/dlls/hidclass.sys/device.c @@ -277,7 +277,7 @@ static DWORD CALLBACK hid_device_thread(void *args) packet->reportId = 0;
irp = IoBuildDeviceIoControlRequest(IOCTL_HID_GET_INPUT_REPORT,
device, NULL, 0, packet, sizeof(packet), TRUE, events[0],
device, NULL, 0, packet, sizeof(*packet)+ext->preparseData->caps.InputReportByteLength, TRUE, events[0], &irp_status); irpsp = IoGetNextIrpStackLocation(irp);This looks wrong, you have to allocate a separate buffer, and then copy it (like in the code below). I don't know why some of these buffers are HEAP_ZERO_MEMORY though, and others not? Adding Aric, he might want to review this part again, especially since Coverity detected more issues in this code (unnecessary assignment of "rc" for example).
something seems wrong though here, yes.
rc issue is "CID 1327478 Unused value"
Ciao, Marcus
What is happening here is that the HID_XFER_PACKET is not a variable length structure, it takes a pointer to the buffer. I am taking a bit of a shortcut and instead of allocating the HID_XFER_PACKET structure and a separate buffer, I am allocating 1 block of memory that is both, the buffer just comes after the HID_XFER_PACKET structure.
I am pretty sure Marcus' original patch is unnecessary. I am not surprised that there are issues in this code area though. It when through a lot of revision and tweaking as I was getting things working and that often results in cruft.
-aric
I still don't think its correct. The IOCTL_HID_GET_INPUT_REPORT expects a raw data buffer, without any header in front of it, right? Then you would have to pass packet->reportBuffer (and the correct size) to IoBuildDeviceIoControlRequest, but that doesn't work because the buffer will be deallocated automatically after the request is done.
On 10/19/15 9:08 AM, Sebastian Lackner wrote:
On 19.10.2015 15:04, Aric Stewart wrote:
On 10/17/15 8:34 AM, Marcus Meissner wrote:
On Sat, Oct 17, 2015 at 03:31:24PM +0200, Sebastian Lackner wrote:
On 17.10.2015 14:52, Marcus Meissner wrote:
1327477 Wrong sizeof argument
Signed-off-by: Marcus Meissner marcus@jet.franken.de
dlls/hidclass.sys/device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c index e7e7e11..dcc7d3c 100644 --- a/dlls/hidclass.sys/device.c +++ b/dlls/hidclass.sys/device.c @@ -277,7 +277,7 @@ static DWORD CALLBACK hid_device_thread(void *args) packet->reportId = 0;
irp = IoBuildDeviceIoControlRequest(IOCTL_HID_GET_INPUT_REPORT,
device, NULL, 0, packet, sizeof(packet), TRUE, events[0],
device, NULL, 0, packet, sizeof(*packet)+ext->preparseData->caps.InputReportByteLength, TRUE, events[0], &irp_status); irpsp = IoGetNextIrpStackLocation(irp);This looks wrong, you have to allocate a separate buffer, and then copy it (like in the code below). I don't know why some of these buffers are HEAP_ZERO_MEMORY though, and others not? Adding Aric, he might want to review this part again, especially since Coverity detected more issues in this code (unnecessary assignment of "rc" for example).
something seems wrong though here, yes.
rc issue is "CID 1327478 Unused value"
Ciao, Marcus
What is happening here is that the HID_XFER_PACKET is not a variable length structure, it takes a pointer to the buffer. I am taking a bit of a shortcut and instead of allocating the HID_XFER_PACKET structure and a separate buffer, I am allocating 1 block of memory that is both, the buffer just comes after the HID_XFER_PACKET structure.
I am pretty sure Marcus' original patch is unnecessary. I am not surprised that there are issues in this code area though. It when through a lot of revision and tweaking as I was getting things working and that often results in cruft.
-aric
I still don't think its correct. The IOCTL_HID_GET_INPUT_REPORT expects a raw data buffer, without any header in front of it, right? Then you would have to pass packet->reportBuffer (and the correct size) to IoBuildDeviceIoControlRequest, but that doesn't work because the buffer will be deallocated automatically after the request is done.
The confusion here, and I remember it well, is that IOCTL_HID_GET_INPUT_REPORT is used twice, one from user->hidclass IOCTL and again from hidclass->minidriver IOCTL, and the IOCTL parameters are different depending on usage.
This case is the minidriver IOCTL: https://msdn.microsoft.com/en-us/library/windows/hardware/ff541126%28v=vs.85...
The minidriver IOCTL uses the HID_XFER_PACKET structure.
-aric
On 19.10.2015 17:20, Aric Stewart wrote:
On 10/19/15 9:08 AM, Sebastian Lackner wrote:
On 19.10.2015 15:04, Aric Stewart wrote:
On 10/17/15 8:34 AM, Marcus Meissner wrote:
On Sat, Oct 17, 2015 at 03:31:24PM +0200, Sebastian Lackner wrote:
On 17.10.2015 14:52, Marcus Meissner wrote:
1327477 Wrong sizeof argument
Signed-off-by: Marcus Meissner marcus@jet.franken.de
dlls/hidclass.sys/device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c index e7e7e11..dcc7d3c 100644 --- a/dlls/hidclass.sys/device.c +++ b/dlls/hidclass.sys/device.c @@ -277,7 +277,7 @@ static DWORD CALLBACK hid_device_thread(void *args) packet->reportId = 0;
irp = IoBuildDeviceIoControlRequest(IOCTL_HID_GET_INPUT_REPORT,
device, NULL, 0, packet, sizeof(packet), TRUE, events[0],
device, NULL, 0, packet, sizeof(*packet)+ext->preparseData->caps.InputReportByteLength, TRUE, events[0], &irp_status); irpsp = IoGetNextIrpStackLocation(irp);This looks wrong, you have to allocate a separate buffer, and then copy it (like in the code below). I don't know why some of these buffers are HEAP_ZERO_MEMORY though, and others not? Adding Aric, he might want to review this part again, especially since Coverity detected more issues in this code (unnecessary assignment of "rc" for example).
something seems wrong though here, yes.
rc issue is "CID 1327478 Unused value"
Ciao, Marcus
What is happening here is that the HID_XFER_PACKET is not a variable length structure, it takes a pointer to the buffer. I am taking a bit of a shortcut and instead of allocating the HID_XFER_PACKET structure and a separate buffer, I am allocating 1 block of memory that is both, the buffer just comes after the HID_XFER_PACKET structure.
I am pretty sure Marcus' original patch is unnecessary. I am not surprised that there are issues in this code area though. It when through a lot of revision and tweaking as I was getting things working and that often results in cruft.
-aric
I still don't think its correct. The IOCTL_HID_GET_INPUT_REPORT expects a raw data buffer, without any header in front of it, right? Then you would have to pass packet->reportBuffer (and the correct size) to IoBuildDeviceIoControlRequest, but that doesn't work because the buffer will be deallocated automatically after the request is done.
The confusion here, and I remember it well, is that IOCTL_HID_GET_INPUT_REPORT is used twice, one from user->hidclass IOCTL and again from hidclass->minidriver IOCTL, and the IOCTL parameters are different depending on usage.
This case is the minidriver IOCTL: https://msdn.microsoft.com/en-us/library/windows/hardware/ff541126%28v=vs.85...
The minidriver IOCTL uses the HID_XFER_PACKET structure.
-aric
But Henris comment still applies, you want to pass the size of the struct, not the size of a pointer.
On 10/19/15 10:21 AM, Sebastian Lackner wrote:
On 19.10.2015 17:20, Aric Stewart wrote:
On 10/19/15 9:08 AM, Sebastian Lackner wrote:
On 19.10.2015 15:04, Aric Stewart wrote:
On 10/17/15 8:34 AM, Marcus Meissner wrote:
On Sat, Oct 17, 2015 at 03:31:24PM +0200, Sebastian Lackner wrote:
On 17.10.2015 14:52, Marcus Meissner wrote: > 1327477 Wrong sizeof argument > > Signed-off-by: Marcus Meissner marcus@jet.franken.de > --- > dlls/hidclass.sys/device.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c > index e7e7e11..dcc7d3c 100644 > --- a/dlls/hidclass.sys/device.c > +++ b/dlls/hidclass.sys/device.c > @@ -277,7 +277,7 @@ static DWORD CALLBACK hid_device_thread(void *args) > packet->reportId = 0; > > irp = IoBuildDeviceIoControlRequest(IOCTL_HID_GET_INPUT_REPORT, > - device, NULL, 0, packet, sizeof(packet), TRUE, events[0], > + device, NULL, 0, packet, sizeof(*packet)+ext->preparseData->caps.InputReportByteLength, TRUE, events[0], > &irp_status); > > irpsp = IoGetNextIrpStackLocation(irp); >
This looks wrong, you have to allocate a separate buffer, and then copy it (like in the code below). I don't know why some of these buffers are HEAP_ZERO_MEMORY though, and others not? Adding Aric, he might want to review this part again, especially since Coverity detected more issues in this code (unnecessary assignment of "rc" for example).
something seems wrong though here, yes.
rc issue is "CID 1327478 Unused value"
Ciao, Marcus
What is happening here is that the HID_XFER_PACKET is not a variable length structure, it takes a pointer to the buffer. I am taking a bit of a shortcut and instead of allocating the HID_XFER_PACKET structure and a separate buffer, I am allocating 1 block of memory that is both, the buffer just comes after the HID_XFER_PACKET structure.
I am pretty sure Marcus' original patch is unnecessary. I am not surprised that there are issues in this code area though. It when through a lot of revision and tweaking as I was getting things working and that often results in cruft.
-aric
I still don't think its correct. The IOCTL_HID_GET_INPUT_REPORT expects a raw data buffer, without any header in front of it, right? Then you would have to pass packet->reportBuffer (and the correct size) to IoBuildDeviceIoControlRequest, but that doesn't work because the buffer will be deallocated automatically after the request is done.
The confusion here, and I remember it well, is that IOCTL_HID_GET_INPUT_REPORT is used twice, one from user->hidclass IOCTL and again from hidclass->minidriver IOCTL, and the IOCTL parameters are different depending on usage.
This case is the minidriver IOCTL: https://msdn.microsoft.com/en-us/library/windows/hardware/ff541126%28v=vs.85...
The minidriver IOCTL uses the HID_XFER_PACKET structure.
-aric
But Henris comment still applies, you want to pass the size of the struct, not the size of a pointer.
That is correct. I am working on a patch for that, and the unused assignment. Had an e-mail in composition to that regard, but multitasking pulled me away. :)
-aric
-
Aric Stewart -
Henri Verbeet -
Marcus Meissner -
Sebastian Lackner