tor/doc/HACKING/HowToReview.md

How to review a patch

Some folks have said that they'd like to review patches more often, but they don't know how.

So, here are a bunch of things to check for when reviewing a patch!

Note that if you can't do every one of these, that doesn't mean you can't do a good review! Just make it clear what you checked for and what you didn't.

Top-level smell-checks

(Difficulty: easy)

• Does it compile with --enable-fatal-warnings?

• Does make check-spaces pass?

• Does make check-changes pass?

• Does it have a reasonable amount of tests? Do they pass? Do they leak memory?

• Do all the new functions, global variables, types, and structure members have documentation?

• Do all the functions, global variables, types, and structure members with modified behavior have modified documentation?

• Do all the new torrc options have documentation?

• If this changes Tor's behavior on the wire, is there a design proposal?

• If this changes anything in the code, is there a "changes" file?

Let's look at the code!

• Does the code conform to CodingStandards.txt?

• Does the code leak memory?

• If two or more pointers ever point to the same object, is it clear which pointer "owns" the object?

• Are all allocated resources freed?

• Are all pointers that should be const, const?

• Are #defines used for 'magic' numbers?

• Can you understand what the code is trying to do?

• Can you convince yourself that the code really does that?

• Is there duplicated code that could be turned into a function?

Let's look at the documentation!

• Does the documentation confirm to CodingStandards.txt?

• Does it make sense?

• Can you predict what the function will do from its documentation?

Let's think about security!

• If there are any arrays, buffers, are you 100% sure that they cannot overflow?

• If there is any integer math, can it overflow or underflow?

• If there are any allocations, are you sure there are corresponding deallocations?

• Is there a safer pattern that could be used in any case?

• Have they used one of the Forbidden Functions?

(Also see your favorite secure C programming guides.)