Code review comment for ~utkarsh/ubuntu/+source/libapache2-mod-perl2:fix-sigsev-perl-parse

Revision history for this message
Bryce Harrington (bryce) wrote :

### SRU paperwork review ###

Some tips based on my experience filing SRUs...

The Impact section should be kept super short and concise. It just needs to state the use case that sees the bug, and what the effect is (from a user POV). The first sentence in your Impact section is perfect, you probably don't need to say much more.

The discussion of the debugging work done is great, but should be moved down to a [Discussion] section at the end (or an [Original Report] section if preferred). The SRU review team is less interested in knowing how the bug got fixed, than they are with the other aspects, to from their POV that's all just background detail. For Impact they want to quickly understand "how bad is this bug, and is it worth my attention?"

The [Test Plan] section looks good. I usually direct them to install via 'apt', test, then use 'add-apt-repository -yus <ppa>', upgrade the package and re-test, but this works too.

One possible improvement might be to list using valgrind like in the original report, to show not just that it's crashing, but note exactly where in the code its crashing. I.e.:

    # valgrind apache2 -k start -X
    ...
    ==22529== Invalid read of size 8
    ==22529== at 0x564AFF5: perl_parse (in /usr/lib/x86_64-linux-gnu/libperl.so.5.30.0)
    ...

Reasoning is that there are some situations that crash a certain way, you fix it, and it still crashes but somewhere else or under certain conditions. So being specific can help more quickly isolate those situations when they crop up.

For the [Where problems could occur] section, for crashes like these I usually same some variation of: The patch fixes buffer termination issue and as such things to watch for in testing include the usual pointer handling problems like crashes during startup or runtime, or weird memory related issues.

« Back to merge proposal