Opened 7 years ago

Closed 7 years ago

#8308 closed defect (fixed)

Use smartlist-of-strings rather than preallocated-charbuf for more directory stuff

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-auth tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In too many places, we have code that looks like this:

   r = tor_snprintf(s+written, maxlen-s, "foo %s %s\nbar %s\n", baz, qux, quuz);
   if (r<0) {
      goto err;
   }
   written += n;
   ...
   return buf;

or other variants on the theme. This is much easier phrased as

   smartlist_add_asprintf(chunks, "foo %s %s\nbar %s\n", baz, qux, quuz);
   ...
   return smartlist_join_strings(chunks, "", ...);

Making this change will remove a bunch of boilerplate, eliminate some risk of cut-and-paste errors, make a large class of "didn't allocate enough" bugs impossible, and generally make our code easier to read.

Child Tickets

Change History (12)

comment:1 Changed 7 years ago by nickm

Status: newneeds_review

See "less_charbuf" in my public repository for my proposed approach.. Only lightly tested.

comment:2 Changed 7 years ago by asn

Status: needs_reviewneeds_revision

Hm, less_charbuf changes dangerous parts of the code. More review should be needed. Good thing is that the code looks much much cleaner now.

  • crypto_digest_smartlist() has a len_out argument that gets passed to crypto_digest_get_digest() which in turn assumes that it is <= DIGEST256_LEN. Maybe this should be mentioned in the doc of crypto_digest_smartlist() too?
  • There is a DOCDOC lying around.
  • You use:
    crypto_digest_smartlist(digest, digest_len, chunks, "", digest_alg)
    

do you want the NUL in all cases, or do you want:

crypto_digest_smartlist(digest, digest_len, chunks, NULL, digest_alg);

instead? (I'm not sure myself)

  • In some cases where the old code would error out, we now continue. Is this intended? Examples:
-    if (routerstatus_format_entry(outp, endp-outp, &vrs->status,
+    if (routerstatus_format_entry(rs_buf, sizeof(rs_buf), &vrs->status,
                                   vrs->version, NS_V3_VOTE, vrs) < 0) {
-      log_warn(LD_BUG, "Unable to print router status.");
-      goto err;
+      log_warn(LD_BUG, "Unable to print router status; skipping");
+      continue;
-      if (routerstatus_format_entry(outp, endp-outp, &rs, version, NS_V2,
-                                    NULL)) {
-        log_warn(LD_BUG, "Unable to print router status.");
-        tor_free(version);
-        goto done;
+      {
+        char *rsf = routerstatus_format_entry(&rs, version, NS_V2, NULL);
+        if (rsf) {
+          memcpy(outp, rsf, strlen(rsf)+1);
+          outp += strlen(outp);
+          tor_free(rsf);
+        }

Marking as needs_revision for obvious things like the DOCDOC.

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

Status: needs_revisionneeds_review

Replying to asn:

Hm, less_charbuf changes dangerous parts of the code. More review should be needed. Good thing is that the code looks much much cleaner now.

do you want the NUL in all cases, or do you want:

crypto_digest_smartlist(digest, digest_len, chunks, NULL, digest_alg);

instead? (I'm not sure myself)

The NUL isn't included; "" just semeed a cleaner way to specify "append no bytes."

  • In some cases where the old code would error out, we now continue. Is this intended? Examples:

I think that in all the cases I did this, it's right to do so; if one element of a vote or a v2 routerstatus is unformattable, there's no reason to give up IMO.

Fixed the docdoc and the comment.

comment:4 Changed 7 years ago by andrea

Begin code review:

  • 9f044eac77ee2245de71283e71361346ee194f25 looks good to me
  • 72150e6a6d36e1537796cbf279c87b3e6079290e looks good
  • 1859de1cd3b6b5c6737cc36a291eac68f72f5b96 looks good
  • 0607c06ddd8ac3c39148fa58f0b00a5bbeb200a9 looks good
  • b293284110375e1567be7f20a2a0ab1db9c3b35a looks good; the factor of 2 for base64 seems quite extreme and I believe 1.4 would suffice, but memory is abundant and the overallocation is temporary.
  • 185d93d1895f1ace9cde96022b8558faec3b82f7 looks good
  • 14e2fa9801abf18d132c4d6f96f83a4347133420 looks good
  • df49a2a7a77cea7b196879f4641109651906904d looks good
  • e5d828f524b7a729faf1bb20c182ce81a6ba2823 looks good

I concur with nickm that continue rather than goto err is the right thing. One potential issue comes to mind: this removes limits on the length of serialized entities RS_ENTRY_LEN and replaces them with 'however much tor_asprintf()/smartlist_join() can allocate'; are we quite certain this could not be used for a DoS?

In light of the amount of critical code this touches, it should receive intensive testing before being merged.

comment:5 Changed 7 years ago by nickm

Oops; it didn't pass unit tests. How did we all miss that? :( Fixed in the branch now.

I'm going to test it in two ways. First, I'm going to write tests till every line that changed has unit test coverage. (Right now, there appear to be 47 without.) Second, I'm going to do an interop test on a network where some nodes (including 2/4 authorities have this branch merged) and others do not.

Any other tests I should do here?

comment:6 Changed 7 years ago by nickm

Findings:

  • Our tests didn't cover actually making microdesc consensuses with microdesc lines in them. Whoops. Wrote a test.
  • We have no test for generate_v2_networkstatus_opinion(). Right now that function's obsolescent, but we don't want to lose the ability to turn it back on in a hurry. I'll write a quick-and-dirty test.
  • We have no test for routerstatus_getinfo_helper_single. I'll write one.
  • Our test for dumping a routerinfo_t doesn't include a ContactInfo line or an ntor key or an exit policy that policy_write_item would dump, or an ipv6_exit_policy, it seems. I'll try adding those.
  • Our code no longer uses router_get_networkstatus_v3_hash. Removing it.

comment:7 Changed 7 years ago by nickm

Status: needs_reviewneeds_revision

comment:8 Changed 7 years ago by nickm

(Because this is obvious the best place to record the methodology: Run gcov on the before and after tests in separate directories, then use the following perl one-liners:

perl -i -pe 's/^Cs*\d+:/        1:/;' *.gcov
perl -i -pe 's/:\s*\d+:/::/;' *.gcov

This renders the gcov output suitable for passing through "diff -ur". Look for lines matching the pattern /\+ *#/. Those are untested lines that changed.)

comment:9 Changed 7 years ago by nickm

Status: needs_revisionneeds_review

Okay, I fixed some bugs and added a bunch more unit tests. Now there are (IMO) no scary new lines without coverage from this patch. Next, interop tests.

comment:10 Changed 7 years ago by nickm

The interop tests seem okay. I'm inclined to merge.

comment:11 Changed 7 years ago by andrea

I think this one looks okay to me if it passes tests.

comment:12 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Rebased and merged. Thanks for all the review!

Note: See TracTickets for help on using tickets.