Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some bugfixes on master might have been missed for cherry-picking to 1.1.1 #7397

Closed
mspncp opened this issue Oct 13, 2018 · 7 comments
Closed
Labels
branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch

Comments

@mspncp
Copy link
Contributor

mspncp commented Oct 13, 2018

In #7363 (comment) ff., Kurt and I more or less agreed that practically all bugfixes (including doc-fixes and typos) which apply to 1.1.1 should be cherry-picked, because it is an LTS branch. This inspired me to write a short python script which helps to identify commits that might have been missed for cherry-picking.

The script prints all commits on master which have not (yet) been cherry-picked to OpenSSL_1_1_1-stable, together with their pull request number. It does not attempt to apply any artificial intelligence to distinguish features from bugfixes, it simply lists them all. You will have to check the list yourself.

Fixes which might have been missed

A brief scan of the output yields the following candidates which might have been missed:

prnum commit subject
#7378 c2e33a0 crypto/rand: fix some style nit's ("dirty" cherry-pick; detected by improved script in openssl/tools#32)
#7365 5f9f67b Fix no-engine (@paulidale merged to 1.1.1)
#7360 5b639d4 Indentation fixes. (@paulidale merged to 1.1.1)
#7329 b770a80 Remove useless check. Hash can be longer than EC group degree and it will be truncated. (@paulidale merged to 1.1.1)
#7338 1b39bc9 Fix the drbgtest with randomized ordering (false positive)
#7326 f3002a2 Change DRBG's to DRBGs (false positive)
#7308 7f1d923 Fix no-tls1_2 (done)
#7306 734af93 Fix no-psk (done)

The Python Script

Here is the listing of the cherry-pick-checker script. The major part of the task is achieved by calling git log with some fancy options. Only the pull request number needs special treatment, it is retrieved from the commit message.

#!/usr/bin/env python3

#
# cherry-pick-checker: find all commits eligible for cherry-picking
#

import subprocess
import re

left  = "master"
right = "OpenSSL_1_1_1-stable"

git_log_command = [
    "git", "log", "--oneline", "--cherry-pick", "--left-only",
    left + "..." + right, "--pretty=%h;%s"
]

regex   = re.compile("|".join([
    # The standard pull request annotation
    "\(Merged from https://github.com/openssl/openssl/pull/([0-9]+)\)",
    # @kroeck's special pull request annotation ;-)
    "GH: #([0-9]+)"
]))

print("""
 cherry-pick candidates ({} -> {})

 prnum |   commit   |   subject
 ----- | ---------- | -------------------------------------------""".format(left, right))

for line in subprocess.check_output(git_log_command).decode().splitlines():

    commit, subject = line.split(";")

    # shorten overlong subject lines
    if len(subject) > 70:
        subject = subject[:70] + "..."

    # search commit message for pull request number
    message = subprocess.check_output(
        ["git", "show", "--no-patch", commit]
    ).decode()

    match = regex.search(message)
    if match:
        if match.group(1):
            prnum = match.group(1)
        else:
            prnum = match.group(2)
    else:
        prnum = "????"

    print(' #{} | {} | {} '.format(prnum, commit, subject))

The Output

The output is not only optimized for the terminal, it also has the nice property that it's a valid github markdown table. You can copy & paste it into an issue, switch to preview mode and then jump to the pull requests and the commits for inspection. (See issue description above)

~/src/openssl$ ./cherry-pick-checker

 cherry-pick candidates (master -> OpenSSL_1_1_1-stable)

 prnum |   commit   |   subject
 ----- | ---------- | -------------------------------------------
 #7378 | c2e33a05b1 | crypto/rand: fix some style nit's 
 #7365 | 5f9f67b9d4 | Fix no-engine 
 #7376 | a5fcce6b95 | mkdef: bsd-gcc uses solaris symbol version scripts 
 #7360 | 5b639d4cb3 | Indentation fixes. 
 #7329 | b770a80f6d | Remove useless check. Hash can be longer than EC group degree and it w... 
 #7347 | 36d3acb91d | util/mkdef.pl: for VMS, allow generation of case insensitive symbol ve... 
 #7347 | 05a72c28b2 | Configure: use correct variable to infer the .ld file location 
 #7347 | 66a24ab868 | Add build file support for generic symbol exports with DSOs 
 #7347 | ed57d89bd1 | Change the build of engines to use ordinal files for symbol export 
 #7347 | 97624638b0 | util/mkdef.pl: Produce version scripts from unversioned symbols 
 #7191 | 30699aa194 | Refactor util/mknum.pl for clearer separation of functionality 
 #7191 | 15ba109631 | Add code to manipulate the items in OpenSSL::Ordinals 
 #7191 | d73c44404d | A perl module to parse through C headers 
 #7191 | ab1e5495e4 | Move ZLIB from 'platforms' to 'features' 
 #7191 | 8effd8fa67 | Refactor util/mkdef.pl for clearer separation of functionality 
 #7191 | 91a99748d3 | Add a perl module that deals with ordinals files 
 #7340 | 18958cefd8 | Remove SSL_version_str 
 #7338 | 1b39bc9bcf | Fix the drbgtest with randomized ordering 
 #7339 | a21f4cec14 | Ignore libcrypto.ld and libssl.ld 
 #7326 | f3002a2ed3 | Change DRBG's to DRBGs 
 #6702 | 8ddbff9c08 | 'openssl list': add option -objects to list built in objects 
 #7333 | ef2dfc9902 | Refactor linker script generation 
 #6779 | 8bf3665196 | Added DRBG_HMAC & DRBG_HASH + Added defaults for setting DRBG for mast... 
 #7308 | 7f1d923aa9 | Fix no-tls1_2 
 #7306 | 734af93a27 | Fix no-psk 
 #7249 | 0db957dbbc | Add a GMAC demonstration program. 
 #7208 | f09877c12c | VMS libtestutil: look for lower case "main" 
 #7208 | 2935f6241c | VMS: turn on name mangling for all our programs 
 #7208 | c40af30ec5 | VMS build: fix a misspelled 'bin_cflags' and a wrongly coded 'NO_INST_... 
 #7198 | aa343982d2 | Update the documentation on libobj2shlib / obj2shlib 
 #7198 | f619622715 | VMS: stop trying to build shared libraries from static ones 
 #7159 | 9dfc868025 | Build files: Separate 'lib' intent from 'shlib' intent 
 #7159 | bec2db1809 | Configure: Name object files according to the product they are part of 
 #7159 | 609e4be88e | Configure: DON'T trickle down includes from products to sources 
 #7129 | d6b345708f | Limit the number of AES-GCM keys allowed in TLS.  A new error is raise... 
 #6945 | f88b9b7915 | Speed for HMACs. 
 #7120 | b28bfa7e56 | Add a note to CHANGES indicating that AES-XTS now enforces two differe... 
 #7120 | 95eda4f09a | FIPS 140-2 IG A.9 XTS key check. 
 #7180 | a4a90a8a3b | The next version in master is at least 1.1.2, not 1.1.1x 
@mspncp mspncp added the branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch label Oct 13, 2018
@t-j-h
Copy link
Member

t-j-h commented Oct 14, 2018

You should add the script to the tools repository.

@mspncp
Copy link
Contributor Author

mspncp commented Oct 14, 2018

I thought about that already. Let me finetune it a little more and then I'll make a pull request.

@mspncp
Copy link
Contributor Author

mspncp commented Oct 14, 2018

You should add the script to the tools repository.

See openssl/tools#32.

@kaduk
Copy link
Contributor

kaduk commented Oct 15, 2018

This seems helpful, and thank you for putting it together.
I will just note the existence of the git cherry tool, which looks for the "cherry picked from" lines, and would probably produce somewhat different output than looking for our "Merged from" lines.

@mspncp
Copy link
Contributor Author

mspncp commented Oct 15, 2018

I also experimented also with the git cherry tool when I wrote the script and I don't believe it scans the commit messages for "cherry picked from" lines. Instead, equivalence of patch sets is the criterion, see git-cherry(1):

DESCRIPTION
       Determine whether there are commits in <head>..<upstream> that are equivalent to those 
       in the range <limit>..<head>.

       The equivalence test is based on the diff, after removing whitespace and line numbers.
       git-cherry therefore detects when commits have been "copied" by means of
       git-cherry-pick(1), git-am(1) or git-rebase(1).

My experiments indeed showed that the output of

git cherry master OpenSSL_1_1_1-stable

is essentially equivalent to the output of

git log --cherry --oneline master...OpenSSL_1_1_1-stable

(except for formatting).

After experimenting I chose the symmetric variant

git log --cherry-mark --left-right --oneline

which shows commits on either side which do not have a patch-equivalent commit on the other side. The advantage of the symmetric approach is that it also show for example bug fixes which are only applied to 1.1.1 but not to master. In contrast the two commands git cherry and git log --cherry have a --right-only asymmetry, as you can see from the git-log(1) manpage:

--cherry
A synonym for --right-only --cherry-mark --no-merges; useful to limit the output to the commits
on our side and mark those that have been applied to the other side of a forked history with
git log --cherry upstream...mybranch, similar to git cherry upstream mybranch.

@paulidale
Copy link
Contributor

I've merged #7365 , #7360 and #7329 to 1.1.1 and edited the initial post to indicate this.

@mspncp
Copy link
Contributor Author

mspncp commented Oct 27, 2018

The todo list from the initial post has been completed. For the future, use cherry-checker ;-).

@mspncp mspncp closed this as completed Oct 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch
Projects
None yet
Development

No branches or pull requests

4 participants