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

Check requested transfer size in SCSI_NETWORK_WIFI_CMD_SCAN_RESULTS #174

Closed
wants to merge 1 commit into from

Conversation

th-otto
Copy link
Contributor

@th-otto th-otto commented Jul 24, 2024

See #173

@erichelgeson
Copy link
Contributor

erichelgeson commented Aug 1, 2024

Thanks for taking the time to make this PR @th-otto and apologies it took me a week to get to it. I was building out a test suite around it.

I don't think we should be adjusting the size to the boundary of the next entry. If you look at other SCSI commands that have lists of data, if the user requests a size less than the data, it always chops it off at the requested bytes. Likely due to the client app potentially only allocating a buffer of that size. The client app should know (and I'll document this) the size of the wifi_network_entry and offset of the entries(2) in the data.

Edit: I'm re-thinking this after sleeping on it. Since it's a custom command we can do this and it does make more sense (why would you want a 1/2 a wifi ssid?!). If a BlueSCSI Toolbox app wants 1, 5, or all networks it'll need know the size anyways. It can call with len 2 to know how many are available if really required for saving memory.

I did take a look at @jcs code and whatever we do here should be backwards compatible as he has a of buffer, and requests 2048 bytes.

I think the check condition if size is less than 2 is probably good - you can get no usable info from just getting the first half of the entry size.

It was a good idea to unshadow the size var.

If you have time to make these adjustments, great! If you don't I can cherry pick it down and adjust and merge. Let me know!

@erichelgeson erichelgeson mentioned this pull request Sep 17, 2024
@erichelgeson
Copy link
Contributor

cherry picked and added sense info - will merge under #189 - thanks!

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.

2 participants