Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 39317 Build 36206: arc lint + arc unit
Event Timeline
Current coccinelle patch
@@ type T; identifier V; expression PATH; expression PRIO; @@ T V; ... - memset(&V, 0, sizeof(V)); ... - xpt_setup_ccb(&V.ccb_h, PATH, PRIO) + xpt_setup_stack_ccb(&V.ccb_h, sizeof(V), PATH, PRIO) @@ type T; identifier V; expression PATH; expression PRIO; @@ T V; ... - bzero(&V, sizeof(V)); ... - xpt_setup_ccb(&V.ccb_h, PATH, PRIO) + xpt_setup_stack_ccb(&V.ccb_h, sizeof(V), PATH, PRIO) @@ type T; identifier V; expression PATH; expression PRIO; @@ T V; ... - xpt_setup_ccb(&V.ccb_h, PATH, PRIO) + xpt_setup_stack_ccb(&V.ccb_h, sizeof(V), PATH, PRIO)
Oh, this works better
// Script to find all the uses of ccb's allocated on the stack and change // them to use the new setup function which bzeros before doing a normal // setup of the CCB. @@ identifier V; expression PATH; expression PRIO; @@ - memset(&V, 0, sizeof(V)); ... - xpt_setup_ccb(&V.ccb_h, PATH, PRIO) + xpt_setup_stack_ccb(&V.ccb_h, sizeof(V), PATH, PRIO) @@ identifier V; expression PATH; expression PRIO; @@ - bzero(&V, sizeof(V)); ... - xpt_setup_ccb(&V.ccb_h, PATH, PRIO) + xpt_setup_stack_ccb(&V.ccb_h, sizeof(V), PATH, PRIO) @@ identifier V; expression PATH; expression PRIO; @@ - xpt_setup_ccb(&V.ccb_h, PATH, PRIO) + xpt_setup_stack_ccb(&V.ccb_h, sizeof(V), PATH, PRIO)
updated the patch based on this
Found five errors by hand at least, but unsure how to automate it.
| sys/cam/ata/ata_da.c | ||
|---|---|---|
| 1121–1122 | this is a bug, this should be deleted. | |
| sys/cam/cam_xpt.c | ||
| 502 | this is a bug, but we dump the path into into this ccb, and then dump the path into it, then setup things, which is a pattern not otherwise done. | |
| sys/cam/scsi/scsi_target.c | ||
| 275 | also an ordering bug | |
| sys/dev/sym/sym_hipd.c | ||
| 3428 | ordering bug. | |
| 3449–3450 | hmmm, this one is a bug... | |
Tweaked the script to replace the bzero/memset with the new function.
// Script to find all the uses of ccb's allocated on the stack and change // them to use the new setup function which bzeros before doing a normal // setup of the CCB. @@ identifier V; expression PATH; expression PRIO; @@ - memset(&V, 0, sizeof(V)); + xpt_setup_stack_ccb(&V.ccb_h, sizeof(V), PATH, PRIO); ... - xpt_setup_ccb(&V.ccb_h, PATH, PRIO); @@ identifier V; expression PATH; expression PRIO; @@ - bzero(&V, sizeof(V)); + xpt_setup_stack_ccb(&V.ccb_h, sizeof(V), PATH, PRIO); ... - xpt_setup_ccb(&V.ccb_h, PATH, PRIO); @@ identifier V; expression PATH; expression PRIO; @@ - xpt_setup_ccb(&V.ccb_h, PATH, PRIO); + xpt_setup_stack_ccb(&V.ccb_h, sizeof(V), PATH, PRIO);
The latest script still has 2 bugs: ata_da.c (extra memset) and cam_xpt.c (need to create a temp path like is done elsewhere but aren't)
How does cocinelle know the CCB is from the stack, as opposed to being allocated dynamically?
Because the script uses a variable, not a pointer, to do the matching. This can distinguish between
a pointer that's cast about vs one that's on the stack. My script is a bit primitive, though, in that
it doesn't find xpt_setup_ccb((struct ccb_hdr *)&stack), but I couldn't find any of those in the tree.
Coiccinelle has a fairly sophisticated semantic matching algorithm. It's main use is migrating APIs
in the linux kernel, which is why the output is linux kernel normal form...