Opened 9 years ago

Closed 9 years ago

Last modified 7 years ago

#3035 closed defect (fixed)

fix a few clang-static analyzer complaints in 0.2.2

Reported by: Sebastian Owned by:
Priority: Medium Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I started working on a branch to fix some clang complaints. Here's a checkpoint progress because a release is supposed to happen soon

Child Tickets

Change History (15)

comment:1 Changed 9 years ago by Sebastian

Milestone: Tor: 0.2.2.x-final
Status: newneeds_review

comment:2 Changed 9 years ago by Sebastian

eh, branchname clang_fixes in my repo

comment:3 Changed 9 years ago by arma

 #define CONN_LOG_PROTECT(conn, stmt)                                    \
   STMT_BEGIN                                                            \
-    int _log_conn_is_control = (conn && conn->type == CONN_TYPE_CONTROL); \
+    int _log_conn_is_control;                                           \
+    tor_assert(conn);                                                   \
+    _log_conn_is_control = (conn->type == CONN_TYPE_CONTROL);           \

What if tor_assert triggers, causing us to do a log to the controller at the same time, which would want to disable logging, ...

comment:4 Changed 9 years ago by arma

+  /* Determine the N most occured build times */

You wanted to say occurred. But actually that's not good either. Perhaps 'common'?

comment:5 Changed 9 years ago by arma

+      if (cur && (cur_n > most_n ||
+          (cur_n == most_n && cur->status.published_on > most_published))) {

would be more readable formatted as

+      if (cur && (cur_n > most_n ||
+                  (cur_n == most_n &&
+                   cur->status.published_on > most_published))) {

We try to line up the parts of the clause that are grouped by the parens.

comment:6 Changed 9 years ago by arma

   memset(addrbuf,0,sizeof(addrbuf));
-  dest_addr = (struct sockaddr*) addrbuf;
   dest_addr_len = tor_addr_to_sockaddr(addr, port, dest_addr, sizeof(addrbuf));

looks fine, but Nick may prefer (for readability) to actually remove the first time we assign to it, rather than this time. I'll leave that up to Nick.

comment:7 Changed 9 years ago by arma

The rest looks fine to me. Thanks!

comment:8 in reply to:  3 ; Changed 9 years ago by Sebastian

Replying to arma:

 #define CONN_LOG_PROTECT(conn, stmt)                                    \
   STMT_BEGIN                                                            \
-    int _log_conn_is_control = (conn && conn->type == CONN_TYPE_CONTROL); \
+    int _log_conn_is_control;                                           \
+    tor_assert(conn);                                                   \
+    _log_conn_is_control = (conn->type == CONN_TYPE_CONTROL);           \

What if tor_assert triggers, causing us to do a log to the controller at the same time, which would want to disable logging, ...

I don't see the issue here, maybe I am silly? We haven't yet stopped logging to the controller, so the assert will get logged (we don't disable logging to the controller until after the assert, and tor_assert doesn't disable control logging) and die.

comment:9 Changed 9 years ago by Sebastian

I fixed all the other issues you mentioned.

comment:10 in reply to:  8 ; Changed 9 years ago by arma

Replying to Sebastian:

What if tor_assert triggers, causing us to do a log to the controller at the same time, which would want to disable logging, ...

I don't see the issue here, maybe I am silly? We haven't yet stopped logging to the controller, so the assert will get logged (we don't disable logging to the controller until after the assert, and tor_assert doesn't disable control logging) and die.

Yeah. I dunno. Maybe? I know tor_assert logs when it triggers, and I think it also tries to flush to the controller.

I'm not smart enough right now to be confident that this is safe. And if we currently think the tor_assert is never triggered, but we don't know what would happen if it is, we're basically putting down a landmine for our future selves.

If Nick wants to merge it, go ahead.

comment:11 in reply to:  10 Changed 9 years ago by Sebastian

Replying to arma:

Replying to Sebastian:

What if tor_assert triggers, causing us to do a log to the controller at the same time, which would want to disable logging, ...

I don't see the issue here, maybe I am silly? We haven't yet stopped logging to the controller, so the assert will get logged (we don't disable logging to the controller until after the assert, and tor_assert doesn't disable control logging) and die.

Yeah. I dunno. Maybe? I know tor_assert logs when it triggers, and I think it also tries to flush to the controller.

Yes, it does log but doesn't call CONN_LOG_PROTECT() again, so an infinite loop isn't possible here.

I'm not smart enough right now to be confident that this is safe. And if we currently think the tor_assert is never triggered, but we don't know what would happen if it is, we're basically putting down a landmine for our future selves.

I'm not saying our behavior is safe because we don't think we're currently not triggering the assert

comment:12 Changed 9 years ago by nickm

re 4a02eb37c97e: I'm pretty sure we don't change the changelogs for releases that have already happened.

re the change to connection_or.c : If you're removing the out += len, you should also change the following *out++ = 0; to just be *out = 0; , IMO

Otherwise looks fine. Resolving those, squashing, and merging.

comment:13 Changed 9 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

comment:14 Changed 7 years ago by nickm

Keywords: tor-client added

comment:15 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.