[4/4] unix: don't use %m in pr_warn

Submitted by Andrei Vagin on May 15, 2018, 10:55 p.m.

Details

Message ID 20180515225559.7309-4-avagin@virtuozzo.com
State New
Series "Series without cover letter"
Headers show

Commit Message

Andrei Vagin May 15, 2018, 10:55 p.m.
pr_warn calls a few system calls before printing a message,
so it will override errno.

CID 190176 (#1 of 1): Printf format string issue (PW.BAD_PRINTF_FORMAT_STRING)
1. bad_printf_format_string: invalid format string conversion

Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
---
 criu/sk-unix.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/criu/sk-unix.c b/criu/sk-unix.c
index ea314bb9f..2838958d6 100644
--- a/criu/sk-unix.c
+++ b/criu/sk-unix.c
@@ -595,8 +595,8 @@  static int unix_resolve_name(int lfd, u32 id, struct unix_sk_desc *d,
 	snprintf(rpath, sizeof(rpath), ".%s", name);
 	if (fstatat(mntns_root, rpath, &st, 0)) {
 		if (errno != ENOENT) {
-			pr_warn("Can't stat socket %#x(%s), skipping: %m (err %d)\n",
-				id, rpath, errno);
+			pr_warn("Can't stat socket %#x(%s), skipping: %s (err %d)\n",
+				id, rpath, strerror(errno), errno);
 			goto skip;
 		}
 

Comments

Dmitry Safonov May 15, 2018, 11:10 p.m.
2018-05-15 23:55 GMT+01:00 Andrei Vagin <avagin@virtuozzo.com>:
> pr_warn calls a few system calls before printing a message,
> so it will override errno.
>
> CID 190176 (#1 of 1): Printf format string issue (PW.BAD_PRINTF_FORMAT_STRING)
> 1. bad_printf_format_string: invalid format string conversion
>
> Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
> ---
>  criu/sk-unix.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/criu/sk-unix.c b/criu/sk-unix.c
> index ea314bb9f..2838958d6 100644
> --- a/criu/sk-unix.c
> +++ b/criu/sk-unix.c
> @@ -595,8 +595,8 @@ static int unix_resolve_name(int lfd, u32 id, struct unix_sk_desc *d,
>         snprintf(rpath, sizeof(rpath), ".%s", name);
>         if (fstatat(mntns_root, rpath, &st, 0)) {
>                 if (errno != ENOENT) {
> -                       pr_warn("Can't stat socket %#x(%s), skipping: %m (err %d)\n",
> -                               id, rpath, errno);
> +                       pr_warn("Can't stat socket %#x(%s), skipping: %s (err %d)\n",
> +                               id, rpath, strerror(errno), errno);

I don't get this one.
pr_warn()=>print_on_level()=>vprint_on_level():
:        int __errno = errno;

Care to explain?
I might not spot something obvious, then should we correct all pr_warn/pr_*,
like the first in the same file:
:                pr_warn("Unable to open a socket file: %m\n");

Probably, Coverity just doesn't know %m modifier, not really sure if it worth
to correct ALL %m in criu..

Thanks,
             Dmitry
Andrei Vagin May 15, 2018, 11:22 p.m.
On Wed, May 16, 2018 at 12:10:04AM +0100, Dmitry Safonov wrote:
> 2018-05-15 23:55 GMT+01:00 Andrei Vagin <avagin@virtuozzo.com>:
> > pr_warn calls a few system calls before printing a message,
> > so it will override errno.
> >
> > CID 190176 (#1 of 1): Printf format string issue (PW.BAD_PRINTF_FORMAT_STRING)
> > 1. bad_printf_format_string: invalid format string conversion
> >
> > Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
> > ---
> >  criu/sk-unix.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/criu/sk-unix.c b/criu/sk-unix.c
> > index ea314bb9f..2838958d6 100644
> > --- a/criu/sk-unix.c
> > +++ b/criu/sk-unix.c
> > @@ -595,8 +595,8 @@ static int unix_resolve_name(int lfd, u32 id, struct unix_sk_desc *d,
> >         snprintf(rpath, sizeof(rpath), ".%s", name);
> >         if (fstatat(mntns_root, rpath, &st, 0)) {
> >                 if (errno != ENOENT) {
> > -                       pr_warn("Can't stat socket %#x(%s), skipping: %m (err %d)\n",
> > -                               id, rpath, errno);
> > +                       pr_warn("Can't stat socket %#x(%s), skipping: %s (err %d)\n",
> > +                               id, rpath, strerror(errno), errno);
> 
> I don't get this one.
> pr_warn()=>print_on_level()=>vprint_on_level():
> :        int __errno = errno;
> 
> Care to explain?

vprint_on_level
	print_ts
		gettimeofday
		snprintf
	        size  = vsnprintf(buffer + buf_off, sizeof buffer - buf_off, format, params);


gettimeofday() and snprintf() can change errno. We probably can fix this
by restoring errno before vsnprintf, but I would prefer to use stderror()
to avoid side effects.

> I might not spot something obvious, then should we correct all pr_warn/pr_*,
> like the first in the same file:
> :                pr_warn("Unable to open a socket file: %m\n");
> 
> Probably, Coverity just doesn't know %m modifier, not really sure if it worth
> to correct ALL %m in criu..

I don't know why it shows this warning here, but I remember that we have
issues with %m...

> 
> Thanks,
>              Dmitry
Andrei Vagin May 15, 2018, 11:25 p.m.
On Tue, May 15, 2018 at 04:22:15PM -0700, Andrei Vagin wrote:
> On Wed, May 16, 2018 at 12:10:04AM +0100, Dmitry Safonov wrote:
> > 2018-05-15 23:55 GMT+01:00 Andrei Vagin <avagin@virtuozzo.com>:
> > > pr_warn calls a few system calls before printing a message,
> > > so it will override errno.
> > >
> > > CID 190176 (#1 of 1): Printf format string issue (PW.BAD_PRINTF_FORMAT_STRING)
> > > 1. bad_printf_format_string: invalid format string conversion
> > >
> > > Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
> > > ---
> > >  criu/sk-unix.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/criu/sk-unix.c b/criu/sk-unix.c
> > > index ea314bb9f..2838958d6 100644
> > > --- a/criu/sk-unix.c
> > > +++ b/criu/sk-unix.c
> > > @@ -595,8 +595,8 @@ static int unix_resolve_name(int lfd, u32 id, struct unix_sk_desc *d,
> > >         snprintf(rpath, sizeof(rpath), ".%s", name);
> > >         if (fstatat(mntns_root, rpath, &st, 0)) {
> > >                 if (errno != ENOENT) {
> > > -                       pr_warn("Can't stat socket %#x(%s), skipping: %m (err %d)\n",
> > > -                               id, rpath, errno);
> > > +                       pr_warn("Can't stat socket %#x(%s), skipping: %s (err %d)\n",
> > > +                               id, rpath, strerror(errno), errno);
> > 
> > I don't get this one.
> > pr_warn()=>print_on_level()=>vprint_on_level():
> > :        int __errno = errno;
> > 
> > Care to explain?
> 
> vprint_on_level
> 	print_ts
> 		gettimeofday
> 		snprintf
> 	        size  = vsnprintf(buffer + buf_off, sizeof buffer - buf_off, format, params);
> 
> 
> gettimeofday() and snprintf() can change errno. We probably can fix this
> by restoring errno before vsnprintf, but I would prefer to use stderror()
> to avoid side effects.
> 
> > I might not spot something obvious, then should we correct all pr_warn/pr_*,
> > like the first in the same file:
> > :                pr_warn("Unable to open a socket file: %m\n");
> > 
> > Probably, Coverity just doesn't know %m modifier, not really sure if it worth
> > to correct ALL %m in criu..
> 
> I don't know why it shows this warning here, but I remember that we have
> issues with %m...

Acrually I know. In other cases, there is not other arguments except %m.

For example:

criu/cr-check.c:                        pr_msg("prctl: PR_SET_MM_EXE_FILE is not supported: %m\n");

> 
> > 
> > Thanks,
> >              Dmitry
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
Dmitry Safonov May 15, 2018, 11:30 p.m.
2018-05-16 0:25 GMT+01:00 Andrei Vagin <avagin@virtuozzo.com>:
> On Tue, May 15, 2018 at 04:22:15PM -0700, Andrei Vagin wrote:
>> On Wed, May 16, 2018 at 12:10:04AM +0100, Dmitry Safonov wrote:
>> > 2018-05-15 23:55 GMT+01:00 Andrei Vagin <avagin@virtuozzo.com>:
>> > > pr_warn calls a few system calls before printing a message,
>> > > so it will override errno.
>> > >
>> > > CID 190176 (#1 of 1): Printf format string issue (PW.BAD_PRINTF_FORMAT_STRING)
>> > > 1. bad_printf_format_string: invalid format string conversion
>> > >
>> > > Signed-off-by: Andrei Vagin <avagin@virtuozzo.com>
>> > > ---
>> > >  criu/sk-unix.c | 4 ++--
>> > >  1 file changed, 2 insertions(+), 2 deletions(-)
>> > >
>> > > diff --git a/criu/sk-unix.c b/criu/sk-unix.c
>> > > index ea314bb9f..2838958d6 100644
>> > > --- a/criu/sk-unix.c
>> > > +++ b/criu/sk-unix.c
>> > > @@ -595,8 +595,8 @@ static int unix_resolve_name(int lfd, u32 id, struct unix_sk_desc *d,
>> > >         snprintf(rpath, sizeof(rpath), ".%s", name);
>> > >         if (fstatat(mntns_root, rpath, &st, 0)) {
>> > >                 if (errno != ENOENT) {
>> > > -                       pr_warn("Can't stat socket %#x(%s), skipping: %m (err %d)\n",
>> > > -                               id, rpath, errno);
>> > > +                       pr_warn("Can't stat socket %#x(%s), skipping: %s (err %d)\n",
>> > > +                               id, rpath, strerror(errno), errno);
>> >
>> > I don't get this one.
>> > pr_warn()=>print_on_level()=>vprint_on_level():
>> > :        int __errno = errno;
>> >
>> > Care to explain?
>>
>> vprint_on_level
>>       print_ts
>>               gettimeofday
>>               snprintf
>>               size  = vsnprintf(buffer + buf_off, sizeof buffer - buf_off, format, params);
>>
>>
>> gettimeofday() and snprintf() can change errno. We probably can fix this
>> by restoring errno before vsnprintf, but I would prefer to use stderror()
>> to avoid side effects.

Hmm, ok, but still I don't follow why fixing only one particular place then..

>> > I might not spot something obvious, then should we correct all pr_warn/pr_*,
>> > like the first in the same file:
>> > :                pr_warn("Unable to open a socket file: %m\n");
>> >
>> > Probably, Coverity just doesn't know %m modifier, not really sure if it worth
>> > to correct ALL %m in criu..
>>
>> I don't know why it shows this warning here, but I remember that we have
>> issues with %m...
>
> Acrually I know. In other cases, there is not other arguments except %m.
>
> For example:
>
> criu/cr-check.c:                        pr_msg("prctl: PR_SET_MM_EXE_FILE is not supported: %m\n");

Yeah, so probably Coverity expects %d for errno, but there is '%m (err %d)'
and it thinks we messed up with this.
If it's about fixing Coverity warning - maybe just swap them?
'(err %d): %m'

I do not feel strongly about the patch, anyway, may go as-is.