search

arpruss / USBComposite_stm32f1

USB Composite library for STM32F1 (HID, Serial, MIDI and XBox360 controller)
Other
382 stars 76 forks source link

MassStorage scsi_read_memory bug #66

Closed goobur closed 4 years ago

goobur commented 4 years ago

Packets from the mass storage device are being incorrectly constructed. As a simple test, open the mass example, write a text file of 512 bytes to the mounted disk, unmount it, then remount it and read the file --- you should see the bug manifest itself.

It looks as the library is skipping a block of MAX_BULK_PACKET_SIZE after it sends the first. Funny enough, this bug doesn't prevent the device from mounting or presenting its FAT partition since enough data in each sector remains in-tact for the OS to recognize the drive/partition. Moreover, people likely haven't noticed this bug since their respective OS will likely cache writes (and reads) to the device (and will also avoid reading back sectors.)

A simple fix is to move usb_mass_sil_write(SCSI_dataBuffer + SCSI_blockOffset, MAX_BULK_PACKET_SIZE); to line 277 in usb_scsi.c, but I don't know a lot about USB, and I don't know if that's a comprehensive fix (although it works for me).

I'm concerned that there would be a similar bug in writing to files, which could corrupt whatever media a user is connected to, but I haven't seen that issue in my limited testing.

arpruss commented 4 years ago

I know next to nothing about SCSI.

I fixed what you suggested, but I am worried if there isn't a similar problem in the if (SCSI_blockReadCount == 0) branch (lines 270-275). What do you think?

goobur commented 4 years ago

I think it should be fine -- the bug is resulting from the fact that the 512-byte sectors have to be chunked up into multiple packets to be transmitted via USB. The problem with the code before was that for any chunk after the first, the function was incrementing the buffer, /then/ sending the data from the new buffer, but the behaviour in the first chunk also increments the buffer --- this is why it was skipping MAX_BULK_PACKET_SIZE bytes. By changing the order of operations we avoid that behaviour. The if statement you're referring to is the case where you transmit the first chunk of the 512 bytes, which was (and still is) being transmitted properly, so I think it should be OK.

goobur commented 4 years ago

As a side-thought, this might have also fixed issue #28