Opened 10 months ago

Closed 7 months ago

#24484 closed defect (fixed)

free(NULL) always works (nowadays) so stop acting like it might not

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

Description

src/common/util.h has the following text in a comment:

 * Unlike the free() function, tor_free() will still work on NULL pointers,
 * and it sets the pointer value to NULL after freeing it.

free(NULL) has been required to be a no-op since C89 at least. Some pre-ANSI platforms might have had a libc free() that didn't allow a null pointer, but we mostly don't care about those.

We should stop propagating this common misconception that free(NULL) might undefined on modern platforms. We should remove that text from the comment and remove the conditional from the implementation.

Child Tickets

Change History (12)

comment:1 Changed 10 months ago by catalyst

Summary: free(NULL) always works (nowadays) so delete comment saying that it might notfree(NULL) always works (nowadays) so stop acting like it might not

comment:2 Changed 10 months ago by aruna1234

so, this requires a change in the comments?
What are the new comments that are required?

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

Replying to aruna1234:

so, this requires a change in the comments?
What are the new comments that are required?

It requires both a comment change and a code change. tor_free() and possibly related functions and macros conditionally call the real free() function when it should be unconditional. They should instead unconditionally call free().

We should fix or remove existing comments that suggest or imply that free(NULL) is invalid in modern C. In the original description, I quoted one of the comments that needs changing. Perhaps you will find others.

Please be aware that #24337 touches a lot of related code so you might want to wait until it's merged before working on this. (nickm, what do you think is best here for sequencing?)

comment:4 Changed 10 months ago by nickm

IMO, best to take this after the #24337 changes go in.

comment:5 Changed 10 months ago by aruna1234

okay! thanks!!

comment:6 Changed 9 months ago by teor

We should do this after we decide how to resolve #24733.

comment:7 Changed 8 months ago by nickm

Owner: set to nickm
Status: newaccepted

comment:8 Changed 8 months ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final
Status: acceptedneeds_review

See my branch bug24484

comment:9 Changed 8 months ago by catalyst

Status: needs_reviewneeds_revision

Code changes look good! The autoconf test is a great idea, too. Please also delete the part of the tor_free() comment that implies that free(NULL) won't work.

comment:10 Changed 8 months ago by nickm

Status: needs_revisionneeds_review

ok, updated.

comment:11 Changed 8 months ago by catalyst

Status: needs_reviewmerge_ready

Thanks! Looks good to me now.

comment:12 Changed 7 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merging to master, which is now 0.3.4!

Note: See TracTickets for help on using tickets.