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

🐛registry_get_plugged_type uses incorrect bounds-check for port index. #7

Open
NoahK216 opened this issue Feb 19, 2025 · 0 comments
Labels
sub-issue a smaller issue that's part of a larger item

Comments

@NoahK216
Copy link

Referring to purduesigbots/pros#623

Describe the bug
This is a fun one.

  • pros::c::registry_get_plugged_type supposedly takes a zero-indexed smart port index and returns the assocaited v5_device_e_t.
  • The function supposedly returns ENXIO (errno 6) if the provided port is out of range (docs claim the range is 0-20, which makes sense, given there are 21 smart ports and this argument is zero-indexed).
    • Despite this, the docs never describe what's actually returned when errno is set.
    • From looking at source code, the function actually returns -1 to intentionally overflow the return value (v5_device_e_t -- which the compiler assumes is a unsigned 8-bit integer) to 255 (which is the value of E_DEVICE_UNDEFINED).

Okay, so when you pass an out of bounds port, registry_get_plugged_type will return E_DEVICE_UNDEFINED (255 overflowed from -1) and set errno to ENXIO, right? Nope! Specifically when requesting port 21's plugged type (a supposedly out-of-bounds port), pros::c::registry_get_plugged_type will return 12, which is E_DEVICE_ADI. Huh?

  • It seems like registry_get_plugged_type bound-checks the port number against VALIDATE_PORT_NO, which uses a max input value of 22. This would work fine if the argument wasn't a zero-indexed port, but in this case it allows us to index into registry_types one port index above the intended maximum. For some reason, this happens to be 12.
    • I can only speculate at the actual reason, but I'm assuming this is due to the registry being initialized using V5_MAX_DEVICE_PORTS which has some internal V5 device access stuff after the regular smart port indexes, and vexDeviceGetStatus happens to store INTERNAL_ADI_PORT at index 21 (which is the brain's "internal ADI expander").

To Reproduce

pros::c::registry_get_plugged_type(21) == 12; // true!

Expected behavior

pros::c::registry_get_plugged_type(21) == E_DEVICE_UNDEFINED;
errno == ENXIO;

Screenshots
N/A

Desktop (please complete the following information):

  • OS: Windows 11 23H2 (26020.1000)

Additional context
Tested on PROS 3.8.0

This bug very likely applies to other device registry-related functions in apix.h, as well as some internal ones probably. To list a few:

  • registry_get_bound_type
  • registry_unbind_port (wonder what happens when I try to unbind the internal ADI slots)
  • registry_bind_port
@NoahK216 NoahK216 added the bug label Feb 19, 2025
@SizzinSeal SizzinSeal changed the title 🐛[BUG] - registry_get_plugged_type uses incorrect bounds-check for port index. 🐛registry_get_plugged_type uses incorrect bounds-check for port index. Feb 20, 2025
@SizzinSeal SizzinSeal added this to the No More Upstream Bugs milestone Feb 23, 2025
@SizzinSeal SizzinSeal added sub-issue a smaller issue that's part of a larger item and removed bug labels Feb 24, 2025
@SizzinSeal SizzinSeal removed this from the No More Upstream Bugs milestone Feb 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sub-issue a smaller issue that's part of a larger item
Projects
None yet
Development

No branches or pull requests

2 participants