-Waggressive-loop-optimizations UB warning in to_words52 on 3.0.x, but not 3.1.x/master

[thesamesam]

Config:

$ perl configdata.pm --dump

Command line (with current working directory = .):

    /usr/bin/perl ./Configure

Perl information:

    /usr/bin/perl
    5.36.1 for x86_64-linux

Enabled features:

    afalgeng
    aria
    asm
    async
    autoalginit
    autoerrinit
    autoload-config
    bf
    blake2
    bulk
    cached-fetch
    camellia
    capieng
    cast
    chacha
    cmac
    cmp
    cms
    comp
    ct
    deprecated
    des
    dgram
    dh
    dsa
    dso
    dtls
    dynamic-engine
    ec
    ec2m
    ecdh
    ecdsa
    engine
    err
    filenames
    gost
    idea
    legacy
    loadereng
    makedepend
    md4
    mdc2
    module
    multiblock
    nextprotoneg
    ocb
    ocsp
    padlockeng
    pic
    pinshared
    poly1305
    posix-io
    psk
    rc2
    rc4
    rdrand
    rfc3779
    rmd160
    scrypt
    secure-memory
    seed
    shared
    siphash
    siv
    sm2
    sm3
    sm4
    sock
    srp
    srtp
    sse2
    ssl
    ssl-trace
    static-engine
    stdio
    tests
    threads
    tls
    ts
    ui-console
    whirlpool
    tls1
    tls1-method
    tls1_1
    tls1_1-method
    tls1_2
    tls1_2-method
    tls1_3
    dtls1
    dtls1-method
    dtls1_2
    dtls1_2-method

Disabled features:

    acvp-tests          [cascade]        OPENSSL_NO_ACVP_TESTS
    asan                [default]        OPENSSL_NO_ASAN
    buildtest-c++       [default]
    crypto-mdebug       [default]        OPENSSL_NO_CRYPTO_MDEBUG
    devcryptoeng        [default]        OPENSSL_NO_DEVCRYPTOENG
    ec_nistp_64_gcc_128 [default]        OPENSSL_NO_EC_NISTP_64_GCC_128
    egd                 [default]        OPENSSL_NO_EGD
    external-tests      [default]        OPENSSL_NO_EXTERNAL_TESTS
    fips                [default]
    fips-securitychecks [cascade]        OPENSSL_NO_FIPS_SECURITYCHECKS
    fuzz-afl            [default]        OPENSSL_NO_FUZZ_AFL
    fuzz-libfuzzer      [default]        OPENSSL_NO_FUZZ_LIBFUZZER
    ktls                [default]        OPENSSL_NO_KTLS
    md2                 [default]        OPENSSL_NO_MD2 (skip crypto/md2)
    msan                [default]        OPENSSL_NO_MSAN
    rc5                 [default]        OPENSSL_NO_RC5 (skip crypto/rc5)
    sctp                [default]        OPENSSL_NO_SCTP
    trace               [default]        OPENSSL_NO_TRACE
    ubsan               [default]        OPENSSL_NO_UBSAN
    unit-test           [default]        OPENSSL_NO_UNIT_TEST
    uplink              [no uplink_arch] OPENSSL_NO_UPLINK
    weak-ssl-ciphers    [default]        OPENSSL_NO_WEAK_SSL_CIPHERS
    zlib                [default]
    zlib-dynamic        [default]
    ssl3                [default]        OPENSSL_NO_SSL3
    ssl3-method         [default]        OPENSSL_NO_SSL3_METHOD

Config target attributes:

    AR => "ar",
    ARFLAGS => "qc",
    CC => "gcc",
    CFLAGS => "-Wall -O3",
    CXX => "g++",
    CXXFLAGS => "-Wall -O3",
    HASHBANGPERL => "/usr/bin/env perl",
    RANLIB => "ranlib",
    RC => "windres",
    asm_arch => "x86_64",
    bn_ops => "SIXTY_FOUR_BIT_LONG",
    build_file => "Makefile",
    build_scheme => [ "unified", "unix" ],
    cflags => "-pthread -m64",
    cppflags => "",
    cxxflags => "-std=c++11 -pthread -m64",
    defines => [ "OPENSSL_BUILDING_OPENSSL" ],
    disable => [  ],
    dso_ldflags => "-Wl,-z,defs",
    dso_scheme => "dlfcn",
    enable => [ "afalgeng" ],
    ex_libs => "-ldl -pthread",
    includes => [  ],
    lflags => "",
    lib_cflags => "",
    lib_cppflags => "-DOPENSSL_USE_NODELETE -DL_ENDIAN",
    lib_defines => [  ],
    module_cflags => "-fPIC",
    module_cxxflags => undef,
    module_ldflags => "-Wl,-znodelete -shared -Wl,-Bsymbolic",
    multilib => "64",
    perl_platform => "Unix",
    perlasm_scheme => "elf",
    shared_cflag => "-fPIC",
    shared_defflag => "-Wl,--version-script=",
    shared_defines => [  ],
    shared_ldflag => "-Wl,-znodelete -shared -Wl,-Bsymbolic",
    shared_rcflag => "",
    shared_sonameflag => "-Wl,-soname=",
    shared_target => "linux-shared",
    thread_defines => [  ],
    thread_scheme => "pthreads",
    unistd => "<unistd.h>",

Recorded environment:

    AR =
    ARFLAGS =
    AS =
    ASFLAGS =
    BUILDFILE =
    CC =
    CFLAGS =
    CPP =
    CPPDEFINES =
    CPPFLAGS =
    CPPINCLUDES =
    CROSS_COMPILE =
    CXX =
    CXXFLAGS =
    HASHBANGPERL =
    LD =
    LDFLAGS =
    LDLIBS =
    MT =
    MTFLAGS =
    OPENSSL_LOCAL_CONFIG_DIR =
    PERL =
    RANLIB =
    RC =
    RCFLAGS =
    RM =
    WINDRES =
    __CNF_CFLAGS =
    __CNF_CPPDEFINES =
    __CNF_CPPFLAGS =
    __CNF_CPPINCLUDES =
    __CNF_CXXFLAGS =
    __CNF_LDFLAGS =
    __CNF_LDLIBS =

Makevars:

    AR              = ar
    ARFLAGS         = qc
    CC              = gcc
    CFLAGS          = -Wall -O3
    CPPDEFINES      =
    CPPFLAGS        =
    CPPINCLUDES     =
    CXX             = g++
    CXXFLAGS        = -Wall -O3
    HASHBANGPERL    = /usr/bin/env perl
    LDFLAGS         =
    LDLIBS          =
    PERL            = /usr/bin/perl
    RANLIB          = ranlib
    RC              = windres
    RCFLAGS         =

NOTE: These variables only represent the configuration view.  The build file
template may have processed these variables further, please have a look at the
build file for more exact data:
    Makefile

build file:

    Makefile

build file templates:

    Configurations/common0.tmpl
    Configurations/unix-Makefile.tmpl

$ gcc --version
gcc (Gentoo Hardened 13.1.1_p20230527 p3) 13.1.1 20230527
Copyright (C) 2023 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

With OpenSSL 3.0.9, I get the following while building with -O2 or -O3:

gcc  -I. -Iinclude -Iproviders/common/include -Iproviders/implementations/include  -DAES_ASM -DBSAES_ASM -DCMLL_ASM -DECP_NISTZ256_ASM -DGHASH_ASM -DKECCAK1600_ASM -DMD5_ASM -DOPENSSL_BN_ASM_GF2m -DOPENSSL_BN_ASM_MONT -DOPENSSL_BN_ASM_MONT5 -DOPENSSL_CPUID_OBJ -DOPENSSL_IA32_SSE2 -DPOLY1305_ASM -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DVPAES_ASM -DWHIRLPOOL_ASM -DX25519_ASM -fPIC -pthread -m64 -Wa,--noexecstack -Wall -O3 -DOPENSSL_USE_NODELETE -DL_ENDIAN -DOPENSSL_PIC -DOPENSSLDIR="\"/usr/local/ssl\"" -DENGINESDIR="\"/usr/local/lib64/engines-3\"" -DMODULESDIR="\"/usr/local/lib64/ossl-modules\"" -DOPENSSL_BUILDING_OPENSSL -DNDEBUG  -MMD -MF crypto/bn/libcrypto-lib-rsaz_exp_x2.d.tmp -MT crypto/bn/libcrypto-lib-rsaz_exp_x2.o -c -o crypto/bn/libcrypto-lib-rsaz_exp_x2.o crypto/bn/rsaz_exp_x2.c
crypto/bn/rsaz_exp_x2.c: In function 'to_words52.constprop':
crypto/bn/rsaz_exp_x2.c:500:16: warning: iteration 19 invokes undefined behavior [-Waggressive-loop-optimizations]
  500 |         out_len--;
      |         ~~~~~~~^~
crypto/bn/rsaz_exp_x2.c:498:20: note: within this loop
  498 |     while (out_len > 0) {
      |            ~~~~~~~~^~~

But OpenSSL 1.1.1u and OpenSSL 3.1.1 are fine.

I spent a while staring at and comparing the branches and was avoiding looking at 3d2b47b given it's pretty large, but I think that's the commit which fixes it on 3.1.x. If I try to (very cheesily) cherry-pick it to 3.0.x just to see if it helps, that works.

That commit is unfortunately pretty large and includes some refactoring as prep work for the optimisation it adds, so it's hard as somone unfamiliar with the codebase to figure out quickly which bits are the actual substantive change suppressing the warning here, or if it's just by chance.

[paulidale]

Any idea what's undefined here?
out_len is an int, so can go negative safely. It's checked earlier and known to be positive to begin with.

19 iterations is kind of weird too.

[paulidale]

For 1024 bit numbers, it's reaching the final element (20th). I suspect the optimiser is thinking that it could go one further.

[t8m]

Yeah, this looks like a bug in the compiler. out_len is int so it can go negative without UB.

However this cannot really go negative. So this seems to be bogus warning.

[paulidale]

I think it's stepping off the end of the array, even though it isn't really.

[nhorman]

concur that its probably a compiler bug, though it looks super odd in disassembly:

 if (in_bitsize > DIGIT_SIZE) {
        uint64_t digit = get_digit52(in_str, 7);

        out[0] = digit & DIGIT_MASK;
 664:   48 21 f1                and    %rsi,%rcx
 667:   48 89 8a 90 00 00 00    mov    %rcx,0x90(%rdx)
        digit += (uint64_t)(in[in_len - 1]);
 66e:   0f b6 70 7f             movzbl 0x7f(%rax),%esi
 672:   0f b6 48 7e             movzbl 0x7e(%rax),%ecx
        digit <<= 8;
 676:   48 c1 e6 08             shl    $0x8,%rsi
        digit += (uint64_t)(in[in_len - 1]);
 67a:   48 01 f1                add    %rsi,%rcx
 67d:   0f b6 70 7d             movzbl 0x7d(%rax),%esi
        digit <<= 8;
 681:   48 c1 e1 08             shl    $0x8,%rcx
        digit += (uint64_t)(in[in_len - 1]);
 685:   48 01 ce                add    %rcx,%rsi
 688:   0f b6 48 7c             movzbl 0x7c(%rax),%ecx
 68c:   0f b6 40 7b             movzbl 0x7b(%rax),%eax
        digit <<= 8;
 690:   48 c1 e6 08             shl    $0x8,%rsi
        digit += (uint64_t)(in[in_len - 1]);
 694:   48 01 f1                add    %rsi,%rcx
        digit <<= 8;
697:   48 c1 e1 08             shl    $0x8,%rcx
        digit += (uint64_t)(in[in_len - 1]);
 69b:   48 01 c8                add    %rcx,%rax
        in_str += 6;
        in_bitsize -= DIGIT_SIZE;
        digit = get_digit52(in_str, BITS2WORD8_SIZE(in_bitsize));
        out[1] = digit >> 4;
 69e:   48 c1 e8 04             shr    $0x4,%rax
 6a2:   48 89 82 98 00 00 00    mov    %rax,0x98(%rdx)
    while (out_len > 0) {
        *out = 0;
        out_len--;
        out++;
    }
}
 6a9:   c3                      ret
 6aa:   66 0f 1f 44 00 00       nopw   0x0(%rax,%rax,1)

Reading -O3 bits is always really hard for me, but that to me looks like the compiler decided to optimize away the entire while (out_len > 0) loop. out I believe gets stored in %rsi (which makes sense), but I don't ever see anything in that assembly that winds up storing a zero to %rsi in the unrolled loop