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
Attachment:
signature.asc
Description: PGP signature
