[PR] os_dev: add boundary check on od_open_ref [mynewt-core]

["94xhn (via GitHub)" <gi...@apache.org>]

94xhn opened a new pull request, #3709:
URL: https://github.com/apache/mynewt-core/pull/3709

   Fixes #1457.
   
   ## Problem
   
   `os_dev_open()`/`os_dev_close()` increment/decrement the `uint8_t od_open_ref` reference counter with no bounds checking.
   
   Calling `os_dev_close()` on a device that is not currently open (e.g. an extra/erroneous close call somewhere in a driver's error-handling path) underflows `od_open_ref` from 0 to 255 instead of being rejected. Because the "clear `OS_DEV_F_STATUS_OPEN`" logic only fires when `od_open_ref` reaches exactly 0, a single spurious close call leaves the device desynchronized for the rest of the program's life: every subsequent, perfectly matched open/close pair keeps missing the `==0` transition (ref wraps 255->0 on the next open, then 0->255 again on the next close), so the device can never be marked closed again.
   
   `os_dev_open()` has the mirroring problem: incrementing `od_open_ref` with no ceiling check could wrap 255->0, again making an actually-open device look closed.
   
   ## Fix
   
   - `os_dev_close()`: reject the call with `OS_EINVAL` when `od_open_ref` is already 0, checked under a critical section before doing any work or calling into the driver's close handler (matching the pattern already used by `os_dev_suspend()`/`os_dev_resume()`).
   - `os_dev_open()`: cap `od_open_ref` at `UINT8_MAX` instead of letting it wrap.
   
   ## Test plan
   
   Verified with a standalone host-side harness that compiles the real, unmodified `kernel/os/src/os_dev.c` (with a couple of headers replaced by minimal stand-ins, since it normally only builds inside the full `newt` build system) against a small test driver, driving exactly the open -> close -> erroneous extra close -> open -> close sequence described above:
   
   - Before this change: the sequence ends with `od_open_ref=255` and `OS_DEV_F_STATUS_OPEN` stuck set after a fully correct, matched final open/close pair.
   - After this change: the extra close is rejected with `OS_EINVAL`, and the final state is correctly `od_open_ref=0` / `OPEN` clear.
   
   ## Disclosure
   
   Generative AI (Claude) was used to help investigate this issue, implement, and verify this fix. All changes were reviewed by me before submission.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@mynewt.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] os_dev: add boundary check on od_open_ref [mynewt-core]

["94xhn (via GitHub)" <gi...@apache.org>]

94xhn commented on PR #3709:
URL: https://github.com/apache/mynewt-core/pull/3709#issuecomment-5342714514

   You're right, that guard didn't make sense in context of the actual bug (#1457) -- it was defending against 256+ unmatched opens, which isn't a scenario that happens with real device usage in this tree, and it wasn't what was reported. Removed it in 60ccaae, keeping just the close-side od_open_ref==0 check, which is what actually fixes the reported underflow-on-double-close.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@mynewt.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] os_dev: add boundary check on od_open_ref [mynewt-core]

["sjanc (via GitHub)" <gi...@apache.org>]

sjanc commented on code in PR #3709:
URL: https://github.com/apache/mynewt-core/pull/3709#discussion_r3683432254


##########
kernel/os/src/os_dev.c:
##########
@@ -229,7 +229,14 @@ os_dev_open(const char *devname, uint32_t timo, void *arg)
     }
 
     OS_ENTER_CRITICAL(sr);
-    ++dev->od_open_ref;
+    /*
+     * Do not let od_open_ref wrap back to 0: that would make the device
+     * look closed (see os_dev_close()) even though it is still held open
+     * by at least one caller.
+     */
+    if (dev->od_open_ref < UINT8_MAX) {
+        ++dev->od_open_ref;
+    }

Review Comment:
   this makes no sense



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@mynewt.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org