Subject: Re: [PATCH 00/19] Compile with `-Wwrite-strings`

From: Patrick Steinhardt <[email-masked]>
Date: Wed, 29 May 2024 14:52:15 +0200

On Wed, May 29, 2024 at 02:44:01PM +0200, Patrick Steinhardt wrote:
> Hi,
> 
> there were some recent discussions about compiler warnings and how to
> stay on top of breaking changes in compilers in general [1] and about
> string constants in particular [2]. This made me look into what kind of
> warnings we should reasonably enable, which led me to the following
> list of warnings that may be sensible:
> 
>   - `-Wformat-nonliteral` to warn about non-constant strings being
>     passed as format string.
> 
>   - `-Wwrite-strings` to warn about string constants being assigned to a
>     non-constant variable.
> 
>   - `-Wredundant-decls` to warn about redundant declarations.
> 
>   - `-Wconversion` to warn about implicit integer casts when they may
>     alter the value.
> 
> This patch series adapts our code to compile with `-Wwrite-strings`.
> This option will change the type of string constants from `char []` to
> `const char []` such that it is now invalid to assign it to non-const
> variables without a cast. The intent is to avoid undefined behaviour
> when accedintally writing to such strings and to avoid free'ing such a
> variable.
> 
> There are quite some cases where we mishandle this. Oftentimes we just
> didn't bother to free any memory at all, which made it a non-issue in
> the first place. Other times we had some special logic that prevents
> writing or freeing such strings. But in most cases it was an accident
> waiting to happen.
> 
> Even though the changes are quite invasive, I think that this is a step
> into the right direction. Many of the constructs feel quite fragile, and
> most of those get fixed in this series. Some others I just paper over,
> for example when assigning to structures with global lifetime where we
> know that they are never released at all.
> 
> I also have a patch series cooking for `-Wredundant-decls`. But while
> that warning detects some redundant declarations indeed, it creates a
> problem with `extern char **environ`. There is no header for it and
> programs are asked to declare it by themselves. But of course, some libc
> implementations disagree and declare it. I haven't found a nice way to
> work around this issue, but may send the patches that drop the redundant
> declarations nonetheless.
> 
> The other two warnings I haven't yet looked into.
> 
> I ran some test jobs on both GitHub [3] and GitLab [4] to verify that
> the result is sane.
> 
> Thanks!

I forgot to say that this is based on top of 3a57aa566a (The eighth
batch, 2024-05-28) with ps/leakfixes merged into it at ebdbefa4fe
(builtin/mv: fix leaks for submodule gitfile paths, 2024-05-27).

Patrick