Disabling PSK suites has the desired effect

Please check this, sorry about the increased scope.

Tim, you seem to have found and fixed the leaks in https://codereview.appspot.com/299800043/, is the change in test_io.cc unnecessary based on that?  (I looked but failed to find where the TestAdapter was freed.)

[mt, 2016-04-20 02:15:18]

Adjusting memory management so that it works

[mt, 2016-04-20 02:19:16]

Removing rebase muck

[ekr-rietveld, 2016-04-26 12:59:48]

https://codereview.appspot.com/296860043/diff/40001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/ssl_loopback_unittest.cc (right):

https://codereview.appspot.com/296860043/diff/40001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:1109:
EXPECT_GT(clientCapture->extension().len(), 0UL);
Put the constant first.

https://codereview.appspot.com/296860043/diff/40001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/296860043/diff/40001/lib/ssl/ssl3con.c#newcode968
lib/ssl/ssl3con.c:968: }
I think this is a harmless change, but what's the rationale?

[ttaubert, 2016-04-26 13:09:10]

> Tim, you seem to have found and fixed the leaks in
> https://codereview.appspot.com/299800043/, is the
> change in test_io.cc unnecessary based on that? (I
> looked but failed to find where the TestAdapter was
> freed.)

I don't know for sure, I was able to get rid of the leak with my cset. Did you
hit any new leaks with your patch and that's why you added those changes?

[mt, 2016-04-27 05:53:41]

On 2016/04/26 13:09:10, ttaubert wrote:
> > Tim, you seem to have found and fixed the leaks in
> > https://codereview.appspot.com/299800043/, is the
> > change in test_io.cc unnecessary based on that? (I
> > looked but failed to find where the TestAdapter was
> > freed.)
> 
> I don't know for sure, I was able to get rid of the leak with my cset. Did you
> hit any new leaks with your patch and that's why you added those changes?

OK, here's my testing results using a simple valgrind run.

NSS baseline:
==3892==     in use at exit: 7,475,373 bytes in 18,119 blocks
==3892==    definitely lost: 894,680 bytes in 2,837 blocks
==3892==    indirectly lost: 1,461,324 bytes in 8,439 blocks
==3892==      possibly lost: 23,508 bytes in 47 blocks
==3892==    still reachable: 5,095,861 bytes in 6,796 blocks

With Tim's patch only:
==3745==     in use at exit: 7,473,698 bytes in 18,109 blocks
==3745==    definitely lost: 905,448 bytes in 2,846 blocks
==3745==    indirectly lost: 1,448,881 bytes in 8,420 blocks
==3745==      possibly lost: 23,508 bytes in 47 blocks
==3745==    still reachable: 5,095,861 bytes in 6,796 blocks

With this patch only:
==3141==     in use at exit: 6,222,025 bytes in 9,567 blocks
==3141==    definitely lost: 843,888 bytes in 2,094 blocks
==3141==    indirectly lost: 97,784 bytes in 490 blocks
==3141==      possibly lost: 28,876 bytes in 51 blocks
==3141==    still reachable: 5,251,477 bytes in 6,932 blocks

With both this and Tim's patch:
==3326==     in use at exit: 6,220,671 bytes in 9,560 blocks
==3326==    definitely lost: 847,536 bytes in 2,091 blocks
==3326==    indirectly lost: 92,782 bytes in 486 blocks
==3326==      possibly lost: 28,876 bytes in 51 blocks
==3326==    still reachable: 5,251,477 bytes in 6,932 blocks

Note that this patch adds two tests, so the overall work is slightly higher.  It
still means a big drop in lost blocks.  Tim's changes are more important,
because mine only affect the tests, but this seems like a worthwhile change
based on the above.

BTW, valgrind causes a major slowdown.  But it reports 19G of memory allocations
over 36M discrete allocations.  I had no idea it was that much work.

[mt, 2016-04-27 06:03:19]

Updated and rebased

[mt, 2016-04-27 06:03:30]

https://codereview.appspot.com/296860043/diff/40001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/296860043/diff/40001/lib/ssl/ssl3con.c#newcode968
lib/ssl/ssl3con.c:968: }
On 2016/04/26 12:59:48, ekr-webrtc wrote:
> I think this is a harmless change, but what's the rationale?

It's necessary to have the checks below take effect.  Otherwise the presence
tests we run don't apply to PSK suites.  Now, we don't enable PSK without
enabling TLS 1.3 and we don't enable 1.3 without ECC, so this has no effect now.
 But any changes to the tests in config_match_init will be ignored if we don't
check.

[mt, 2016-04-27 06:04:44]

Ugh, that picked up a ton of changes.

Clean diff here: http://codereview.appspot.com/292550043