Comment 27 for bug 1366174

I've spent a few hours over the last couple of days reviewing this in detail. I've gone over Alex's proposed patch to Trusty carefully, making sure I understand every line in the context of the existing code. I've also carefully gone through upstream's review comments, and upstream's commit to their 2.4 branch (based on a more recent version which necessitated Alex's backport). I've compared upstream's patch to Alex's patch and verified his backport.

I haven't found any issues after quite some time looking for them.

Although the patch is more invasive than I'd like for an SRU, the refactoring he did is the most reasonable way to fix the memory corruption issues in this bug. The only other option to get a more minimal patch would be to leave a memory leak on reload.

This doesn't completely eliminate the regression risk with this kind of update, but I have reviewed as carefully as I can to mitigate it.

+1 for Alex's patch. There are some minor whitespace, comment, constant and name changes that aren't strictly minimal, but I think that given the complexity of the patch it's safer to leave them as-is as they've come directly from the upstream backport. The only code touched is in the areas that needed touching anyway.

I have uploaded this to trusty-proposed and this now awaits review from the SRU team. As Chris Arges looked at this previously, I'll ask him if he can look at this again now.

I recommend that for SRU verification, in addition to checking for crashes, we test SSL functionality carefully - both with and without OCSP certificates if possible.

Top quality work, Alex, especially considering the complexity involved. Thank you.