Tor helpers' launch error message contains useless whitespace

While playing with legacy/trac#5099 (moved), 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);

changed milestone to %Tor: 0.2.3.x-final in legacy/trac

added component::core tor/tor in Legacy / Trac milestone::Tor: 0.2.3.x-final in Legacy / Trac priority::medium in Legacy / Trac resolution::fixed in Legacy / Trac status::closed in Legacy / Trac tor-bridge in Legacy / Trac type::defect in Legacy / Trac labels

Trac:
Cc: N/A to sjm

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

Trac:
Cc: sjm to sjmurdoch

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?

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...

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?

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.

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).

Trac:
Status: new to needs_review

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.

Trac:
Status: needs_review to needs_revision

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.

Trac:
Status: needs_revision to needs_review

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.

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.

Tweaked in my branch "bug5557". Looks ok?

Squashed a little and merged. Thanks!

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

Trac:
Keywords: N/A deleted, tor-bridge added

Trac:
Component: Tor Bridge to Tor

closed

mentioned in issue legacy/trac#6225 (moved)

moved from legacy/trac#5557 (moved)

added Bug label and removed 1 deleted label

added Bridge label and removed 1 deleted label