Opened 7 years ago

Closed 7 years ago

Last modified 19 months ago

#5969 closed defect (fixed)

A couple of compile warnings with clang 3.1

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

Description

First one is

crypto.c:1888:38: error: size argument in 'strlcpy' call appears to be size of
      the source; expected the size of the destination
      [-Werror,-Wstrlcpy-strlcat-size]
    strlcpy(fname_new, fname, strlen(fname) + 8);
                              ~~~~~~~^~~~~~~~~~

second one is

compat.c:362:28: error: format string is not a string literal
      [-Werror,-Wformat-nonliteral]
  r = vsnprintf(str, size, format, args);
      ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
/usr/include/secure/_stdio.h:72:63: note: expanded from macro 'vsnprintf'
  __builtin___vsnprintf_chk (str, len, 0, __darwin_obsz(str), format, ap)
                                                              ^
compat.c:412:32: error: format string is not a string literal
      [-Werror,-Wformat-nonliteral]
  int r = vasprintf(&strp_tmp, fmt, args);
                               ^~~

Child Tickets

Change History (15)

comment:1 Changed 7 years ago by Sebastian

Component: - Select a componentTor Client
Milestone: Tor: 0.2.2.x-final
Status: newneeds_review

A patch for the first one is in branch bug5969 in my repo, I don't know right away how to fix the second one. My branch goes on top of master, the second warning happens in maint-0.2.2 tho.

comment:2 Changed 7 years ago by nickm

Status: needs_reviewnew

For the first one, I'm going to be a little more radical and just make it use tor_asprintf() instead. Committed that as a5a8296892441bf435 .

For the second, I think all we can do is look for a way to suppress the warning. It is completely correct that a function implementing vaprintf should take a format string as an argument as opposed to passing a constant string.

comment:3 Changed 7 years ago by nickm

Status: newneeds_review

I think I've got a possible fix; see bug5969_022 in my public repo. I have made sure it builds, but haven't tested it with clang 3.1. It tells the compiler that the v*printf/v*scanf functions receive a format string, which may suffice to keep clang from warning in this case.

comment:4 Changed 7 years ago by Sebastian

Unfortunately that's not yet the whole fix, these two errors get introduced by it:

log.c:273:37: error: format string is not a string literal [-Werror,-Wformat-nonliteral]
  r = tor_vsnprintf(buf+n,buf_len-n,format,ap);
                                    ^~~~~~

control.c:619:29: error: format string is not a string literal [-Werror,-Wformat-nonliteral]
  len = tor_vasprintf(&buf, format, ap);
                            ^~~~~~

comment:5 Changed 7 years ago by nickm

Added the correct fixes/workarounds for those too. Same branch (bug5969_022). Does it work now, or did we turn up something else?

comment:6 Changed 7 years ago by Sebastian

it works now. You introduce a check-spaces error in log.c tho, maybe you want to amend the patch before merging it.

comment:7 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Fixed and merged and merged-forward. thanks!

comment:8 Changed 7 years ago by arma

Resolution: fixed
Status: closedreopened

comment:9 Changed 7 years ago by arma

Priority: normalmajor

Commit 3a9351b57e52 includes

-  if (tor_snprintf(format_buf, sizeof(format_buf), "650 %s %s %s\r\n",
-                   status, sev, format)<0) {
+  if (tor_snprintf(format_buf, sizeof(format_buf), "650 %s %s\r\n",
+                   status, sev)<0) {
[...]
+  tor_vasprintf(&user_buf, format, args);

Before, 'format' was before the \r\n. After, it is after.

This causes Tor to output malformed control events, make Vidalia freak out and freeze.

comment:10 Changed 7 years ago by arma

Priority: majorblocker

This is a bug in 0.2.3.16-alpha (released).

For 0.2.2, it is not released yet (whew). We should either fix this is maint-0.2.2, or revert the whole thing from maint-0.2.2. Probably fixing it is better at this point, along with carefully looking over the rest.

In the future, we should use this as a lesson for why backporting non-critical things to stable code is a poor plan.

comment:11 Changed 7 years ago by arma

Ok, chiiph opened #6094 for this bug too. We should probably close this one and fix the new bug in #6094.

comment:12 Changed 7 years ago by nickm

Resolution: fixed
Status: reopenedclosed

Merged and closed #6094.

BTW, in general, when the fix for a ticket is bad in a way that other than "you didn't fix the original problem", I'd much prefer a new ticket to a reopened ticket. It's not a problem in this case, but for tickets that don't get fixed right away, I get confused when the title and description of the ticket have nothing to do with the problem that the ticket represents.

comment:13 Changed 7 years ago by nickm

Keywords: tor-client added

comment:14 Changed 7 years ago by nickm

Component: Tor ClientTor

comment:15 Changed 19 months ago by teor

Severity: Normal

Set all tickets without a severity to "Normal"

Note: See TracTickets for help on using tickets.