Closed Bug 614489 Opened 9 years ago Closed 9 years ago

Base 32 decoder for Sync backend

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file)

Attached patch Base32 decoder.Splinter Review
Bug to track review of Base32 decoder, needed for new approach to key derivation in simple crypto.
Attachment #492920 - Flags: review?(philipp)
Comment on attachment 492920 [details] [diff] [review]
Base32 decoder.

>+    function processBlock(ret, cOffset, rOffset) {
>+      let c, ch;

<3 Only one 1-letter variable!!1! My constant whining must've gotten through :p

>+      
>+      function accumulate(val) {
>+        ret[rOffset] |= val;
>+      }

ret[rOffset] will not always exist yet (see comment on array constructor below), so it will be undefined. So essentially we're relying on undefined | number == number in JavaScript. *cringe* Please document in comment :)

>+      
>+      function solidify() {
>+        ret[rOffset] = String.fromCharCode(ret[rOffset]);
>+        ++rOffset;
>+      }

So ret first contains integers, then solidify() turns them into strings... confusing (and not necessarily faster or more efficient...) Suggestion: Get rid of solidify(), inline the ++rOffset; below and do the conversion to string at the end. See below.

>+      
>+      function advance() {
>+        c  = str.charAt(cOffset++);

c = str[cOffset++];

>+        if (c == "" || c == "=") // Easier than range checking.
>+          throw "Done";          // Will be caught far away.

Ew. Is this really necessary? Can't the loop surrounding processBlock() be smarter about this?

>+    // Our output.
>+    let ret = new Array(bytes);

(Note that this way of initializing arrays does in no way mean imply that ret[i] with i < bytes will have a value, it will still be undefined. Also, it does not guarantee that the array grows no more than bytes. That's why this style of initializing array is usually frowned upon, but I guess it's ok here. JS Arrays are weird.)

>+    // Slice in case our shift overflowed to the right.
>+    return ret.slice(0, bytes).join("");

If ret were always an array of integers, do

  return [String.fromCharCode(byte) for each (byte in ret.slice(0, bytes))].join("");

here.

>+  // Decoding.
>+  do_check_eq(Utils.decodeBase32(""), "");
>+  do_check_eq(Utils.decodeBase32("MY======"), "f");
>+  do_check_eq(Utils.decodeBase32("MZXQ===="), "fo");
>+  do_check_eq(Utils.decodeBase32("MZXW6YTB"), "fooba");
>+  do_check_eq(Utils.decodeBase32("MZXW6YTBOI======"), "foobar");
>+  
>+  do_check_eq(Utils.decodeBase32(Utils.encodeBase32("Bacon is a vegetable.")),
>+              "Bacon is a vegetable.");
> }

You implemented code paths for a couple of other scenarios that should be tested here as well

* non-base32 characters throw an exception

* proper base32 string but missing or wrong amount of padding is processed just fine

r=me with those changes. (If you can find a more elegant solution for the throw "Done" thing, excellent. If not, we'll can always refactor/prettify later as long as the test coverage is good.)
Attachment #492920 - Flags: review?(philipp) → review+
(In reply to comment #1)

> <3 Only one 1-letter variable!!1! My constant whining must've gotten through :p

:)

> ret[rOffset] will not always exist yet (see comment on array constructor
> below), so it will be undefined. So essentially we're relying on undefined |
> number == number in JavaScript. *cringe* Please document in comment :)

Will do.
 
> So ret first contains integers, then solidify() turns them into strings...
> confusing (and not necessarily faster or more efficient...) Suggestion: Get rid
> of solidify(), inline the ++rOffset; below and do the conversion to string at
> the end. See below.

This is actually how I did it before submitting, but I noticed that "I'm done" and "move to the next one" curiously occurred right next to each other... and once done with a byte, we never come back to it. Seems like a great time to do finalizing acts, like char-ifying.

 
> >+      
> >+      function advance() {
> >+        c  = str.charAt(cOffset++);
> 
> c = str[cOffset++];

Was trying to be explicit about stringiness, but sure.

> 
> >+        if (c == "" || c == "=") // Easier than range checking.
> >+          throw "Done";          // Will be caught far away.
> 
> Ew. Is this really necessary? Can't the loop surrounding processBlock() be
> smarter about this?

Doing it this way allows us to handle input with or without padding, and without any flags for 'done'. The alternative to the former is to pad the string prior to processBlock, but that still needs padding detection.

Using exceptions for flow control isn't very conventional in JS, I know, but we don't have goto :)

Think of this as Python's StopIteration.

> JS Arrays are weird.)

Yup. Again, just being explicit (and maybe giving some hints to the JS environment).

> If ret were always an array of integers, do...

Will do.

> You implemented code paths for a couple of other scenarios that should be
> tested here as well

Gotcha.

Thanks for the review!
The implementation for this is in Bug 603489, particularly this attachment:

https://bug603489.bugzilla.mozilla.org/attachment.cgi?id=493181

No point in breaking it out.

Leaving as ASSIGNED until that patch drops.
Patch approved in Bug 603489, so I'm closing this.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [qa-]
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.