• Enabling some -Werror=format* by default?

    From Andrey Rakhmatullin@21:1/5 to All on Thu Jun 6 21:20:01 2024
    We recently increased the time_t size on certain architectures and some packages started failing to build because they were using a format
    specifier too narrow for the wider time_t, e.g. #1067829.
    But the only reason those package FTBFS is they enable either -Werror or
    some more specific non-default switch like -Werror=format=2, so I suspect
    much more packages contain similar code but gained only a warning. Isn't
    this a bad thing? Should we enable at least some combination of -Wformat* switches by default? Should we at least add a new flag to dpkg-buildflags
    and do some test rebuilds with it enabled?

    --
    WBR, wRAR

    -----BEGIN PGP SIGNATURE-----

    iQJhBAABCgBLFiEEolIP6gqGcKZh3YxVM2L3AxpJkuEFAmZiC7wtFIAAAAAAFQAP cGthLWFkZHJlc3NAZ251cGcub3Jnd3JhckBkZWJpYW4ub3JnAAoJEDNi9wMaSZLh KWAQAI7THG0zW/9PQ0oculK0m1DlIXU01g5R+30EDt96UgQjG4oO0WzY7IA+DJMs l0Ee+kiD39Tm12vfYhXAD8kIXDnJ/d99S2bKD5gOFs/z0gEd06Ir/UE+9j0eLZ9N gl4oylDuoNnCMe7s3+leGSti8HKeBe2wf6ZIweO59za+V0jJZhU3vg16WgRS/2Xe ZmDpOQ4he/781179l38kSRMxlhNV71x2tUTgxoq+kjK7/muLsAoPID7FPNKcQ0OC zh0rBPuH+apVcytuWAP4AF8/fuBn4kAViLeI1QwTl91aAJ0aZhvZIiryaN0o8GNM vIn06xfQeKQVhKKjKlDEOXdcNGfKxa5ZOkPOzcPwvDOnPtcfYTQa6cTRsBpIvIiB urt5aRx1yLX9R5am6e9rV+VJK9+YdNbusMgIO9vfjDG1Ieo6jFgu990a35k3hsSY 6+bb+AmNqDBXGUKNDfiGOwwcWW0lp/5nTJvuujfCzIrrsw+QtZZxPNpLHDnZoJBp Mj09ZsR1IxNhPI9mOW302yanThepjJBdh3cdzQSL0W+PPVTB8+0EzIkuuo0hQIL/ Fbo2ChtahT9KC/vvTGjzBpbaOlI5sUTHNo53WqZIg7oQHfy9H71Zy7QNny6aWlqJ PA9a08F12srS1JG/4BrLQUT2HHsKj+QbI0HxBsQ/H3wA/EFF
    =/Mqm
    -----END PGP SIGNATURE-----

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Andrey Rakhmatullin@21:1/5 to Helmut Grohne on Mon Jun 10 13:10:02 2024
    On Mon, Jun 10, 2024 at 07:48:59AM +0200, Helmut Grohne wrote:
    We recently increased the time_t size on certain architectures and some packages started failing to build because they were using a format specifier too narrow for the wider time_t, e.g. #1067829.
    But the only reason those package FTBFS is they enable either -Werror or some more specific non-default switch like -Werror=format=2, so I suspect much more packages contain similar code but gained only a warning. Isn't this a bad thing? Should we enable at least some combination of -Wformat* switches by default? Should we at least add a new flag to dpkg-buildflags and do some test rebuilds with it enabled?

    It wasn't quite clear to me what -Werror=format=2 actually means.
    According to the gcc documentation[1], -Wformat=2 currently means:

    -Wformat -Wformat-nonliteral -Wformat-security -Wformat-y2k.

    Of these, we already enable -Werror=format-security, but not the other
    ones. It is not clear to me, which of these actually catches the the
    type mismatches. Would you do more research here?

    #include <stdio.h>

    void foo()
    {
    printf("%d", 0L);
    }

    This produces a warning with just -Wformat ("format ‘%d’ expects argument of type ‘int’, but argument 2 has type ‘long int’ [-Wformat=]"). It doesn't look like any more narrow options for this exist (-Wno-* ones
    listed in the -Wformat description don't silence this one, and they don't
    look like they should anyway).

    But I don't know if even -Werror=format is too much to enable by default globally (I assume it's a good thing to enable it, though).

    It also is unclear how this impacts the archive and yes, I'd recommend a rebuild. Note though that we likely need this rebuild both on a 64bit architecture and a 32bit architecture that is not i386 (due to how t64 works). A partial archive rebuild may work to gauge the size of the
    problem.

    I note that this kind of change affects cross builds, so performing
    cross builds for armhf on amd64 will likely show many of these failures
    (in addition to all the regular cross build failures).

    I recommend doing more research before moving forward with this. In particular a MBF about introduced problems would be prudent before doing
    the switch and even if we don't switch, such a MBF bears value on its
    own.
    Do you think it makes sense to add this a flag that enables -Werror=format
    to dpkg-buildflags(1), before, or after a test rebuild, before, or after
    the MBF if we do one?

    --
    WBR, wRAR

    -----BEGIN PGP SIGNATURE-----

    iQJhBAABCgBLFiEEolIP6gqGcKZh3YxVM2L3AxpJkuEFAmZm3iUtFIAAAAAAFQAP cGthLWFkZHJlc3NAZ251cGcub3Jnd3JhckBkZWJpYW4ub3JnAAoJEDNi9wMaSZLh tuIP/1B0zI4mV/5tpg1Lqo/p04spOYqLWdF8Hl3nvcE0yJP7u4v7xp3et5Dzcf13 r+8egWFa9XujJNkpycvFu3MDDNLGAkbYonn6loqRrZlhcKN3hk7L66epi5fE0Wvh N7f2OX4wsE4sdC9jF6oXRhhNLpsQZyk6+X36pfysl0I+yioAUZ5HB97D3WoiN5q9 FKb/X9O/8PnRpOY+tQ1ppSmvfe5HWly1L7pGMhalcxxs5FoDVjP9HCd5ONoiDJ7E 2WuNdDXWav+ZxbgQBomaBL/2psXk/WwW9nDx6AHldPIqctP9uxJYp0VY0GHofRfS VS1rPO06qBKQXbQFakd4iIetG2tjXbQwxi+c6dHWi5RjCatt2oYNCtg04kpriFgc 6ICo4u91eV3+fUxL6FT3uFQQJcnOMV219fVF1UqkmYHS2/7ePYUWPXbkDq+u2h9t BOv3KucL0bgV7eGkrZaDzEhB2xtJiMi/cIZCNRyH5wMwEeHLq0w3DJZchEyIj35r KSx2UaeTwdYXpXnZ3apJgdjjVsmWmtf2n8zgMoYI3zpBsPNsDpClOFL1dw0HOsxu /sDKdHH9H0UCANDp7pnN5FG03RjvN3n9Xkk+RzYIeTHsYnNJ8phtBV6vv4aT8x72 QaQ7Dou4uiZdysp4zHbwCZvdrMFye3smdWqlweCsaFI92uVI
    =0dw2
    -----END PGP SIGNATURE-----

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Helmut Grohne@21:1/5 to Andrey Rakhmatullin on Mon Jun 10 12:40:01 2024
    On Fri, Jun 07, 2024 at 12:19:28AM +0500, Andrey Rakhmatullin wrote:
    We recently increased the time_t size on certain architectures and some packages started failing to build because they were using a format
    specifier too narrow for the wider time_t, e.g. #1067829.
    But the only reason those package FTBFS is they enable either -Werror or
    some more specific non-default switch like -Werror=format=2, so I suspect much more packages contain similar code but gained only a warning. Isn't
    this a bad thing? Should we enable at least some combination of -Wformat* switches by default? Should we at least add a new flag to dpkg-buildflags
    and do some test rebuilds with it enabled?

    It wasn't quite clear to me what -Werror=format=2 actually means.
    According to the gcc documentation[1], -Wformat=2 currently means:

    -Wformat -Wformat-nonliteral -Wformat-security -Wformat-y2k.

    Of these, we already enable -Werror=format-security, but not the other
    ones. It is not clear to me, which of these actually catches the the
    type mismatches. Would you do more research here?

    It also is unclear how this impacts the archive and yes, I'd recommend a rebuild. Note though that we likely need this rebuild both on a 64bit architecture and a 32bit architecture that is not i386 (due to how t64
    works). A partial archive rebuild may work to gauge the size of the
    problem.

    I note that this kind of change affects cross builds, so performing
    cross builds for armhf on amd64 will likely show many of these failures
    (in addition to all the regular cross build failures).

    I recommend doing more research before moving forward with this. In
    particular a MBF about introduced problems would be prudent before doing
    the switch and even if we don't switch, such a MBF bears value on its
    own.

    Helmut

    [1] https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Andrey Rakhmatullin@21:1/5 to Guillem Jover on Mon Jun 10 17:10:01 2024
    On Mon, Jun 10, 2024 at 04:24:54PM +0200, Guillem Jover wrote:
    Do you think it makes sense to add this a flag that enables -Werror=format to dpkg-buildflags(1), before, or after a test rebuild, before, or after the MBF if we do one?

    If by adding you mean adding a new feature flag that is disabled by
    default, then depending on the feature area, that might unfortunately
    be equivalent to enable it by default (due to the «all» keyword).

    Another related question: if not via dpkg-buildflags, how do we do
    rebuilds with changed default flags?

    --
    WBR, wRAR

    -----BEGIN PGP SIGNATURE-----

    iQJhBAABCgBLFiEEolIP6gqGcKZh3YxVM2L3AxpJkuEFAmZnFyEtFIAAAAAAFQAP cGthLWFkZHJlc3NAZ251cGcub3Jnd3JhckBkZWJpYW4ub3JnAAoJEDNi9wMaSZLh ZlEQAJd5tro8LvJQ8nEd5QZ1g1bS4OKFbL2QdUHrrECVCwQzRSdCwxlyblccnnGV uy74IAfUMkmklfCAk3HXsHHDyRCpd7H5t4OcLlyMMlzPzBd0HIreA/IjHHYgeB2G pqyzAWqlEkPjympYyJ0nULCg8/rFHeteXFqn/548FF6eQKwItZyl0ZrFQolLV43k gi+od7ySCXzUvBXT34oxzHDZ0MRoWluvDi+MnKoxzhVNueDhpux5YWdcfCfTm/Cg ZLzrME7sZfbevWcAofa+H/qdkgc1EU7of+gtU9knjEy6WGbPl4j0LEYktg9+7yAF JwuWwxBV5Od6taeZLK7eqksMYKfL6V8zWBo/1EEUC5KSC6do552bIaMXYdnTKUPf eMvC7o76gnV8wzYlP9XXN4vyoXmU+ih51VrCM+a4Ov34QdWVFJ3OHBAjKA2u6fQ9 dngVPM23SXo4B0FjtpjS7dj745cDx7zXKEBGmLTK3PS6etzv/LajCR4WjU9ZzUNa wrt6SeToxZoGBXD8sw1gJv6nel2l7rBIKnldTfyd/LOX3Y2Bc9itzwHrLJh4Qy9y kB8vk85fZ7Kg/yB/nbhCdbITns1NzFkCJIXSN+KDHTKNmiga1j3F5Ufakjxmw3x2 ft+UeIPG+L42DGWqj3GAHcZN2a7s7BSs1vi4MAb7uAPK4mnY
    =DEGC
    -----END PGP SIGNATURE-----

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Guillem Jover@21:1/5 to Andrey Rakhmatullin on Mon Jun 10 16:30:01 2024
    Hi!

    On Mon, 2024-06-10 at 16:06:13 +0500, Andrey Rakhmatullin wrote:
    On Mon, Jun 10, 2024 at 07:48:59AM +0200, Helmut Grohne wrote:
    We recently increased the time_t size on certain architectures and some packages started failing to build because they were using a format specifier too narrow for the wider time_t, e.g. #1067829.
    But the only reason those package FTBFS is they enable either -Werror or some more specific non-default switch like -Werror=format=2, so I suspect much more packages contain similar code but gained only a warning. Isn't this a bad thing? Should we enable at least some combination of -Wformat* switches by default? Should we at least add a new flag to dpkg-buildflags and do some test rebuilds with it enabled?

    It wasn't quite clear to me what -Werror=format=2 actually means.
    According to the gcc documentation[1], -Wformat=2 currently means:

    -Wformat -Wformat-nonliteral -Wformat-security -Wformat-y2k.

    Those are in addition to the ones enabled by -Wformat=1, which are:

    -Wnonnull -Wno-format-contains-nul -Wno-format-extra-args
    -Wno-format-zero-length

    So all the above would supposedly turn into -Werror too.

    But I don't know if even -Werror=format is too much to enable by default globally (I assume it's a good thing to enable it, though).

    Hard to say w/o a rebuild I guess. (And take into account this can
    also affect configure-style checks, which might become silent errors
    but end up disabling features or enabling unexpected code paths or
    similar.)

    In general I think it's good (in principle) to enable -Werror flags
    that detect actual bugs in code, the problem is always going to be
    the amount of fallout and work that creates, and whether there's
    consensus about that work commitment being acceptable to maintainers
    in Debian, or whether that can interfere with transitions going on,
    etc.

    It also is unclear how this impacts the archive and yes, I'd recommend a rebuild. Note though that we likely need this rebuild both on a 64bit architecture and a 32bit architecture that is not i386 (due to how t64 works). A partial archive rebuild may work to gauge the size of the problem.

    I note that this kind of change affects cross builds, so performing
    cross builds for armhf on amd64 will likely show many of these failures
    (in addition to all the regular cross build failures).

    I recommend doing more research before moving forward with this. In particular a MBF about introduced problems would be prudent before doing the switch and even if we don't switch, such a MBF bears value on its
    own.

    Do you think it makes sense to add this a flag that enables -Werror=format
    to dpkg-buildflags(1), before, or after a test rebuild, before, or after
    the MBF if we do one?

    If by adding you mean adding a new feature flag that is disabled by
    default, then depending on the feature area, that might unfortunately
    be equivalent to enable it by default (due to the «all» keyword).

    (I started a design to version the build flags interface, but I got
    stuck, so I let it brew as a background process, have pending to finish
    that up and propose it on d-dpkg.)

    Thanks,
    Guillem

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Simon McVittie@21:1/5 to Guillem Jover on Mon Jun 10 17:10:02 2024
    On Mon, 10 Jun 2024 at 16:24:54 +0200, Guillem Jover wrote:
    In general I think it's good (in principle) to enable -Werror flags
    that detect actual bugs in code, the problem is always going to be
    the amount of fallout and work that creates

    Yes. The difficult thing here is balancing "detect actual (important)
    bugs in code" against "detect non-bugs or unimportant bugs, and make
    them be treated as if they were important".

    Unfortunately, because compiler warnings are mostly heuristics involving
    a reasonable guess about what the developer had intended to happen, most compiler warnings do a mixture of both.

    smcv

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Guillem Jover@21:1/5 to Andrey Rakhmatullin on Mon Jun 10 18:30:01 2024
    Hi!

    On Mon, 2024-06-10 at 20:09:21 +0500, Andrey Rakhmatullin wrote:
    On Mon, Jun 10, 2024 at 04:24:54PM +0200, Guillem Jover wrote:
    Do you think it makes sense to add this a flag that enables -Werror=format
    to dpkg-buildflags(1), before, or after a test rebuild, before, or after the MBF if we do one?

    If by adding you mean adding a new feature flag that is disabled by default, then depending on the feature area, that might unfortunately
    be equivalent to enable it by default (due to the «all» keyword).

    Another related question: if not via dpkg-buildflags, how do we do
    rebuilds with changed default flags?

    Ah, you can still use its mechanism (just not its policy) where a
    rebuild should be able to use DEB_<flag>_APPEND for that, or set
    them in one of its config files.

    If having a modified dpkg-dev emitting those by default would be less cumbersome for the rebuilder, I'm happy to provide an out of archive
    package for that for example, which I think we have done for some
    rebuilds.

    Thanks,
    Guillem

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Helmut Grohne@21:1/5 to Andrey Rakhmatullin on Mon Jun 10 18:30:01 2024
    On Mon, Jun 10, 2024 at 04:06:13PM +0500, Andrey Rakhmatullin wrote:
    Do you think it makes sense to add this a flag that enables -Werror=format
    to dpkg-buildflags(1), before, or after a test rebuild, before, or after
    the MBF if we do one?

    I think that a test rebuild and the MBF are reasonable preconditions to
    extend the default build flags (and with default I mean changing hardening=+all).

    As multiple people pointed out, the effects of the flags are hard to
    predict and they can even cause misbuilds (via failing configure
    checks), so these flags do have a non-trivial cost (and benefits).

    Ideally, we'd not just do a rebuild with the flags, but also do a
    rebuild without and then compare the binary .debs. In the event that we misguide configure, we expect the .debs to differ and otherwise to equal
    due to the work of the reproducible builds folks. That equality has a
    really annoying difference in practice though: Build ids are dependent
    on the compiler flags, so the comparison would have to magically ignore
    changes in build id and this is where things become quite difficult.

    Another related question: if not via dpkg-buildflags, how do we do
    rebuilds with changed default flags?

    If you export DEB_CFLAGS_APPEND=-Werror=format=2 and DEB_CXXFLAGS_APPEND=-Werror=format=2 (not to be confused with DEB_*_MAINT_APPEND which is often set in d/rules), you should get most
    packages to pick up these flags.

    Possibly debusine.debian.net can be used to perform such a rebuild
    rather than using your own resources. Steering it to do this is a
    non-trivial task at present, but I my impression is that you will
    receive support in doing so and it can do native armhf builds
    (eliminating the need for cross builds). Your mileage will vary.

    In any case, my impression is that the first step towards changing
    hardening flags is actually performing test builds in whatever form.

    Helmut

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Fay Stegerman@21:1/5 to All on Tue Jun 11 03:00:01 2024
    * Helmut Grohne <helmut@subdivi.de> [2024-06-10 18:01]:
    Ideally, we'd not just do a rebuild with the flags, but also do a
    rebuild without and then compare the binary .debs. In the event that we misguide configure, we expect the .debs to differ and otherwise to equal
    due to the work of the reproducible builds folks. That equality has a
    really annoying difference in practice though: Build ids are dependent
    on the compiler flags, so the comparison would have to magically ignore changes in build id and this is where things become quite difficult.

    Indeed build IDs can be a very annoying source of unreproducible builds (though I mostly encounter those working with Android toolchains, not Debian packages).

    But build IDs can be disabled via appropriate linker flags (e.g. -Wl,--build-id=none). Is there some reason simply doing that for both rebuilds is not an option?

    - Fay

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Guillem Jover@21:1/5 to Helmut Grohne on Tue Jun 11 12:10:02 2024
    Hi!

    On Mon, 2024-06-10 at 18:01:58 +0200, Helmut Grohne wrote:
    Ideally, we'd not just do a rebuild with the flags, but also do a
    rebuild without and then compare the binary .debs. In the event that we misguide configure, we expect the .debs to differ and otherwise to equal
    due to the work of the reproducible builds folks. That equality has a
    really annoying difference in practice though: Build ids are dependent
    on the compiler flags, so the comparison would have to magically ignore changes in build id and this is where things become quite difficult.

    A quick check seems to indicate warning flags do not get recorded in
    the object files, which make sense as these are not supposed to change
    the emitted objects.

    (This can be seen with «dwarfdump .../*.debug | grep DW_AT_producer»
    on the debug files from the -dbgsym packages.)

    Thanks,
    Guillem

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)