Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.debian.devel > #112078 > unrolled thread

Enabling some -Werror=format* by default?

Back to article view | Back to linux.debian.devel

Contents

  Enabling some -Werror=format* by default? Andrey Rakhmatullin <wrar@debian.org> - 2024-06-06 21:20 +0200
    Re: Enabling some -Werror=format* by default? Helmut Grohne <helmut@subdivi.de> - 2024-06-10 12:40 +0200
      Re: Enabling some -Werror=format* by default? Andrey Rakhmatullin <wrar@debian.org> - 2024-06-10 13:10 +0200
        Re: Enabling some -Werror=format* by default? Guillem Jover <guillem@debian.org> - 2024-06-10 16:30 +0200
          Re: Enabling some -Werror=format* by default? Andrey Rakhmatullin <wrar@debian.org> - 2024-06-10 17:10 +0200
            Re: Enabling some -Werror=format* by default? Guillem Jover <guillem@debian.org> - 2024-06-10 18:30 +0200
          Re: Enabling some -Werror=format* by default? Andrey Rakhmatullin <wrar@debian.org> - 2024-06-10 17:10 +0200
          Re: Enabling some -Werror=format* by default? Simon McVittie <smcv@debian.org> - 2024-06-10 17:10 +0200
        Re: Enabling some -Werror=format* by default? Helmut Grohne <helmut@subdivi.de> - 2024-06-10 18:30 +0200
          Re: Enabling some -Werror=format* by default? Fay Stegerman <flx@obfusk.net> - 2024-06-11 03:00 +0200
          Re: Enabling some -Werror=format* by default? Guillem Jover <guillem@debian.org> - 2024-06-11 12:10 +0200

#112078 — Enabling some -Werror=format* by default?

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [next] | [standalone]

#112111

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

[toc] | [prev] | [next] | [standalone]

#112112

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#112116

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

[toc] | [prev] | [next] | [standalone]

#112118

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#112125

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

[toc] | [prev] | [next] | [standalone]

#112119

[Multipart message — attachments visible in raw view] — view raw

On Mon, Jun 10, 2024 at 04:24:54PM +0200, Guillem Jover 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:

(note that -Wformat is already included in that list)

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

(note that -Wformat is not equal to these flags, it just implies -Wnonnull
and those three -Wno-* can be used to turn off some parts of it)


> 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.

Yeah.

> > > 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).

That's a good point, though hopefully people who use "all" understand the
implication...

-- 
WBR, wRAR

[toc] | [prev] | [next] | [standalone]

#112120

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

[toc] | [prev] | [next] | [standalone]

#112124

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

[toc] | [prev] | [next] | [standalone]

#112127

* 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

[toc] | [prev] | [next] | [standalone]

#112132

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.debian.devel

csiph-web