Opened 7 years ago

Closed 7 years ago

#7192 closed defect (fixed)

parsing exit policy summary drops last character

Reported by: arma Owned by:
Priority: High Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version: Tor: 0.2.3.23-rc
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description


Child Tickets

Change History (12)

comment:1 Changed 7 years ago by arma

see my bug7192 branch for the unit tests that show the bug.

comment:2 Changed 7 years ago by arma

This resolves the issue:

diff --git a/src/or/policies.c b/src/or/policies.c
index 486c264..1f11d36 100644
--- a/src/or/policies.c
+++ b/src/or/policies.c
@@ -1366,8 +1366,8 @@ parse_short_policy(const char *summary)
       continue;
     }
 
-    memcpy(ent_buf, summary, next-summary-1);
-    ent_buf[next-summary-1] = '\0';
+    memcpy(ent_buf, summary, next-summary);
+    ent_buf[next-summary] = '\0';
 
     if (tor_sscanf(ent_buf, "%u-%u%c", &low, &high, &dummy) == 2) {
       if (low<1 || low>65535 || high<1 || high>65535) {

but we probably want to adjust our fencepost counts in the "next-summary > (int)(sizeof(ent_buf)-1)" clause above that too.

comment:3 Changed 7 years ago by arma

Bug reported and tracked by Erik Kline.

comment:4 Changed 7 years ago by arma

I haven't fully explored the implications of the bug, but it looks like a serious 0.2.3-stopper.

comment:5 Changed 7 years ago by nickm

Agreed.

Yuck. Authorities don't parse and use these during voting, do they? If so, we've got to do a really ugly consensus method dance. 0.2.2 doesn't parse these, right?

comment:6 in reply to:  5 Changed 7 years ago by nickm

Replying to nickm:

Agreed.

Yuck. Authorities don't parse and use these during voting, do they? If so, we've got to do a really ugly consensus method dance. 0.2.2 doesn't parse these, right?

Confirming: 0.2.2 doesn't even have parse_short_policy. parse_short_policy is used only when *parsing* microdescriptors. The microdesc voting process doesn't involve looking at the parsed contents of microdescriptors; only at their digests.

comment:7 in reply to:  2 Changed 7 years ago by nickm

Status: newneeds_review

Replying to arma:

This resolves the issue:

Watch out there: next is *either* the closing nul, or the character right after the comma.

Preferred fix in branch "bug7192" in my public repository. Needs careful review and a changes file.

comment:8 Changed 7 years ago by andrea

I think that looks fine; one question though. This was there before, but why the %c in tor_sscanf(ent_buf, "%u-%u%c", &low, &high, &dummy), and then the comparison to 2 (i.e., the %c isn't supposed to match anything)?

comment:9 Changed 7 years ago by nickm

That %c is how you use sscanf to do exact matches rather than prefix matches.

#include <stdio.h>
int main(int argc, char **argv)
{
  unsigned a,b;
  char dummy;

  /* Here we're using %c: notice how we can tell if there are any extra
     characters after the 1-2 based on the return value. */
  printf("%d\n", sscanf("1-2",  "%u-%u%c", &a, &b, &dummy));
  printf("%d\n", sscanf("1-2x", "%u-%u%c", &a, &b, &dummy));

  /* Here we are not using %c: there isn't a way to tell whether we
   * had extra characters after the 1-2 */
  printf("%d\n", sscanf("1-2", "%u-%u", &a, &b));
  printf("%d\n", sscanf("1-2x", "%u-%u", &a, &b));

  return 0;
}

Did that answer the question?

comment:10 Changed 7 years ago by nickm

Added a changes file and some more unit tests.

comment:11 Changed 7 years ago by nickm

Roger said he liked it too, then I added one more unit test to my "bug7192" branch, and made a "bug7192_squashed" branch to clean it up before merge.

comment:12 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merging to 0.2.3 and master

Note: See TracTickets for help on using tickets.