Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#5557 closed defect (fixed)

Tor helpers' launch error message contains useless whitespace

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-bridge
Cc: sjmurdoch Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

While playing with #5099, I noticed that the spawn error message that Tor uses to denote "launch failure" usually looks like this:

ERR: Failed to spawn background process - code          9/2

The whitespace between 'code' and '9/2' occurs because format_helper_exit_status() writes the last part of that string in reverse order, after first populating it with whitespace.

That is mainly an aesthetics issue but it also bugs log_from_pipe() since it uses tor_sscanf() to parse the error message, but without taking the whitespace into account:

      retval = tor_sscanf(buf, SPAWN_ERROR_MESSAGE "%x/%x",
                          &child_state, &saved_errno);

Child Tickets

TicketStatusOwnerSummaryComponent
#6225closed"get_string_from_pipe: Assertion len>0 failed; aborting." in log_from_pipe() for tor-fw-helperCore Tor/Tor

Change History (15)

comment:1 Changed 8 years ago by asn

Cc: sjm added

comment:2 Changed 8 years ago by asn

Cc: sjmurdoch added; sjm removed

ugh, it's "sjmurdoch", not "sjm".

comment:3 Changed 8 years ago by asn

I spent too many hours on this bug today. coding different fixes and realizing that they are all ugly.

I ended up preferring these two:

a) Refactor format_helper_exit_status() to create the string from left-to-right, instead of right-to-left. The problem here is the hex encode function, since an integer-to-hex-string (or an itoa()) function is kind of messy (it needs a reverse()). I was thinking that maybe a simpler solution would be better while in the signal-handler realm.

b) Use eat_whitespace() to eat through the useless whitespace, and print the correct substring. This seems like the nicest solution, except from the fact that eat_whitespace() calls tor_assert() which is not signal-handler-safe. I'm thinking of making eat_whitespace_impl() with an extra boolean argument (say, sig_safe) which skips the tor_assert() if set. Then I can macro-ify eat_whitespace_impl() to eat_whitespace() and eat_whitespace_signal_safe().

Nick, any opinions or other ideas?

comment:4 Changed 8 years ago by asn

The other idea is to, instead of filling hex_errno with spaces, fill hex_errno with an invisible ASCII character. This seems stupidly hacky and clumsy though.

I'm wondering what I'm missing...

comment:5 Changed 8 years ago by sjmurdoch

I agree on the aesthetics point, but does this actually cause a bug in log_from_pipe? SPAWN_ERROR_MESSAGE has a trailing space, and the MacOS X manpage says "The format string may also contain other characters. White space (such as blanks, tabs, or newlines) in the format string match any amount of white space, including none, in the input." Can we not assume this on all platforms?

comment:6 in reply to:  5 Changed 8 years ago by asn

Replying to sjmurdoch:

I agree on the aesthetics point, but does this actually cause a bug in log_from_pipe? SPAWN_ERROR_MESSAGE has a trailing space, and the MacOS X manpage says "The format string may also contain other characters. White space (such as blanks, tabs, or newlines) in the format string match any amount of white space, including none, in the input." Can we not assume this on all platforms?

tor_sscanf() doesn't support the whitespace trick. In tor_vsscanf() we have:

    if (*pattern != '%') {
      if (*buf == *pattern) {
        ++buf;
        ++pattern;
        continue;
      } else {
        return n_matched;
      }

Not sure if this is a design choice or a bug.

comment:7 Changed 8 years ago by andrea

Status: newneeds_review

Fixed; format_helper_exit_status() still constructs its output right to left, but pre-computes the exact length rather than space-padding. See my bug5557 branch (https://gitweb.torproject.org/user/andrea/tor.git/shortlog/refs/heads/bug5557).

comment:8 Changed 8 years ago by nickm

Status: needs_reviewneeds_revision

Looks good. A few things to fix up or add on review:

  • changes files should mention bug numbers
  • The hex-number-formatting function should probably get extracted and get a couple of unit tests of its own.
  • We still write our if (...)s without internal spaces inside the parenthesis.
  • Does this break badly if sizeof(int) > 4, and we ever get a huge errno? If so we should check for values more or less than INT32_MIN and INT32_MAX.

Up to you if you want to fix these or if you'd rather I do it.

comment:9 Changed 8 years ago by andrea

Status: needs_revisionneeds_review

This should be portable to systems with arbitrarily large sizeof(int); HEX_ERRNO_SIZE is defined in terms of sizeof(int) in src/common/util.h. All other points addressed in new version.

comment:10 Changed 8 years ago by nickm

Better! Here are a couple of changes I'll make before merging:

In format_hex_number_for_helper_exit_status:

  • The else clause isn't how we usually write them.
  • The function should either nul-terminate its output, or warn loudly in its output that it doesn't NUL-terminate its output.

comment:11 Changed 8 years ago by andrea

Okay. I vote for 'warn loudly'; it doesn't NUL-terminate because it gets called from format_helper_exit_status() to construct part of a larger string that gets terminated in that function.

comment:12 Changed 8 years ago by nickm

Tweaked in my branch "bug5557". Looks ok?

comment:13 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Squashed a little and merged. Thanks!

comment:14 Changed 7 years ago by nickm

Keywords: tor-bridge added

comment:15 Changed 7 years ago by nickm

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