Re: [PATCH v3] Check and report for incomplete 'global' option format

[Rohit Kumar]

Hi Markus, thanks for the review. Please let me know if this patch needs 
to be rebased on top of current master or does it looks good to merge.

Thanks !

On 16/02/22 7:25 pm, Markus Armbruster wrote:

Rohit Kumar  writes:


Qemu might crash when provided incomplete '-global' option.
For example:
  qemu-system-x86_64 -global driver=isa-fdc
  qemu-system-x86_64: ../../devel/qemu/qapi/string-input-visitor.c:394:
  string_input_visitor_new: Assertion `str' failed.
  Aborted (core dumped)

Fixes: 3751d7c43f795b ("vl: allow full-blown QemuOpts syntax for -global")
Resolves: 
https://urldefense.proofpoint.com/v2/url?u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_issues_604&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=ABSkr7gy7ZTfApFfI-Xxt1gZNtsDDiXoXOXc0OrkyFs&m=_FT9FHpCayLV7VOqTV1sshekKFR0H-be14Rx8GwuhkF6FyEaMtUWc0vvbuoZOJP1&s=yH_2KUONf-QJFFyoSnAGOJIzyhREMalkQuli_BY-y4U&e=
Signed-off-by: Rohit Kumar 
---
  diff to v2:
   - Avoided double reporting of error.
   - Added the "Fixes" line in the commit message.

  softmmu/qdev-monitor.c | 7 +++
  1 file changed, 7 insertions(+)

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 01f3834db5..e918ab8bf3 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -1034,6 +1034,13 @@ int qemu_global_option(const char *str)
  if (!opts) {
  return -1;
  }
+if (!qemu_opt_get(opts, "driver")
+|| !qemu_opt_get(opts, "property")
+|| !qemu_opt_get(opts, "value")) {
+error_report("options 'driver', 'property', and 'value'"
+ " are required");
+return -1;
+}
  
  return 0;

  }

Reviewed-by: Markus Armbruster 

[PATCH v3] Check and report for incomplete 'global' option format

[Rohit Kumar]

Qemu might crash when provided incomplete '-global' option.
For example:
 qemu-system-x86_64 -global driver=isa-fdc
 qemu-system-x86_64: ../../devel/qemu/qapi/string-input-visitor.c:394:
 string_input_visitor_new: Assertion `str' failed.
 Aborted (core dumped)

Fixes: 3751d7c43f795b ("vl: allow full-blown QemuOpts syntax for -global")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/604
Signed-off-by: Rohit Kumar 
---
 diff to v2:
  - Avoided double reporting of error.
  - Added the "Fixes" line in the commit message.

 softmmu/qdev-monitor.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 01f3834db5..e918ab8bf3 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -1034,6 +1034,13 @@ int qemu_global_option(const char *str)
 if (!opts) {
 return -1;
 }
+if (!qemu_opt_get(opts, "driver")
+|| !qemu_opt_get(opts, "property")
+|| !qemu_opt_get(opts, "value")) {
+error_report("options 'driver', 'property', and 'value'"
+ " are required");
+return -1;
+}
 
 return 0;
 }
-- 
2.25.1

Re: [PATCH v2] Check and report for incomplete 'global' option format

[Rohit Kumar]

On 15/02/22 3:00 pm, Markus Armbruster wrote:

Rohit Kumar  writes:


Qemu might crash when provided incomplete '-global' option.
For example:
 qemu-system-x86_64 -global driver=isa-fdc
 qemu-system-x86_64: ../../devel/qemu/qapi/string-input-visitor.c:394:
 string_input_visitor_new: Assertion `str' failed.
 Aborted (core dumped)

Resolves: 
https://urldefense.proofpoint.com/v2/url?u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_issues_604&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=ABSkr7gy7ZTfApFfI-Xxt1gZNtsDDiXoXOXc0OrkyFs&m=YfuOt9ozXVjjleGOXTdWK4Hu8PoU3kTzGTXAJ223138_AM934NRnuqUQHpaLSWTs&s=WEGcm58m7pR4XrWGsUvHOVPj18ym0OrlAObwY_CfKlc&e=

The original qemu_global_option() only ever created QemuOpts with all
three options present.  Code consuming these QemuOpts relies on this
invariant.  Commit 3751d7c43f "vl: allow full-blown QemuOpts syntax for
-global" (v2.4.0) wrecked it.

Let's point to the root cause:

   Fixes: 3751d7c43f795b45ffdb9429cfb09c6beea55c68

Thanks, Markus for pointing out this. I will add it in the next patch.



Signed-off-by: Rohit Kumar 
---
  diff to v1:
   - Removed '\n' from error log message.

  softmmu/qdev-monitor.c | 8 +++-
  1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 01f3834db5..51b33caeca 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -1020,6 +1020,7 @@ int qemu_global_option(const char *str)
  char driver[64], property[64];
  QemuOpts *opts;
  int rc, offset;
+Error *err = NULL;
  
  rc = sscanf(str, "%63[^.=].%63[^=]%n", driver, property, &offset);

  if (rc == 2 && str[offset] == '=') {
@@ -1031,7 +1032,12 @@ int qemu_global_option(const char *str)
  }
  
  opts = qemu_opts_parse_noisily(&qemu_global_opts, str, false);

-if (!opts) {
+if (!opts || !qemu_opt_get(opts, "driver") || 
!qemu_opt_get(opts,"property") ||
+!qemu_opt_get(opts, "value")) {
+error_setg(&err, "Invalid 'global' option format! "
+"Use -global .= or "
+"-global driver=driver,property=property,value=value");
+error_report_err(err);
  return -1;
  }

This fix isn't quite right.

When qemu_opts_parse_noisily() fails, it reports an error and returns
null.  Your patch reports a second error then.  Reproducer:

 $ qemu-system-x86_64 -global =
 qemu-system-x86_64: -global =: Invalid parameter ''
 qemu-system-x86_64: -global =: Invalid 'global' option format! Use -global 
.= or -global 
driver=driver,property=property,value=value

You should do something like

 opts = qemu_opts_parse_noisily(&qemu_global_opts, str, false);
 if (!opts) {
 return -1;
 }
 if (!qemu_opt_get(opts, "driver")
 || !qemu_opt_get(opts, "property")
 || !qemu_opt_get(opts, "value")) {
 error_report("options 'driver', 'property', and 'value'"
  " are required');
 return -1;
 }

Ack., I will update it in the next patch. Thanks.

[PATCH v2] Check and report for incomplete 'global' option format

[Rohit Kumar]

Qemu might crash when provided incomplete '-global' option.
For example:
qemu-system-x86_64 -global driver=isa-fdc
qemu-system-x86_64: ../../devel/qemu/qapi/string-input-visitor.c:394:
string_input_visitor_new: Assertion `str' failed.
Aborted (core dumped)

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/604
Signed-off-by: Rohit Kumar 
---
 diff to v1:
  - Removed '\n' from error log message.

 softmmu/qdev-monitor.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 01f3834db5..51b33caeca 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -1020,6 +1020,7 @@ int qemu_global_option(const char *str)
 char driver[64], property[64];
 QemuOpts *opts;
 int rc, offset;
+Error *err = NULL;
 
 rc = sscanf(str, "%63[^.=].%63[^=]%n", driver, property, &offset);
 if (rc == 2 && str[offset] == '=') {
@@ -1031,7 +1032,12 @@ int qemu_global_option(const char *str)
 }
 
 opts = qemu_opts_parse_noisily(&qemu_global_opts, str, false);
-if (!opts) {
+if (!opts || !qemu_opt_get(opts, "driver") || 
!qemu_opt_get(opts,"property") ||
+!qemu_opt_get(opts, "value")) {
+error_setg(&err, "Invalid 'global' option format! "
+"Use -global .= or "
+"-global driver=driver,property=property,value=value");
+error_report_err(err);
 return -1;
 }
 
-- 
2.25.1

Re: [PATCH v1] Check and report for incomplete 'global' option format

[Rohit Kumar]

Ping.

Hi, please review this patch.
Link: https://lists.nongnu.org/archive/html/qemu-devel/2022-01/msg00296.html

Thanks !

On 04/01/22 7:11 pm, Rohit Kumar wrote:

Qemu might crash when provided incomplete '-global' option.
For example:
qemu-system-x86_64 -global driver=isa-fdc
qemu-system-x86_64: ../../devel/qemu/qapi/string-input-visitor.c:394:
string_input_visitor_new: Assertion `str' failed.
Aborted (core dumped)

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/604
Signed-off-by: Rohit Kumar 
---
  softmmu/qdev-monitor.c | 9 -
  1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 01f3834db5..7aee7b9882 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -1020,6 +1020,7 @@ int qemu_global_option(const char *str)
  char driver[64], property[64];
  QemuOpts *opts;
  int rc, offset;
+Error *err = NULL;
  
  rc = sscanf(str, "%63[^.=].%63[^=]%n", driver, property, &offset);

  if (rc == 2 && str[offset] == '=') {
@@ -1031,7 +1032,13 @@ int qemu_global_option(const char *str)
  }
  
  opts = qemu_opts_parse_noisily(&qemu_global_opts, str, false);

-if (!opts) {
+if (!opts || !qemu_opt_get(opts, "driver") || !qemu_opt_get(opts, 
"property") ||
+!qemu_opt_get(opts, "value")) {
+error_setg(&err, "Invalid 'global' option format\n"
+   "Expected: -global .= or "
+   "-global driver=driver,property=property,value=value\n"
+   "Received: -global %s", str);
+error_report_err(err);
  return -1;
  }
  

Re: [PATCH v1] Check and report for incomplete 'global' option format

[Rohit Kumar]

Ping. Please take a look at this patch.
Link: https://lists.nongnu.org/archive/html/qemu-devel/2022-01/msg00296.html

On 04/01/22 7:22 pm, Philippe Mathieu-Daudé wrote:

Cc'ing Markus / Thomas

On 4/1/22 14:41, Rohit Kumar wrote:

Qemu might crash when provided incomplete '-global' option.
For example:
qemu-system-x86_64 -global driver=isa-fdc
qemu-system-x86_64: 
../../devel/qemu/qapi/string-input-visitor.c:394:

   string_input_visitor_new: Assertion `str' failed.
  Aborted (core dumped)

Resolves: 
https://urldefense.proofpoint.com/v2/url?u=https-3A__gitlab.com_qemu-2Dproject_qemu_-2D_issues_604&d=DwICaQ&c=s883GpUCOChKOHiocYtGcg&r=ABSkr7gy7ZTfApFfI-Xxt1gZNtsDDiXoXOXc0OrkyFs&m=D981HXzQDqFNmq9tQqGqTedyzdOsi9F2fju4ltYq8HjOwS9Le2sJAgk09AWgxQg-&s=AGQxOHDyd9OPvXiP4hHLIb4FptkdlQFHDf2Xtadueo0&e= 
Signed-off-by: Rohit Kumar 

---
  softmmu/qdev-monitor.c | 9 -
  1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 01f3834db5..7aee7b9882 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -1020,6 +1020,7 @@ int qemu_global_option(const char *str)
  char driver[64], property[64];
  QemuOpts *opts;
  int rc, offset;
+    Error *err = NULL;
    rc = sscanf(str, "%63[^.=].%63[^=]%n", driver, property, 
&offset);

  if (rc == 2 && str[offset] == '=') {
@@ -1031,7 +1032,13 @@ int qemu_global_option(const char *str)
  }
    opts = qemu_opts_parse_noisily(&qemu_global_opts, str, false);
-    if (!opts) {
+    if (!opts || !qemu_opt_get(opts, "driver") || 
!qemu_opt_get(opts, "property") ||

+    !qemu_opt_get(opts, "value")) {
+    error_setg(&err, "Invalid 'global' option format\n"
+   "Expected: -global .= or "
+   "-global 
driver=driver,property=property,value=value\n"

+   "Received: -global %s", str);
+    error_report_err(err);
  return -1;
  }

[PATCH v1] Check and report for incomplete 'global' option format

[Rohit Kumar]

Qemu might crash when provided incomplete '-global' option.
For example:
qemu-system-x86_64 -global driver=isa-fdc
qemu-system-x86_64: ../../devel/qemu/qapi/string-input-visitor.c:394:
string_input_visitor_new: Assertion `str' failed.
Aborted (core dumped)

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/604
Signed-off-by: Rohit Kumar 
---
 softmmu/qdev-monitor.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index 01f3834db5..7aee7b9882 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -1020,6 +1020,7 @@ int qemu_global_option(const char *str)
 char driver[64], property[64];
 QemuOpts *opts;
 int rc, offset;
+Error *err = NULL;
 
 rc = sscanf(str, "%63[^.=].%63[^=]%n", driver, property, &offset);
 if (rc == 2 && str[offset] == '=') {
@@ -1031,7 +1032,13 @@ int qemu_global_option(const char *str)
 }
 
 opts = qemu_opts_parse_noisily(&qemu_global_opts, str, false);
-if (!opts) {
+if (!opts || !qemu_opt_get(opts, "driver") || !qemu_opt_get(opts, 
"property") ||
+!qemu_opt_get(opts, "value")) {
+error_setg(&err, "Invalid 'global' option format\n"
+   "Expected: -global .= or "
+   "-global driver=driver,property=property,value=value\n"
+   "Received: -global %s", str);
+error_report_err(err);
 return -1;
 }
 
-- 
2.27.0