[Bugfix] Padding chars no longer allowed anywhere but the end #31

emilypi · 2020-04-22T17:12:33Z

This is a bit of a surprising bug in base64-bytestring:

> decode "eAo=eAo="
Right "x\n"

> decode "eAo=" ==  decode "eAo=eAo="
True

It stems from the fact that the decode algorithm is just wrong: it does not check that no padding chars occur in the first and second positions of the 32 bytes read off in each step of the decoding process! This PR fixes that, improves error reporting, and also improves performance by separating concerns in the decode routine to consider head, inner loop, and tail separately.

emilypi · 2020-04-22T17:13:34Z

Data/ByteString/Base64/Internal.hs

+            poke8 (plusPtr dst 2) (fromIntegral w)
+            go (plusPtr dst 3) (plusPtr src 4) (n + 3)
+
+    finish !dst !src !n


This unrolls the tail end of the loop (the last 4 bytes to be encoded). Note the difference in padding checks!

you know... comments such as these would be valuable to be included as source-code comments in the source-code... then you wouldn't have to point this out as github-PR-annotations... ;->

B-but. IT'S ALL UNDOCUMENTED HERBERT. I've added comments, but i'm trying to keep with the style of the thing. If we want to add a fuller documentation PR, i'm happy to do it separately.

I can add it to the pile of spring cleanings we need to do in this library.

Data/ByteString/Base64/Internal.hs

emilypi · 2020-04-22T17:31:59Z

Benchmarks with cabal run bench:benchmarks -O2:

Pre-fix:

benchmarking lazy/small/normal/decode
time                 104.7 ns   (101.7 ns .. 107.4 ns)
                     0.997 R²   (0.995 R² .. 0.999 R²)
mean                 101.7 ns   (100.7 ns .. 103.3 ns)
std dev              4.031 ns   (3.019 ns .. 5.765 ns)
variance introduced by outliers: 60% (severely inflated)

benchmarking lazy/small/url/decode
time                 112.0 ns   (110.5 ns .. 114.1 ns)
                     0.998 R²   (0.995 R² .. 0.999 R²)
mean                 112.9 ns   (111.7 ns .. 114.7 ns)
std dev              4.789 ns   (3.393 ns .. 7.550 ns)
variance introduced by outliers: 63% (severely inflated)

benchmarking lazy/medium/normal/decode
time                 3.099 μs   (3.021 μs .. 3.169 μs)
                     0.997 R²   (0.996 R² .. 0.999 R²)
mean                 3.095 μs   (3.053 μs .. 3.161 μs)
std dev              174.2 ns   (128.1 ns .. 284.6 ns)
variance introduced by outliers: 69% (severely inflated)


benchmarking lazy/medium/url/decode
time                 3.222 μs   (3.170 μs .. 3.287 μs)
                     0.998 R²   (0.997 R² .. 0.999 R²)
mean                 3.264 μs   (3.225 μs .. 3.306 μs)
std dev              138.8 ns   (124.3 ns .. 161.8 ns)
variance introduced by outliers: 56% (severely inflated)

benchmarking lazy/large/normal/decode
time                 269.3 μs   (262.9 μs .. 275.5 μs)
                     0.996 R²   (0.994 R² .. 0.998 R²)
mean                 272.4 μs   (268.9 μs .. 277.0 μs)
std dev              13.79 μs   (11.13 μs .. 20.29 μs)
variance introduced by outliers: 48% (moderately inflated)

benchmarking lazy/large/url/decode
time                 270.2 μs   (261.7 μs .. 277.1 μs)
                     0.995 R²   (0.993 R² .. 0.997 R²)
mean                 268.7 μs   (264.5 μs .. 273.6 μs)
std dev              15.92 μs   (13.75 μs .. 18.29 μs)
variance introduced by outliers: 56% (severely inflated)

benchmarking strict/small/normal/decode
time                 40.25 ns   (39.32 ns .. 41.41 ns)
                     0.997 R²   (0.996 R² .. 0.999 R²)
mean                 40.67 ns   (40.19 ns .. 41.19 ns)
std dev              1.677 ns   (1.427 ns .. 2.096 ns)
variance introduced by outliers: 64% (severely inflated)

benchmarking strict/small/url/decode
time                 40.33 ns   (39.86 ns .. 40.98 ns)
                     0.998 R²   (0.997 R² .. 0.999 R²)
mean                 40.57 ns   (40.10 ns .. 41.15 ns)
std dev              1.627 ns   (1.240 ns .. 2.050 ns)
variance introduced by outliers: 62% (severely inflated)

benchmarking strict/medium/normal/decode
time                 2.515 μs   (2.488 μs .. 2.540 μs)
                     0.999 R²   (0.999 R² .. 1.000 R²)
mean                 2.502 μs   (2.481 μs .. 2.522 μs)
std dev              63.60 ns   (51.59 ns .. 82.13 ns)
variance introduced by outliers: 31% (moderately inflated)

benchmarking strict/medium/url/decode
time                 2.458 μs   (2.431 μs .. 2.492 μs)
                     0.999 R²   (0.999 R² .. 0.999 R²)
mean                 2.506 μs   (2.487 μs .. 2.527 μs)
std dev              70.56 ns   (58.89 ns .. 88.78 ns)
variance introduced by outliers: 36% (moderately inflated)

benchmarking strict/large/normal/decode
time                 192.6 μs   (187.4 μs .. 195.9 μs)
                     0.997 R²   (0.997 R² .. 0.998 R²)
mean                 192.0 μs   (189.6 μs .. 194.0 μs)
std dev              7.637 μs   (6.680 μs .. 8.715 μs)
variance introduced by outliers: 38% (moderately inflated)

benchmarking strict/large/url/decode
time                 185.1 μs   (184.1 μs .. 185.9 μs)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 185.3 μs   (184.3 μs .. 186.3 μs)
std dev              3.320 μs   (2.776 μs .. 4.100 μs)
variance introduced by outliers: 11% (moderately inflated)

Post-fix:

benchmarking lazy/small/normal/decode
time                 100.1 ns   (98.92 ns .. 101.2 ns)
                     0.999 R²   (0.999 R² .. 1.000 R²)
mean                 99.11 ns   (98.45 ns .. 99.89 ns)
std dev              2.359 ns   (1.981 ns .. 2.750 ns)
variance introduced by outliers: 35% (moderately inflated)

benchmarking lazy/small/url/decode
time                 105.5 ns   (104.0 ns .. 107.5 ns)
                     0.998 R²   (0.997 R² .. 0.999 R²)
mean                 106.4 ns   (104.9 ns .. 107.8 ns)
std dev              4.717 ns   (3.985 ns .. 5.710 ns)
variance introduced by outliers: 65% (severely inflated)

benchmarking lazy/medium/normal/decode
time                 2.848 μs   (2.833 μs .. 2.863 μs)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 2.836 μs   (2.822 μs .. 2.848 μs)
std dev              44.06 ns   (36.87 ns .. 53.06 ns)
variance introduced by outliers: 14% (moderately inflated)

benchmarking lazy/medium/url/decode
time                 2.850 μs   (2.820 μs .. 2.877 μs)
                     0.999 R²   (0.999 R² .. 1.000 R²)
mean                 2.850 μs   (2.833 μs .. 2.872 μs)
std dev              67.69 ns   (52.47 ns .. 102.6 ns)
variance introduced by outliers: 28% (moderately inflated)

benchmarking lazy/large/normal/decode
time                 262.2 μs   (255.4 μs .. 269.4 μs)
                     0.996 R²   (0.994 R² .. 0.999 R²)
mean                 250.7 μs   (247.0 μs .. 254.8 μs)
std dev              13.06 μs   (10.46 μs .. 16.85 μs)
variance introduced by outliers: 49% (moderately inflated)

benchmarking lazy/large/url/decode
time                 250.5 μs   (243.9 μs .. 257.5 μs)
                     0.994 R²   (0.988 R² .. 0.998 R²)
mean                 251.9 μs   (247.8 μs .. 258.5 μs)
std dev              17.30 μs   (12.38 μs .. 24.43 μs)
variance introduced by outliers: 64% (severely inflated)


benchmarking strict/small/normal/decode
time                 36.72 ns   (36.46 ns .. 37.01 ns)
                     0.999 R²   (0.999 R² .. 1.000 R²)
mean                 37.08 ns   (36.63 ns .. 37.86 ns)
std dev              2.011 ns   (1.344 ns .. 2.910 ns)
variance introduced by outliers: 76% (severely inflated)

benchmarking strict/small/url/decode
time                 37.98 ns   (37.23 ns .. 38.75 ns)
                     0.998 R²   (0.997 R² .. 0.999 R²)
mean                 36.99 ns   (36.57 ns .. 37.47 ns)
std dev              1.547 ns   (1.279 ns .. 1.869 ns)
variance introduced by outliers: 65% (severely inflated)

benchmarking strict/medium/normal/decode
time                 2.300 μs   (2.246 μs .. 2.341 μs)
                     0.998 R²   (0.998 R² .. 0.999 R²)
mean                 2.257 μs   (2.236 μs .. 2.280 μs)
std dev              69.26 ns   (53.85 ns .. 87.08 ns)
variance introduced by outliers: 40% (moderately inflated)

benchmarking strict/medium/url/decode
time                 2.326 μs   (2.299 μs .. 2.354 μs)
                     0.999 R²   (0.999 R² .. 1.000 R²)
mean                 2.299 μs   (2.280 μs .. 2.317 μs)
std dev              64.95 ns   (55.00 ns .. 79.01 ns)
variance introduced by outliers: 36% (moderately inflated)

benchmarking strict/large/normal/decode
time                 174.4 μs   (172.1 μs .. 177.7 μs)
                     0.997 R²   (0.996 R² .. 0.999 R²)
mean                 173.8 μs   (171.6 μs .. 176.6 μs)
std dev              8.122 μs   (6.410 μs .. 10.07 μs)
variance introduced by outliers: 47% (moderately inflated)

benchmarking strict/large/url/decode
time                 170.1 μs   (168.8 μs .. 171.7 μs)
                     0.999 R²   (0.999 R² .. 1.000 R²)
mean                 171.0 μs   (169.8 μs .. 172.4 μs)
std dev              4.207 μs   (3.289 μs .. 5.595 μs)
variance introduced by outliers: 19% (moderately inflated)

hvr

here's some preliminary comments... I understand you noticed some issues in finish-part yourself?

Data/ByteString/Base64/Internal.hs

hvr · 2020-04-22T21:08:05Z

Data/ByteString/Base64/Internal.hs

+            poke8 (plusPtr dst 2) (fromIntegral w)
+            go (plusPtr dst 3) (plusPtr src 4) (n + 3)
+
+    finish !dst !src !n


you know... comments such as these would be valuable to be included as source-code comments in the source-code... then you wouldn't have to point this out as github-PR-annotations... ;->

Data/ByteString/Base64/Internal.hs

emilypi · 2020-04-22T23:11:23Z

@hvr all of your comments have been addressed. I factored out -XMultiWayIf and any unnecessary casees, removed the use of a counter, and documented.

emilypi · 2020-04-22T23:25:56Z

Looks like addressing @hvr's comments also managed to achieve better perf:

benchmarking lazy/small/normal/decode
time                 111.1 ns   (110.3 ns .. 112.3 ns)
                     0.999 R²   (0.999 R² .. 0.999 R²)
mean                 112.4 ns   (111.1 ns .. 113.8 ns)
std dev              4.560 ns   (3.827 ns .. 5.798 ns)
variance introduced by outliers: 61% (severely inflated)

benchmarking lazy/small/url/decode
time                 111.3 ns   (110.4 ns .. 112.2 ns)
                     0.999 R²   (0.999 R² .. 1.000 R²)
mean                 110.6 ns   (110.1 ns .. 111.5 ns)
std dev              2.324 ns   (1.699 ns .. 3.566 ns)
variance introduced by outliers: 29% (moderately inflated)

benchmarking lazy/medium/normal/decode
time                 2.801 μs   (2.787 μs .. 2.824 μs)
                     0.999 R²   (0.999 R² .. 1.000 R²)
mean                 2.811 μs   (2.791 μs .. 2.841 μs)
std dev              82.77 ns   (56.08 ns .. 123.2 ns)
variance introduced by outliers: 38% (moderately inflated)

benchmarking lazy/medium/url/decode
time                 2.921 μs   (2.888 μs .. 2.944 μs)
                     0.999 R²   (0.999 R² .. 0.999 R²)
mean                 2.852 μs   (2.823 μs .. 2.881 μs)
std dev              89.48 ns   (76.02 ns .. 105.1 ns)
variance introduced by outliers: 41% (moderately inflated)

benchmarking lazy/large/normal/decode
time                 231.6 μs   (230.1 μs .. 233.8 μs)
                     0.999 R²   (0.999 R² .. 1.000 R²)
mean                 237.9 μs   (236.2 μs .. 240.4 μs)
std dev              7.031 μs   (5.704 μs .. 9.307 μs)
variance introduced by outliers: 24% (moderately inflated)

benchmarking lazy/large/url/decode
time                 233.9 μs   (232.6 μs .. 235.1 μs)
                     1.000 R²   (0.999 R² .. 1.000 R²)
mean                 234.5 μs   (233.3 μs .. 236.5 μs)
std dev              5.003 μs   (3.529 μs .. 7.886 μs)
variance introduced by outliers: 14% (moderately inflated)

benchmarking strict/small/normal/decode
time                 38.37 ns   (38.21 ns .. 38.59 ns)
                     1.000 R²   (1.000 R² .. 1.000 R²)
mean                 38.45 ns   (38.30 ns .. 38.67 ns)
std dev              596.8 ps   (411.3 ps .. 894.0 ps)
variance introduced by outliers: 20% (moderately inflated)

benchmarking strict/small/url/decode
time                 40.20 ns   (39.49 ns .. 40.84 ns)
                     0.999 R²   (0.998 R² .. 1.000 R²)
mean                 39.26 ns   (38.87 ns .. 39.63 ns)
std dev              1.284 ns   (982.8 ps .. 1.749 ns)
variance introduced by outliers: 52% (severely inflated)

benchmarking strict/medium/normal/decode
time                 2.069 μs   (2.058 μs .. 2.082 μs)
                     1.000 R²   (0.999 R² .. 1.000 R²)
mean                 2.075 μs   (2.064 μs .. 2.088 μs)
std dev              41.06 ns   (34.93 ns .. 50.19 ns)
variance introduced by outliers: 22% (moderately inflated)

benchmarking strict/medium/url/decode
time                 2.050 μs   (2.038 μs .. 2.062 μs)
                     1.000 R²   (0.999 R² .. 1.000 R²)
mean                 2.057 μs   (2.044 μs .. 2.072 μs)
std dev              48.59 ns   (39.75 ns .. 61.17 ns)
variance introduced by outliers: 29% (moderately inflated)

benchmarking strict/large/normal/decode
time                 157.9 μs   (156.2 μs .. 159.6 μs)
                     0.999 R²   (0.999 R² .. 0.999 R²)
mean                 158.9 μs   (157.9 μs .. 160.0 μs)
std dev              3.674 μs   (3.208 μs .. 4.353 μs)
variance introduced by outliers: 18% (moderately inflated)

benchmarking strict/large/url/decode
time                 164.0 μs   (162.1 μs .. 166.0 μs)
                     0.998 R²   (0.996 R² .. 0.999 R²)
mean                 158.8 μs   (157.0 μs .. 161.4 μs)
std dev              7.097 μs   (5.337 μs .. 11.20 μs)
variance introduced by outliers: 44% (moderately inflated)

hvr

lgtm now! :-)

fix hvr's bug

c1ec3eb

emilypi commented Apr 22, 2020

View reviewed changes

Data/ByteString/Base64/Internal.hs Outdated Show resolved Hide resolved

tests

b56062e

emilypi requested review from 23Skidoo and hvr April 22, 2020 18:01

hvr reviewed Apr 22, 2020

View reviewed changes

emilypi added 4 commits April 22, 2020 17:59

remove dead cases

4e059fe

refactor for multiwayif removal

ca9f1b9

address hvr's comments

d01aa3d

address hvr's comments

2a6be87

7.2 + 7.0 compat

ce9ede5

hvr approved these changes Apr 23, 2020

View reviewed changes

set bounds on bytestring

1baa4a3

emilypi merged commit f5d1bba into master Apr 23, 2020

emilypi deleted the emily/fix-decode-validity branch April 23, 2020 20:48

emilypi mentioned this pull request Jun 5, 2020

RFC: Should Stringly-typed messages be replaced with Ints? #37

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Bugfix] Padding chars no longer allowed anywhere but the end #31

[Bugfix] Padding chars no longer allowed anywhere but the end #31

Uh oh!

emilypi commented Apr 22, 2020 •

edited

Loading

Uh oh!

emilypi Apr 22, 2020

Uh oh!

hvr Apr 22, 2020

Uh oh!

emilypi Apr 22, 2020 •

edited

Loading

Uh oh!

Uh oh!

emilypi commented Apr 22, 2020 •

edited

Loading

Uh oh!

hvr left a comment

Uh oh!

Uh oh!

hvr Apr 22, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

emilypi commented Apr 22, 2020

Uh oh!

emilypi commented Apr 22, 2020 •

edited

Loading

Uh oh!

hvr left a comment

Uh oh!

Uh oh!

[Bugfix] Padding chars no longer allowed anywhere but the end #31

[Bugfix] Padding chars no longer allowed anywhere but the end #31

Uh oh!

Conversation

emilypi commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emilypi Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

hvr Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

emilypi Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

emilypi commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hvr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hvr Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

emilypi commented Apr 22, 2020

Uh oh!

emilypi commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hvr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

emilypi commented Apr 22, 2020 •

edited

Loading

emilypi Apr 22, 2020 •

edited

Loading

emilypi commented Apr 22, 2020 •

edited

Loading

emilypi commented Apr 22, 2020 •

edited

Loading