Clean up PVS-Studio static-analysis findings for raspberry_pi_pico#3697
Open
hathach wants to merge 5 commits into
Open
Clean up PVS-Studio static-analysis findings for raspberry_pi_pico#3697hathach wants to merge 5 commits into
hathach wants to merge 5 commits into
Conversation
Resolves all TinyUSB-owned alerts reported by the CI PVS-Studio job (static_analysis.yml, run with --security-related-issues) on the raspberry_pi_pico board: 0 remaining in src/ and examples/. Genuine fixes: - ncm_device: validate wNdpIndex against sizeof(nth16_t), not the pointer size sizeof(nth16) (4 bytes) — the latter under-checks the NTB header (V568). - tusb: drop the redundant `ff_buf != NULL && ff_bufsize > 0` guard in tu_edpt_stream_init(); the early return already guarantees it (V560). - midi_host: bounds-check idx in tuh_midi_itf_get_info() instead of the always-true `&_midi_host[idx]` pointer (V560). - examples: fully initialize resolutions_per_format / frame_num / interval_ms arrays instead of leaving trailing elements implicitly zero (V1009). False positives suppressed at the cause: - usbd/usbh: hide the weak dcd_deinit()/hcd_deinit() stubs from the analyzer with #ifndef PVS_STUDIO. PVS analyzes one TU at a time and binds the call to the always-false weak stub (it cannot model the linker selecting the port's strong definition), then reports the cleanup loop after TU_ASSERT(...deinit()) as unreachable (V779). False positives suppressed locally (inline //-V or .pvsconfig): - .pvsconfig: V501 (HID descriptor macros), V763 (rhport override), V785 (audio function-index switch), V1044 (hardware poll loops). - inline //-V for config-dependent or intentional constructs: V512, V514-style contiguous clears, V547, V557, V560, V614, V619, V641, V1008, V1037, V1048, V1086. Verified: all examples build for raspberry_pi_pico; ceedling test:all passes (60/60); re-run of the CI-exact PVS invocation reports zero TinyUSB-owned findings.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR cleans up PVS-Studio static-analysis results for the raspberry_pi_pico CI job by fixing a handful of real defects and adding narrowly-scoped suppressions (inline //-V notes, plus a few global suppressions in .pvsconfig) for cases where PVS can’t correctly model link-time behavior or intentional, config-dependent patterns used in TinyUSB.
Changes:
- Fix correctness issues found by PVS (e.g.,
sizeof(pointer)vssizeof(struct)validation, always-true checks, and example array partial-initialization). - Reduce false positives by hiding always-false weak
*_deinit()stubs from PVS and by adding targeted inline suppressions with rationale. - Add a small set of systemic suppressions to
.PVS-Studio/.pvsconfigfor macro-/config-driven diagnostics that are intentional and recurring.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/tusb.c |
Removes redundant guarding before mutex setup in tu_edpt_stream_init() (early-return already enforces buffer validity). |
src/portable/raspberrypi/rp2040/dcd_rp2040.c |
Adds a localized PVS suppression comment documenting an intentional contiguous memclr. |
src/portable/raspberrypi/pio_usb/hcd_pio_usb.c |
Adds a localized PVS suppression comment explaining root-port indexing. |
src/host/usbh.c |
Hides hcd_deinit() weak stub from PVS and adds localized suppressions for config/linker-analysis limitations. |
src/device/usbd.c |
Hides dcd_deinit() weak stub from PVS to avoid unreachable-code false positives. |
src/class/video/video_device.c |
Adds localized PVS suppressions clarifying intentional estimates / config-dependent branches. |
src/class/usbtmc/usbtmc_device.c |
Adds localized PVS suppressions clarifying intentional header-only clears and bounds rationale. |
src/class/net/ncm_device.c |
Fixes wNdpIndex validation to use sizeof(nth16_t) instead of sizeof(pointer). |
src/class/midi/midi2_host.c |
Adds localized suppression documenting a defensive guard. |
src/class/midi/midi2_device.c |
Adds localized suppressions for defensive/readability guards in chunking/packetization code. |
src/class/midi/midi_host.c |
Replaces an always-non-null pointer check with a real bounds + NULL check (idx < CFG_TUH_MIDI && info != NULL). |
src/class/cdc/cdc_host.c |
Adds localized suppressions documenting intentional constant conditions and safe buffer sizing. |
examples/host/bare_api/src/main.c |
Adds a localized suppression documenting a deliberate cast from raw descriptor bytes. |
examples/device/video_capture_2ch/src/main.c |
Fully initializes per-stream arrays for the 2-stream configuration used by the example. |
examples/device/cdc_uac2/src/uac2_app.c |
Fully initializes per-format resolution array for the example’s two formats. |
.PVS-Studio/.pvsconfig |
Adds a small set of systemic suppressions for macro-/config-driven diagnostics that are intentional across the codebase. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Size Difference ReportBecause TinyUSB code size varies by port and configuration, the metrics below represent the averaged totals across all example builds. Note: If there is no change, only one value is shown. Changes >1% in size
Changes <1% in size
No changes
|
Hardware-in-the-loop (HIL) Test Reporthfp-iar
Legend: ✅ pass · ❌ fail · ⚪ skipped · blank not run hfp.json
Legend: ✅ pass · ❌ fail · ⚪ skipped · blank not run tinyusb.json
Legend: ✅ pass · ❌ fail · ⚪ skipped · blank not run |
|
| target | .text | .rodata | .data | .bss | total | % diff |
|---|---|---|---|---|---|---|
| raspberrypi_cm4/dynamic_configuration | 61,468 → 61,404 (-64) | 4,728 → 696 (-4,032) | — | — | 66,196 → 62,100 (-4,096) | -6.2% |
| sipeed_longan_nano/midi2_device | 55,942 → 55,878 (-64) | — | — | — | 62,528 → 62,464 (-64) | -0.1% |
| nutiny_nuc126v/audio_test_freertos | 16,200 → 16,184 (-16) | — | — | — | 16,360 → 16,344 (-16) | -0.1% |
| frdm_kl25z/audio_test_freertos | 17,648 → 17,632 (-16) | — | — | — | 17,784 → 17,768 (-16) | -0.1% |
| stm32l052dap52/audio_test_freertos | 21,476 → 21,460 (-16) | — | — | — | 22,140 → 22,124 (-16) | -0.1% |
| metro_m0_express/audio_4_channel_mic_freertos | 24,356 → 24,340 (-16) | — | — | — | 24,484 → 24,468 (-16) | -0.1% |
| msp_exp432e401y/uac2_speaker_fb | 14,276 → 14,284 (+8) | — | — | — | 14,848 → 14,856 (+8) | +0.1% |
| stm32c542nucleo/uac2_speaker_fb | 14,912 → 14,920 (+8) | — | — | — | 15,792 → 15,800 (+8) | +0.1% |
| stm32f070rbnucleo/audio_4_channel_mic_freertos | 29,580 → 29,564 (-16) | — | — | — | 31,964 → 31,948 (-16) | -0.1% |
| nutiny_sdk_nuc505/uac2_speaker_fb | — | — | 13,648 → 13,656 (+8) | — | 32,452 → 32,468 (+16) | +0.0% |
Owner
Author
|
The In every failing case the TinyUSB host stack works; the downstream peripherals just didn't appear:
Reasons this isn't a regression from these changes:
A HIL re-run should clear it. Everything else is green (PVS-Studio, CodeQL, SonarCloud, all arm-gcc builds, the other two HIL configs). Generated by Claude Code |
…eter Replace the `rhport = _usbd_rhport;` parameter-rewrite pattern in the 9 usbd_edpt_*/usbd_sof_enable functions with `(void) rhport;` and pass _usbd_rhport directly to the dcd_* calls, matching the existing style of usbd_edpt_claim/release/busy/stalled. This resolves PVS-Studio V763 (parameter always rewritten before use) properly, so drop the global //-V::763 suppression from .pvsconfig. Verified: pico examples rebuild, ceedling test:all 60/60, CI-exact PVS re-run reports zero TinyUSB-owned findings with the suppression removed.
…ressing V641 temp_buf is a uint16_t[128] reused to hold the GET_DESCRIPTOR(Configuration) wire-format blob; casting it to tusb_desc_configuration_t* to read the 9-byte header tripped PVS V641 (buffer size not a multiple of the element size). Route the cast through uintptr_t so the (correct) header read no longer trips the size-ratio heuristic, dropping the inline //-V641 suppression. Verified: pico examples rebuild; single-TU PVS re-run on bare_api/main.c is clean (no V641, no MISRA pointer/integer-cast finding).
Owner
Author
|
Correction to my earlier triage: the Still not caused by this PR: master passed these tests on this PR's base commit ( Generated by Claude Code |
midi2_ump_word_count() is total over the 4-bit message type (every case returns 1..4 words) and callers mask mt with 0x0F, so pkt_bytes can never be 0. Remove the dead break (flagged by PVS V547) instead of suppressing. Verified: pico examples rebuild, ceedling test:all 60/60 (incl. test_midi2_device/test_midi2_host), PVS re-run on both TUs is clean.
The midi2_device/midi2_host tx packing loops advance by this count and no longer carry an explicit == 0 guard (removed in a749fe0).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Brings the CI PVS-Studio job (
.github/workflows/static_analysis.yml, run with--security-related-issues) to zero TinyUSB-owned alerts onraspberry_pi_pico— previously ~43 acrosssrc/andexamples/. Each was triaged individually: real issues fixed in code, false positives suppressed at the narrowest scope that keeps the diagnostic active elsewhere.Genuine fixes
class/net/ncm_device.cwNdpIndexwas validated againstsizeof(nth16)— the pointer size (4 B) — instead ofsizeof(nth16_t)(12 B header). Under-checked the NTB header; now matches the sibling checks on lines 596/604.device/usbd.cusbd_edpt_*/usbd_sof_enablefunctions rewrote theirrhportparameter withrhport = _usbd_rhport;. Now(void) rhport;+ pass_usbd_rhportdirectly to thedcd_*calls, matching the existing style ofusbd_edpt_claim/release/busy/stalled. No suppression needed.tusb.cff_buf != NULL && ff_bufsize > 0guard intu_edpt_stream_init(); the earlyreturn falsealready guarantees it.class/midi/midi_host.ctuh_midi_itf_get_info()tested&_midi_host[idx](always non-NULL). Replaced with a realidx < CFG_TUH_MIDIbound check.examples/.../uac2_app.c,video_capture_2ch/main.cresolutions_per_format/frame_num/interval_ms; trailing elements were silently left zero (a latent example bug for the 2nd format/stream).False positives — fixed at the cause
src/device/usbd.candsrc/host/usbh.cwrap the weakdcd_deinit()/hcd_deinit()stubs in#ifndef PVS_STUDIO. PVS analyzes one translation unit at a time and binds the call to the always-falseweak stub — it cannot model the linker selecting the port's strong definition — then reports the class-driver cleanup loop afterTU_ASSERT(...deinit())as unreachable (V779). Hiding the stub makes the analyzer treat*_deinit()as an ordinary extern, matching link-time reality. (Verified empirically: the V779 disappears with the guard.)False positives — suppressed locally
.pvsconfig(systemic / macro-driven):V501(HID descriptor macros expandproto != HID_ITF_PROTOCOL_NONEto a constant whenNONEis passed),V785(audio function-index switch is constant whenCFG_TUD_AUDIO == 1),V1044(hardware status-register poll loops).//-Vwith rationale for config-dependent / intentional one-offs:V512,V547,V557,V560,V614,V619,V641,V1008,V1037,V1048,V1086.Validation
raspberry_pi_pico(CMake/Ninja, MinSizeRel).ceedling test:all→ 60/60 pass (re-run after each commit).