| Message ID | 20210610084741.456260-1-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vl: Fix an assert failure in error path | expand |
On 10/06/21 10:47, Zhenzhong Duan wrote: > Based on the description of error_setg(), the local variable err in > qemu_maybe_daemonize() should be initialized to NULL. > > Without fix, the uninitialized *errp triggers assert failure which > doesn't show much valuable information. > > Before the fix: > qemu-system-x86_64: ../util/error.c:59: error_setv: Assertion `*errp == NULL' failed. > > After fix: > qemu-system-x86_64: cannot create PID file: Cannot open pid file: Permission denied > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > softmmu/vl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/softmmu/vl.c b/softmmu/vl.c > index 326c1e9080..feb4d201f3 100644 > --- a/softmmu/vl.c > +++ b/softmmu/vl.c > @@ -2522,7 +2522,7 @@ static void qemu_process_help_options(void) > > static void qemu_maybe_daemonize(const char *pid_file) > { > - Error *err; > + Error *err = NULL; > > os_daemonize(); > rcu_disable_atfork(); > Queued, thanks. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 10/06/21 10:47, Zhenzhong Duan wrote: >> Based on the description of error_setg(), the local variable err in >> qemu_maybe_daemonize() should be initialized to NULL. >> Without fix, the uninitialized *errp triggers assert failure which >> doesn't show much valuable information. >> Before the fix: >> qemu-system-x86_64: ../util/error.c:59: error_setv: Assertion `*errp == NULL' failed. >> After fix: >> qemu-system-x86_64: cannot create PID file: Cannot open pid file: Permission denied >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> softmmu/vl.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> diff --git a/softmmu/vl.c b/softmmu/vl.c >> index 326c1e9080..feb4d201f3 100644 >> --- a/softmmu/vl.c >> +++ b/softmmu/vl.c >> @@ -2522,7 +2522,7 @@ static void qemu_process_help_options(void) >> static void qemu_maybe_daemonize(const char *pid_file) >> { >> - Error *err; >> + Error *err = NULL; Common mistake, I'm afraid. >> os_daemonize(); >> rcu_disable_atfork(); >> > > Queued, thanks. Suggest to amend the commit message with Fixes: 03d2b412aaf2078425f8472f31c8a9c2340969eb
On Wed, Jun 09, 2021 at 02:09:47PM +0200, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > > > On 10/06/21 10:47, Zhenzhong Duan wrote: > >> Based on the description of error_setg(), the local variable err in > >> qemu_maybe_daemonize() should be initialized to NULL. > >> Without fix, the uninitialized *errp triggers assert failure which > >> doesn't show much valuable information. > >> Before the fix: > >> qemu-system-x86_64: ../util/error.c:59: error_setv: Assertion `*errp == NULL' failed. > >> After fix: > >> qemu-system-x86_64: cannot create PID file: Cannot open pid file: Permission denied > >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > >> --- > >> softmmu/vl.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> diff --git a/softmmu/vl.c b/softmmu/vl.c > >> index 326c1e9080..feb4d201f3 100644 > >> --- a/softmmu/vl.c > >> +++ b/softmmu/vl.c > >> @@ -2522,7 +2522,7 @@ static void qemu_process_help_options(void) > >> static void qemu_maybe_daemonize(const char *pid_file) > >> { > >> - Error *err; > >> + Error *err = NULL; > > Common mistake, I'm afraid. Initializing isn't likely to be a performance impact, so I'd think we should make 'checkpatch.pl' complain about any 'Error *' variable that is not initialized to NULL, as a safety net, even if not technically required in some cases. Regards, Daniel
On 6/9/2021 8:15 PM, Daniel P. Berrangé wrote: > On Wed, Jun 09, 2021 at 02:09:47PM +0200, Markus Armbruster wrote: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> On 10/06/21 10:47, Zhenzhong Duan wrote: >>>> Based on the description of error_setg(), the local variable err in >>>> qemu_maybe_daemonize() should be initialized to NULL. >>>> Without fix, the uninitialized *errp triggers assert failure which >>>> doesn't show much valuable information. >>>> Before the fix: >>>> qemu-system-x86_64: ../util/error.c:59: error_setv: Assertion `*errp == NULL' failed. >>>> After fix: >>>> qemu-system-x86_64: cannot create PID file: Cannot open pid file: Permission denied >>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>> --- >>>> softmmu/vl.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> diff --git a/softmmu/vl.c b/softmmu/vl.c >>>> index 326c1e9080..feb4d201f3 100644 >>>> --- a/softmmu/vl.c >>>> +++ b/softmmu/vl.c >>>> @@ -2522,7 +2522,7 @@ static void qemu_process_help_options(void) >>>> static void qemu_maybe_daemonize(const char *pid_file) >>>> { >>>> - Error *err; >>>> + Error *err = NULL; >> >> Common mistake, I'm afraid. > > Initializing isn't likely to be a performance impact, so I'd think > we should make 'checkpatch.pl' complain about any 'Error *' variable > that is not initialized to NULL, as a safety net, even if not technically > required in some cases. > > Regards, > Daniel > Hi, Could we add a coccinelle script to check (and fix) these problems? e.g.: @ r @ identifier id; @@ Error *id + = NULL ; Using this script, I found that local variable err in qemu_init_subsystems is not initialized to NULL too. The script is not prefect though, it will initialize all global/static 'Error *' variables and all local 'Error *' variables in util/error.c to NULL, which is unnecessary. Thanks, Peng
Peng Liang <liangpeng10@huawei.com> writes: > On 6/9/2021 8:15 PM, Daniel P. Berrangé wrote: >> On Wed, Jun 09, 2021 at 02:09:47PM +0200, Markus Armbruster wrote: >>> Paolo Bonzini <pbonzini@redhat.com> writes: >>> >>>> On 10/06/21 10:47, Zhenzhong Duan wrote: >>>>> Based on the description of error_setg(), the local variable err in >>>>> qemu_maybe_daemonize() should be initialized to NULL. >>>>> Without fix, the uninitialized *errp triggers assert failure which >>>>> doesn't show much valuable information. >>>>> Before the fix: >>>>> qemu-system-x86_64: ../util/error.c:59: error_setv: Assertion `*errp == NULL' failed. >>>>> After fix: >>>>> qemu-system-x86_64: cannot create PID file: Cannot open pid file: Permission denied >>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>>> --- >>>>> softmmu/vl.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> diff --git a/softmmu/vl.c b/softmmu/vl.c >>>>> index 326c1e9080..feb4d201f3 100644 >>>>> --- a/softmmu/vl.c >>>>> +++ b/softmmu/vl.c >>>>> @@ -2522,7 +2522,7 @@ static void qemu_process_help_options(void) >>>>> static void qemu_maybe_daemonize(const char *pid_file) >>>>> { >>>>> - Error *err; >>>>> + Error *err = NULL; >>> >>> Common mistake, I'm afraid. >> >> Initializing isn't likely to be a performance impact, so I'd think >> we should make 'checkpatch.pl' complain about any 'Error *' variable >> that is not initialized to NULL, as a safety net, even if not technically >> required in some cases. >> >> Regards, >> Daniel >> > > Hi, > Could we add a coccinelle script to check (and fix) these problems? e.g.: Coccinelle is good for finding and fixing instances of bad patterns that have crept in. checkpatch is good for keeping them out. Both has its uses. > @ r @ > identifier id; > @@ > Error *id > + = NULL > ; > > Using this script, I found that local variable err in > qemu_init_subsystems is not initialized to NULL too. Crash bug, broken in commit efd7ab22fb "vl: extract qemu_init_subsystems", v6.0.0. Care to submit a fix? > The script is not > prefect though, it will initialize all global/static 'Error *' variables > and all local 'Error *' variables in util/error.c to NULL, which is > unnecessary. Excluding util/error.c is easy once you know how to: @ depends on !(file in "util/error.c")@ identifier id; @@ ... Excluding variable definitions with static storage duraction shouldn't be too hard, either. If Coccinelle is sufficiently clever, adding keyword auto suffices. Else, try matching keyword static separately, like so: ( static Error *id; | - Error *id; + Error *id = NULL; ) Completely untested.
On 09/06/21 14:09, Markus Armbruster wrote:
> Fixes: 03d2b412aaf2078425f8472f31c8a9c2340969eb
Actually 0546c0609c ("vl: split various early command line options to a
separate function", 2020-12-10).
Done, thanks!
Paolo
On 6/10/2021 3:32 PM, Markus Armbruster wrote: > Peng Liang <liangpeng10@huawei.com> writes: > >> On 6/9/2021 8:15 PM, Daniel P. Berrangé wrote: >>> On Wed, Jun 09, 2021 at 02:09:47PM +0200, Markus Armbruster wrote: >>>> Paolo Bonzini <pbonzini@redhat.com> writes: >>>> >>>>> On 10/06/21 10:47, Zhenzhong Duan wrote: >>>>>> Based on the description of error_setg(), the local variable err in >>>>>> qemu_maybe_daemonize() should be initialized to NULL. >>>>>> Without fix, the uninitialized *errp triggers assert failure which >>>>>> doesn't show much valuable information. >>>>>> Before the fix: >>>>>> qemu-system-x86_64: ../util/error.c:59: error_setv: Assertion `*errp == NULL' failed. >>>>>> After fix: >>>>>> qemu-system-x86_64: cannot create PID file: Cannot open pid file: Permission denied >>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>>>> --- >>>>>> softmmu/vl.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> diff --git a/softmmu/vl.c b/softmmu/vl.c >>>>>> index 326c1e9080..feb4d201f3 100644 >>>>>> --- a/softmmu/vl.c >>>>>> +++ b/softmmu/vl.c >>>>>> @@ -2522,7 +2522,7 @@ static void qemu_process_help_options(void) >>>>>> static void qemu_maybe_daemonize(const char *pid_file) >>>>>> { >>>>>> - Error *err; >>>>>> + Error *err = NULL; >>>> >>>> Common mistake, I'm afraid. >>> >>> Initializing isn't likely to be a performance impact, so I'd think >>> we should make 'checkpatch.pl' complain about any 'Error *' variable >>> that is not initialized to NULL, as a safety net, even if not technically >>> required in some cases. >>> >>> Regards, >>> Daniel >>> >> >> Hi, >> Could we add a coccinelle script to check (and fix) these problems? e.g.: > > Coccinelle is good for finding and fixing instances of bad patterns that > have crept in. checkpatch is good for keeping them out. Both has its > uses. > >> @ r @ >> identifier id; >> @@ >> Error *id >> + = NULL >> ; >> >> Using this script, I found that local variable err in >> qemu_init_subsystems is not initialized to NULL too. > > Crash bug, broken in commit efd7ab22fb "vl: extract > qemu_init_subsystems", v6.0.0. Care to submit a fix? Have sent :) > >> The script is not >> prefect though, it will initialize all global/static 'Error *' variables >> and all local 'Error *' variables in util/error.c to NULL, which is >> unnecessary. > > Excluding util/error.c is easy once you know how to: > > @ depends on !(file in "util/error.c")@ > identifier id; > @@ > ... > > Excluding variable definitions with static storage duraction shouldn't > be too hard, either. If Coccinelle is sufficiently clever, adding > keyword auto suffices. Else, try matching keyword static separately, > like so: > > ( > static Error *id; > | > - Error *id; > + Error *id = NULL; > ) > > Completely untested. > Thanks for your improvement! Thanks, Peng
diff --git a/softmmu/vl.c b/softmmu/vl.c index 326c1e9080..feb4d201f3 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -2522,7 +2522,7 @@ static void qemu_process_help_options(void) static void qemu_maybe_daemonize(const char *pid_file) { - Error *err; + Error *err = NULL; os_daemonize(); rcu_disable_atfork();
Based on the description of error_setg(), the local variable err in qemu_maybe_daemonize() should be initialized to NULL. Without fix, the uninitialized *errp triggers assert failure which doesn't show much valuable information. Before the fix: qemu-system-x86_64: ../util/error.c:59: error_setv: Assertion `*errp == NULL' failed. After fix: qemu-system-x86_64: cannot create PID file: Cannot open pid file: Permission denied Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- softmmu/vl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)