Opened 6 months ago

Closed 8 weeks ago

#30743 closed enhancement (fixed)

Write a coccinelle script to catch increment/decrement calls inside log_debug().

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 042-can asn-merge
Cc: Actual Points:
Parent ID: Points:
Reviewer: catalyst Sponsor:

Description

See #30628 for motivation

Child Tickets

Change History (10)

comment:1 Changed 5 months ago by nickm

Here's a script that catches one case, and the other 3 cases are easy enough to infer...

@@
expression E;
@@
*log_debug(... , <+... E-- ...+>, ... );

The problem, though, is that a lot of our files don't parse for coccinelle right now, because of our macro madness. We'll need to extend scripts/coccinelle/tor-coccinelle.h till everything works.

comment:2 Changed 2 months ago by nickm

Pfew, this has gotten complicated. I've opened another ticket to improve our coccinelle tooling: #31705.

comment:3 Changed 2 months ago by nickm

Status: assignedneeds_review

See branch ticket30743 with PR at https://github.com/torproject/tor/pull/1322 . Running it over the source code with the apply.sh script in #31705 found no additional instances of bugs like #30628.

comment:4 Changed 2 months ago by asn

Reviewer: catalyst

comment:5 in reply to:  3 Changed 2 months ago by catalyst

Status: needs_reviewneeds_information

Replying to nickm:

See branch ticket30743 with PR at https://github.com/torproject/tor/pull/1322 . Running it over the source code with the apply.sh script in #31705 found no additional instances of bugs like #30628.

Thanks! Mostly looks good. I manually verified that it detects the bug fixed in #30628.

Do you think it needs a changes file? I think it would be useful it the changes file (and commit message) pointed at an example of a bug that this would catch.

comment:6 Changed 2 months ago by nickm

Status: needs_informationneeds_review

Thanks for the feedback! I've added a changes file and significantly expanded the commit message and the comment at the head of the spatch file. I've mentioned #30628 in the changes file and the commit message. How does it look to you now?

comment:7 Changed 2 months ago by nickm

Keywords: 042-can added

comment:8 in reply to:  6 Changed 2 months ago by catalyst

Status: needs_reviewmerge_ready

Replying to nickm:

Thanks for the feedback! I've added a changes file and significantly expanded the commit message and the comment at the head of the spatch file. I've mentioned #30628 in the changes file and the commit message. How does it look to you now?

Thanks! Looks good now.

comment:9 Changed 2 months ago by nickm

Keywords: asn-merge added

comment:10 Changed 8 weeks ago by asn

Resolution: fixed
Status: merge_readyclosed

Merged!

Note: See TracTickets for help on using tickets.