Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#8206 closed defect (fixed)

Check return values from fcntl and setsockopt

Reported by: nickm Owned by:
Priority: Low Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay easy
Cc: pe.retzlaff@…, alex@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

These functions are allowed to fail, though in practice they won't (at least, not the way we're using them). Still, our failure to check the return values makes some tools (Coverity scan) unhappy. Let's clean up false positives by checking our 12 unchecked fcntls and our one unchecked setsockopt.

Child Tickets

Attachments (2)

fcntl_setsockopt.diff (6.3 KB) - added by flupzor 7 years ago.
fcntl.diff (818 bytes) - added by flupzor 7 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 7 years ago by piet

Cc: pe.retzlaff@… added

Changed 7 years ago by flupzor

Attachment: fcntl_setsockopt.diff added

comment:2 Changed 7 years ago by flupzor

Cc: alex@… added
Status: newneeds_review

This diff adds checks for return codes of fcntl and setsockopt then logs and exits the connection/function if an error occurs.

Also, the function set_non_blocking function was used wrong. Instead of simply overwriting the file status flags it now retrieves the current file status flags and adds the flag O_NONBLOCKING and then sets the new flags.

comment:3 Changed 7 years ago by nickm

Status: needs_reviewneeds_revision

A lot of those changes leak the file descriptor by returning an error code but never calling close()/CLOSE_SOCKET()/fclose() on the appropriate thing.

Similarly, I don't see how connection_free(conn) can close the fd if "conn->s=fd" hasn't been called yet.

comment:4 Changed 7 years ago by nickm

Status: needs_revisionneeds_review

I've done this up as a branch with some tweaks in branch "bug8206" in my public repository. It could use some review!

comment:5 Changed 7 years ago by nickm

(Should this be targetting 0.2.5 instead?)

comment:6 Changed 7 years ago by asn

Status: needs_reviewneeds_revision

Looks good. Two nitpicks:

In _evdns_nameserver_add_impl() there is an int flags definition that seems unused.

Can you use tor_close_socket() in tor_open_socket() instead of the #if closesocket() #else close() block?

comment:7 Changed 7 years ago by nickm

Status: needs_revisionneeds_review

The evdns code is forked from libevent, and shouldn't diverge too much more.

comment:8 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Oops, you were right.

Fixed, squashed, merged.

comment:9 Changed 7 years ago by flupzor

I'm sorry for my terrible diff. And it seems that i've actually forgotten some more fcntl's

in src/common/util.c in the windows part:
3967 /* Set stdout/stderr pipes to be non-blocking */
3968 fcntl(process_handle->stdout_pipe, F_SETFL, O_NONBLOCK);
3969 fcntl(process_handle->stderr_pipe, F_SETFL, O_NONBLOCK);

And attached is a diff i wrote earlier for something in src/common/compat.c i forgot as well.

I tried writing a diff for the stuff above but i can't concentrate at all at the moment.

Changed 7 years ago by flupzor

Attachment: fcntl.diff added

comment:10 Changed 7 years ago by nickm

applied!

Note: See TracTickets for help on using tickets.