[PATCH v2 0/2] Use const whenever we point to literal strings (take 1)

[PATCH v2 0/2] Use const whenever we point to literal strings (take 1)

[Julien Grall]

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 non-controversial use of const in the code.

Julien Grall (2):
  xen/char: console: Use const whenever we point to literal strings
  tools/console: Use const whenever we point to literal strings

 tools/console/client/main.c |  4 ++--
 tools/console/daemon/io.c   | 15 ++++++++-------
 xen/drivers/char/console.c  |  7 ++++---
 3 files changed, 14 insertions(+), 12 deletions(-)

-- 
2.17.1

[PATCH v2 1/2] xen/char: console: Use const whenever we point to literal strings

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Literal strings are not meant to be modified. So we should use const
char * rather than char * when we want to store a pointer to them.

The array should also not be modified at all and is only used by
xenlog_update_val(). So take the opportunity to add an extra const and
move the definition in the function.

Signed-off-by: Julien Grall <jgrall@amazon.com>

---
    Changes in v2:
        - The array content should never be modified
        - Move lvl2opt in xenlog_update_val()
---
 xen/drivers/char/console.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 23583751709c..7d0a603d0311 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -168,10 +168,11 @@ static int parse_guest_loglvl(const char *s);
 static char xenlog_val[LOGLVL_VAL_SZ];
 static char xenlog_guest_val[LOGLVL_VAL_SZ];
 
-static char *lvl2opt[] = { "none", "error", "warning", "info", "all" };
-
 static void xenlog_update_val(int lower, int upper, char *val)
 {
+    static const char * const lvl2opt[] =
+        { "none", "error", "warning", "info", "all" };
+
     snprintf(val, LOGLVL_VAL_SZ, "%s/%s", lvl2opt[lower], lvl2opt[upper]);
 }
 
@@ -262,7 +263,7 @@ static int parse_guest_loglvl(const char *s)
     return ret;
 }
 
-static char *loglvl_str(int lvl)
+static const char *loglvl_str(int lvl)
 {
     switch ( lvl )
     {
-- 
2.17.1

Re: [PATCH v2 1/2] xen/char: console: Use const whenever we point to literal strings

[Luca Fancellu]

> On 18 May 2021, at 15:01, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> Literal strings are not meant to be modified. So we should use const
> char * rather than char * when we want to store a pointer to them.
> 
> The array should also not be modified at all and is only used by
> xenlog_update_val(). So take the opportunity to add an extra const and
> move the definition in the function.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ---
>    Changes in v2:
>        - The array content should never be modified
>        - Move lvl2opt in xenlog_update_val()
> ---
> xen/drivers/char/console.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index 23583751709c..7d0a603d0311 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -168,10 +168,11 @@ static int parse_guest_loglvl(const char *s);
> static char xenlog_val[LOGLVL_VAL_SZ];
> static char xenlog_guest_val[LOGLVL_VAL_SZ];
> 
> -static char *lvl2opt[] = { "none", "error", "warning", "info", "all" };
> -
> static void xenlog_update_val(int lower, int upper, char *val)
> {
> +    static const char * const lvl2opt[] =
> +        { "none", "error", "warning", "info", "all" };
> +
>     snprintf(val, LOGLVL_VAL_SZ, "%s/%s", lvl2opt[lower], lvl2opt[upper]);
> }
> 
> @@ -262,7 +263,7 @@ static int parse_guest_loglvl(const char *s)
>     return ret;
> }
> 
> -static char *loglvl_str(int lvl)
> +static const char *loglvl_str(int lvl)
> {
>     switch ( lvl )
>     {
> -- 
> 2.17.1
> 

Hi Julien,

Seems good to me and very sensible.

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

Cheers,
Luca

Re: [PATCH v2 1/2] xen/char: console: Use const whenever we point to literal strings

[Jan Beulich]

On 18.05.2021 16:01, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Literal strings are not meant to be modified. So we should use const
> char * rather than char * when we want to store a pointer to them.
> 
> The array should also not be modified at all and is only used by
> xenlog_update_val(). So take the opportunity to add an extra const and
> move the definition in the function.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

[PATCH v2 2/2] tools/console: Use const whenever we point to literal strings

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Literal strings are not meant to be modified. So we should use const
char * rather than char * when we want to store a pointer to them.

Take the opportunity to remove the cast (char *) in console_init(). It
is unnecessary and will remove the const.

Signed-off-by: Julien Grall <jgrall@amazon.com>
Acked-by: Wei Liu <wl@xen.org>

---
    Changes in v2:
        - Remove the cast (char *) in console_init()
        - Add Wei's acked-by
---
 tools/console/client/main.c |  4 ++--
 tools/console/daemon/io.c   | 15 ++++++++-------
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/tools/console/client/main.c b/tools/console/client/main.c
index 088be28dff02..80157be42144 100644
--- a/tools/console/client/main.c
+++ b/tools/console/client/main.c
@@ -325,7 +325,7 @@ int main(int argc, char **argv)
 {
 	struct termios attr;
 	int domid;
-	char *sopt = "hn:";
+	const char *sopt = "hn:";
 	int ch;
 	unsigned int num = 0;
 	int opt_ind=0;
@@ -345,7 +345,7 @@ int main(int argc, char **argv)
 	char *end;
 	console_type type = CONSOLE_INVAL;
 	bool interactive = 0;
-	char *console_names = "serial, pv, vuart";
+	const char *console_names = "serial, pv, vuart";
 
 	while((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
 		switch(ch) {
diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 4af27ffc5d02..200b575d76f8 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -87,14 +87,14 @@ struct buffer {
 };
 
 struct console {
-	char *ttyname;
+	const char *ttyname;
 	int master_fd;
 	int master_pollfd_idx;
 	int slave_fd;
 	int log_fd;
 	struct buffer buffer;
 	char *xspath;
-	char *log_suffix;
+	const char *log_suffix;
 	int ring_ref;
 	xenevtchn_handle *xce_handle;
 	int xce_pollfd_idx;
@@ -109,9 +109,9 @@ struct console {
 };
 
 struct console_type {
-	char *xsname;
-	char *ttyname;
-	char *log_suffix;
+	const char *xsname;
+	const char *ttyname;
+	const char *log_suffix;
 	bool optional;
 	bool use_gnttab;
 };
@@ -813,7 +813,8 @@ static int console_init(struct console *con, struct domain *dom, void **data)
 	int err = -1;
 	struct timespec ts;
 	struct console_type **con_type = (struct console_type **)data;
-	char *xsname, *xspath;
+	const char *xsname;
+	char *xspath;
 
 	if (clock_gettime(CLOCK_MONOTONIC, &ts) < 0) {
 		dolog(LOG_ERR, "Cannot get time of day %s:%s:L%d",
@@ -835,7 +836,7 @@ static int console_init(struct console *con, struct domain *dom, void **data)
 	con->log_suffix = (*con_type)->log_suffix;
 	con->optional = (*con_type)->optional;
 	con->use_gnttab = (*con_type)->use_gnttab;
-	xsname = (char *)(*con_type)->xsname;
+	xsname = (*con_type)->xsname;
 	xspath = xs_get_domain_path(xs, dom->domid);
 	s = realloc(xspath, strlen(xspath) +
 		    strlen(xsname) + 1);
-- 
2.17.1

Re: [PATCH v2 2/2] tools/console: Use const whenever we point to literal strings

[Anthony PERARD]

On Tue, May 18, 2021 at 03:01:34PM +0100, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Literal strings are not meant to be modified. So we should use const
> char * rather than char * when we want to store a pointer to them.
> 
> Take the opportunity to remove the cast (char *) in console_init(). It
> is unnecessary and will remove the const.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> Acked-by: Wei Liu <wl@xen.org>
> 
> ---
>     Changes in v2:
>         - Remove the cast (char *) in console_init()

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Re: [PATCH v2 0/2] Use const whenever we point to literal strings (take 1)

[Julien Grall]

On 18/05/2021 15:01, 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 non-controversial use of const in the code.
> 
> Julien Grall (2):
>    xen/char: console: Use const whenever we point to literal strings
>    tools/console: Use const whenever we point to literal strings

I have committed the two patches.

> 
>   tools/console/client/main.c |  4 ++--
>   tools/console/daemon/io.c   | 15 ++++++++-------
>   xen/drivers/char/console.c  |  7 ++++---
>   3 files changed, 14 insertions(+), 12 deletions(-)
> 

-- 
Julien Grall