Re: [PATCH 00/14] Use const whether we point to literal strings (take 1)

From: Jan Beulich <jbeulich@suse.com>
To: Julien Grall <julien@xen.org>
Cc: "Julien Grall" <jgrall@amazon.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Ian Jackson" <iwj@xenproject.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Wei Liu" <wl@xen.org>, "Dario Faggioli" <dfaggioli@suse.com>,
	"Tim Deegan" <tim@xen.org>,
	"Roger Pau Monné" <roger.pau@citrix.com>,
	"Anthony PERARD" <anthony.perard@citrix.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH 00/14] Use const whether we point to literal strings (take 1)
Date: Tue, 6 Apr 2021 09:50:44 +0200	[thread overview]
Message-ID: <bdcf123c-0345-a496-03fa-4279726469ea@suse.com> (raw)
In-Reply-To: <20210405155713.29754-1-julien@xen.org>

On 05.04.2021 17:56, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Hi all,
> 
> By default, both Clang and GCC will happily compile C code where
> non-const char * point to literal strings. This means the following
> code will be accepted:
> 
>     char *str = "test";
> 
>     str[0] = 'a';
> 
> Literal strings will reside in rodata, so they are not modifiable.
> This will result to an permission fault at runtime if the permissions
> are enforced in the page-tables (this is the case in Xen).
> 
> I am not aware of code trying to modify literal strings in Xen.
> However, there is a frequent use of non-const char * to point to
> literal strings. Given the size of the codebase, there is a risk
> to involuntarily introduce code that will modify literal strings.
> 
> Therefore it would be better to enforce using const when pointing
> to such strings. Both GCC and Clang provide an option to warn
> for such case (see -Wwrite-strings) and therefore could be used
> by Xen.
> 
> This series doesn't yet make use of -Wwrite-strings because
> the tree is not fully converted. Instead, it contains some easy
> and likely non-controversial use const in the code.
> 
> The major blockers to enable -Wwrite-strings are the following:
>     - xen/common/efi: union string is used in both const and
>     non-const situation. It doesn't feel right to specific one member
>     const and the other non-const.

I'd be happy to see a suggestion of how to avoid this in a not overly
intrusive way.

>     - libxl: the major block is the flexarray framework as we would use
>     it with string (now const char*). I thought it would be possible to
>     make the interface const, but it looks like there are a couple of
>     places where we need to modify the content (such as in
>     libxl_json.c).
> 
> Ideally, I would like to have -Wwrite-strings unconditionally used
> tree-wide. But, some of the area may required some heavy refactoring.
> 
> One solution would be to enable it tree-wide but turned it off at a
> directroy/file level.

At least as a transient approach I think this would make sense. EFI in
particular has other reasons already to specify a custom option
(-fshort-wchar).

Jan