Opened 5 years ago

Last modified 17 months ago

#8787 new defect

Check return values for more unix functions

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-client tor-relay posix easy correctness safety
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Reportedly, we lack checks for the return values of at least munmap, lseek, unlink. We should fix that for code-quality.

Child Tickets

Change History (16)

comment:1 Changed 5 years ago by nickm

Add strftime to the list of stuff whose return value we should technically be checking.

comment:2 Changed 5 years ago by andrea

My bug8787 branch now has this for munmap(), lseek() and unlink(). For strftime(), the most straightforward way would be to make format_rfc1123_time(), format_iso_time() and format_iso_local_time() able to pass on an error return, but these are called from 88 distinct sites in src/or/*.c. Are we sure changing all of those is the right approach?

comment:3 Changed 5 years ago by andrea

Status: newneeds_review

Setting this to needs_review; if we decide the answer to the above is 'just slog through and deal with it' put it in needs_revision.

comment:4 Changed 5 years ago by andrea

Keywords: 025-triaged added

comment:5 Changed 5 years ago by nickm

35aca52324d835eb1895cb769130a00cde067b3f -- I think 'res' should be block-local to lower the chances of our confusing it with 'r'. Also, should we really return -1 when unlink fails in finish_writing_to_file_impl?

f0bc9874fd28a2ff0f839a16ead514be51cfde25 -- It's scary to have tor_munmap_file() fail but to keep the mapped thing around. In fact, why return -1?

1fb049d2433dfed207d4655114421bbdef9a2e6d -- Our convention on FOO_free(NULL) is to treat it as a no-op. Why not have the same convention for tor_munmap_file() ?

-- Also, You can check the return value of UnmapViewOfFile to see whether it succeeded: http://msdn.microsoft.com/en-us/library/windows/desktop/aa366882(v=vs.85).aspx

comment:6 Changed 5 years ago by nickm

Do you like the fixup commits I made on my branch bug8787 (based on yours)? If so let's squash and merge.

comment:7 Changed 5 years ago by andrea

Yeah, I think these fixups are fine if you're happy with it now.

comment:8 Changed 5 years ago by andrea

What about strftime? Should we make a separate ticket to figure that out or keep this one open?

comment:9 Changed 5 years ago by nickm

Status: needs_reviewnew

comment:10 Changed 5 years ago by nickm

Merged yesterday, plus a few commits.

Wrt strftime, I think we can not do much more for now than ignore failures in those format_*_time functions. I'd be okay with considering a bigger refactor for 0.2.??? or 0.2.6, or with just closing this ticket for now; your call.

comment:11 Changed 5 years ago by andrea

Milestone: Tor: 0.2.5.x-finalTor: 0.2.???

For strftime, I think the only likely cause of failures is output too large for the buffer, and it's too big a refactor to check the returns in 0.2.5. I'm sending this to 0.2.?? for now, and at some point we should do one of the following:

1.) Refactor so we can check strftime() returns

or

2.) Audit each call and be sure the buffer is always large enough, and then decide that suffices and close this.

comment:12 Changed 2 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:13 Changed 22 months ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:14 Changed 17 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:15 Changed 17 months ago by nickm

Keywords: 025-triaged removed

remove an old triage keyword.

comment:16 Changed 17 months ago by nickm

Keywords: tor-relay posix easy correctness safety added
Severity: Normal
Note: See TracTickets for help on using tickets.