Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix length calculation in scsi_command_util::ModeSelect #1447

Closed
wants to merge 2 commits into from

Conversation

kkaempf
Copy link
Contributor

@kkaempf kkaempf commented Apr 1, 2024

OpenVMS Alpha sends a strange ModeSelect payload, apparently one byte too large. This was 'fixed' by a (wrong) length calculation in #1405, breaking #1427.

This PR

Fixes issue #1427

OpenVMS Alpha sends a strange ModeSelect payload, apparently one byte
too large. This was 'fixed' by a (wrong) length calculation in PiSCSI#1405, breaking PiSCSI#1427.

This PR
- fixes the wrong length calculation
- improves the loop test in scsi_command_util::ModeSelect to prevent a
  buffer overflow. (Remaining length was checked for > 0, but buffer
  access is at offset and offset + 1, effectively requiring 2 bytes.)
- the loop test fix makes PiSCSI#1402 pass
- adds a testcase for PiSCSI#1402
- adds a testcase for PiSCSI#1427

Fixes issue PiSCSI#1427

Signed-off-by: Klaus Kämpf <[email protected]>
@uweseimet
Copy link
Contributor

uweseimet commented Apr 1, 2024

@kkaempf I may be wrong, but on first sight this change might not be correct. If the last byte of the payload is a page code (without further potentially required mandatory bytes following) the change seems to ignore this page code. This may result in a wrong (or none at all) SCSI error to be reported for the incomplete page data (incomplete for pages other than page 0).

@kkaempf
Copy link
Contributor Author

kkaempf commented Apr 2, 2024

How about c3f8d94 ?

This just breaks the loop when page code 0 is detected (it's supposed to be last anyway) and checks the (remaining) buffer length before accessing buffer data.

@uweseimet
Copy link
Contributor

That's probably better, but it's up to you to test this and check that it is compliant with the specification, and that in particular there is the correct SCSI error message for pages other than 0. What's definitely not compliant (but maybe not that relevant) is that MODE SELECT supports page 0, but MODE SENSE does not. I have already elaborated on this elsewhere.

I am sorry, but I don't want to spend more time on this.
@kkaempf IMHO you are too focused on getting piscsi to work with your special hardware, but you seem to forget that any new bug may result in piscsi not working anymore for many other users.
@rdmark Regarding the two bug tickets I raised, I would like to bail out ;-). This has become an endless story, with one bug being replaced by the next. Please deal with the bug tickets as you please.

@kkaempf
Copy link
Contributor Author

kkaempf commented Apr 2, 2024

I'm sorry. It was my intention to improve piscsi.

Maybe with better contributors documentation and better test cases things will improve in the future.

@kkaempf kkaempf closed this Apr 2, 2024
@kkaempf
Copy link
Contributor Author

kkaempf commented Apr 2, 2024

@rdmark please remove my contributions to develop. I'll continue in my own branch.

Feel free to reach out in case anyone else has questions on DEC hardware.

@kkaempf kkaempf deleted the issue-1427 branch April 2, 2024 17:41
Copy link

sonarcloud bot commented Apr 2, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
45.5% Coverage on New Code (required ≥ 60%)

See analysis details on SonarCloud

@rdmark
Copy link
Member

rdmark commented Apr 4, 2024

@kkaempf I'm sorry to hear that you're withdrawing your support. I think we were close to fully cross-platform compatible code. The question what to do with vendor specific pages in a compatible way is still open, and will come up again the future I think.

I would appreciate your help reverting the changes, since you're the most familiar with that code. It's up to you if you want to assist with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants