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

[Julien Grall <julien@xxxxxxx>]

Hi,

Ian, Wei, Anthony, can I get some feedbacks on the tools side?

Cheers,

On 05/04/2021 16:56, Julien Grall wrote:
> From: Julien Grall <jgrall@xxxxxxxxxx>
>
> 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.
>      - 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.
>
> Any opinions?
>
> Cheers,
>
> Julien Grall (14):
>    xen: Constify the second parameter of rangeset_new()
>    xen/sched: Constify name and opt_name in struct scheduler
>    xen/x86: shadow: The return type of sh_audit_flags() should be const
>    xen/char: console: Use const whenever we point to literal strings
>    tools/libs: guest: Use const whenever we point to literal strings
>    tools/libs: stat: Use const whenever we point to literal strings
>    tools/xl: Use const whenever we point to literal strings
>    tools/firmware: hvmloader: Use const in __bug() and __assert_failed()
>    tools/console: Use const whenever we point to literal strings
>    tools/kdd: Use const whenever we point to literal strings
>    tools/misc: Use const whenever we point to literal strings
>    tools/top: The string parameter in set_prompt() and set_delay() should
>      be const
>    tools/xenmon: xenbaked: Mark const the field text in stat_map_t
>    tools/xentrace: Use const whenever we point to literal strings
>
>   tools/console/client/main.c         |  4 +-
>   tools/console/daemon/io.c           | 10 ++--
>   tools/debugger/kdd/kdd.c            | 10 ++--
>   tools/firmware/hvmloader/util.c     |  4 +-
>   tools/firmware/hvmloader/util.h     |  4 +-
>   tools/include/xenguest.h            | 10 ++--
>   tools/libs/guest/xg_dom_core.c      |  8 ++--
>   tools/libs/guest/xg_dom_elfloader.c |  4 +-
>   tools/libs/guest/xg_dom_hvmloader.c |  2 +-
>   tools/libs/guest/xg_dom_x86.c       |  9 ++--
>   tools/libs/guest/xg_private.h       |  2 +-
>   tools/libs/stat/xenstat_linux.c     |  4 +-
>   tools/libs/stat/xenstat_qmp.c       | 12 ++---
>   tools/misc/xen-detect.c             |  2 +-
>   tools/misc/xenhypfs.c               |  6 +--
>   tools/xenmon/xenbaked.c             |  2 +-
>   tools/xentop/xentop.c               | 12 ++---
>   tools/xentrace/xenalyze.c           | 71 +++++++++++++++--------------
>   tools/xentrace/xenctx.c             |  4 +-
>   tools/xl/xl.h                       |  8 ++--
>   tools/xl/xl_console.c               |  2 +-
>   tools/xl/xl_utils.c                 |  4 +-
>   tools/xl/xl_utils.h                 |  4 +-
>   xen/arch/x86/mm/shadow/multi.c      | 12 ++---
>   xen/common/rangeset.c               |  2 +-
>   xen/common/sched/private.h          |  4 +-
>   xen/drivers/char/console.c          |  4 +-
>   xen/include/xen/rangeset.h          |  2 +-
>   28 files changed, 113 insertions(+), 109 deletions(-)
--
Julien Grall