Re: [sdl-qemu] [PATCH] fix leaks found wtih fuzzing
On 25.08.2023 12:29, Dmitry Frolov wrote:
> It is true, that there is no problem during runtime
> from the first sight, because the memmory is lost just
> before qemu exits. Nevertheless, this change is necessary,
> because AddressSanitizer is not able to recognize this
> situation and produces crash-report (which is
> false-positive in fact). Lots of False-Positive warnings
> are davaluing problems, found with fuzzing, and thus the
> whole methodology of dynamic analysis.
> This patch eliminates such False-Positive reports,
> and makes every problem, found with fuzzing, more valuable.
It would be good to separe answer to the previous mail and commit message.
>
> Fixes: 060ab76356 ("gtk: don't exit early in case gtk init fails")
>
> Signed-off-by: Dmitry Frolov
> ---
> v2: Moved declarations in the beginning.
>
> ui/gtk.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/ui/gtk.c b/ui/gtk.c
> index 8ba41c8f13..23a78787df 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -2360,7 +2360,7 @@ static void gtk_display_init(DisplayState *ds,
> DisplayOptions *opts)
> {
> VirtualConsole *vc;
>
> -GtkDisplayState *s = g_malloc0(sizeof(*s));
> +GtkDisplayState *s;
> GdkDisplay *window_display;
> GtkIconTheme *theme;
> char *dir;
> @@ -2372,6 +2372,7 @@ static void gtk_display_init(DisplayState *ds,
> DisplayOptions *opts)
> assert(opts->type == DISPLAY_TYPE_GTK);> s->opts = opts;
's' is already used here.
>
> +*s = g_malloc0(sizeof(*s));
s = g_malloc0(sizeof(*s));
> theme = gtk_icon_theme_get_default();
> dir = get_relocated_path(CONFIG_QEMU_ICONDIR);
> gtk_icon_theme_prepend_search_path(theme, dir);
Otherwise, I belive the change makes sense.
--
Alexey Khoroshilov
Linux Verification Center, ISPRAS
Re: [PATCH] fix leaks found wtih fuzzing
25.08.2023 14:58, Dmitry Frolov wrote:
It is true, that there is no problem during runtime
from the first sight, because the memory is lost just
before qemu exits. Nevertheless, this change is necessary,
because AddressSanitizer is not able to recognize this
situation and produces crash-report (which is
false-positive in fact). Lots of False-Positive warnings
are davaluing problems, found with fuzzing, and thus the
whole methodology of dynamic analysis.
This patch eliminates such False-Positive reports,
and makes every problem, found with fuzzing, more valuable.
Fixes: 060ab76356 ("gtk: don't exit early in case gtk init fails")
Signed-off-by: Dmitry Frolov
A nitpkck: I'm suggesting subject prefix to be "ui/gtk:".
With that fixed (can be done at apply time),
Reviewed-by: Michael Tokarev
/mjt
[PATCH] fix leaks found wtih fuzzing
It is true, that there is no problem during runtime
from the first sight, because the memory is lost just
before qemu exits. Nevertheless, this change is necessary,
because AddressSanitizer is not able to recognize this
situation and produces crash-report (which is
false-positive in fact). Lots of False-Positive warnings
are davaluing problems, found with fuzzing, and thus the
whole methodology of dynamic analysis.
This patch eliminates such False-Positive reports,
and makes every problem, found with fuzzing, more valuable.
Fixes: 060ab76356 ("gtk: don't exit early in case gtk init fails")
Signed-off-by: Dmitry Frolov
---
v2: Moved declarations in the beginning.
v3: Fixed errors in v2.
ui/gtk.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/ui/gtk.c b/ui/gtk.c
index 8ba41c8f13..7db972732b 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -2360,7 +2360,7 @@ static void gtk_display_init(DisplayState *ds,
DisplayOptions *opts)
{
VirtualConsole *vc;
-GtkDisplayState *s = g_malloc0(sizeof(*s));
+GtkDisplayState *s;
GdkDisplay *window_display;
GtkIconTheme *theme;
char *dir;
@@ -2370,6 +2370,7 @@ static void gtk_display_init(DisplayState *ds,
DisplayOptions *opts)
exit(1);
}
assert(opts->type == DISPLAY_TYPE_GTK);
+s = g_malloc0(sizeof(*s));
s->opts = opts;
theme = gtk_icon_theme_get_default();
--
2.34.1
[PATCH] fix leaks found wtih fuzzing
It is true, that there is no problem during runtime
from the first sight, because the memmory is lost just
before qemu exits. Nevertheless, this change is necessary,
because AddressSanitizer is not able to recognize this
situation and produces crash-report (which is
false-positive in fact). Lots of False-Positive warnings
are davaluing problems, found with fuzzing, and thus the
whole methodology of dynamic analysis.
This patch eliminates such False-Positive reports,
and makes every problem, found with fuzzing, more valuable.
Fixes: 060ab76356 ("gtk: don't exit early in case gtk init fails")
Signed-off-by: Dmitry Frolov
---
v2: Moved declarations in the beginning.
ui/gtk.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/ui/gtk.c b/ui/gtk.c
index 8ba41c8f13..23a78787df 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -2360,7 +2360,7 @@ static void gtk_display_init(DisplayState *ds,
DisplayOptions *opts)
{
VirtualConsole *vc;
-GtkDisplayState *s = g_malloc0(sizeof(*s));
+GtkDisplayState *s;
GdkDisplay *window_display;
GtkIconTheme *theme;
char *dir;
@@ -2372,6 +2372,7 @@ static void gtk_display_init(DisplayState *ds,
DisplayOptions *opts)
assert(opts->type == DISPLAY_TYPE_GTK);
s->opts = opts;
+*s = g_malloc0(sizeof(*s));
theme = gtk_icon_theme_get_default();
dir = get_relocated_path(CONFIG_QEMU_ICONDIR);
gtk_icon_theme_prepend_search_path(theme, dir);
--
2.34.1
Re: [PATCH] fix leaks found wtih fuzzing
On Thu, 24 Aug 2023 at 17:28, Dmitry Frolov wrote:
>
> Fuzzing causes thousands of identical crashes with message:
> "AddressSanitizer: 3744 byte(s) leaked in 1 allocation(s)"
>
> Fixes: 060ab76356 ("gtk: don't exit early in case gtk init fails")
>
> Signed-off-by: Dmitry Frolov
> ---
> ui/gtk.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/ui/gtk.c b/ui/gtk.c
> index 8ba41c8f13..996ca7949d 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -2358,6 +2358,10 @@ static gboolean gtkinit;
>
> static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
> {
> +if (!gtkinit) {
> +fprintf(stderr, "gtk initialization failed\n");
> +exit(1);
> +}
> VirtualConsole *vc;
This breaks our rule against having variable declarations
in the middle of code blocks. The variable declarations
need to come first, before this code.
More generally, I don't understand why this change is
necessary. If gtkinit is false, we're going to call
exit(), which will clean up all our allocations. The
specific allocation of the GtkDisplayState can hardly
be the only one that we still have allocated and
are relying on the cleanup-on-exit for.
> GtkDisplayState *s = g_malloc0(sizeof(*s));
> @@ -2365,10 +2369,6 @@ static void gtk_display_init(DisplayState *ds,
> DisplayOptions *opts)
> GtkIconTheme *theme;
> char *dir;
>
> -if (!gtkinit) {
> -fprintf(stderr, "gtk initialization failed\n");
> -exit(1);
> -}
> assert(opts->type == DISPLAY_TYPE_GTK);
> s->opts = opts;
thanks
-- PMM
[PATCH] fix leaks found wtih fuzzing
Fuzzing causes thousands of identical crashes with message:
"AddressSanitizer: 3744 byte(s) leaked in 1 allocation(s)"
Fixes: 060ab76356 ("gtk: don't exit early in case gtk init fails")
Signed-off-by: Dmitry Frolov
---
ui/gtk.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/ui/gtk.c b/ui/gtk.c
index 8ba41c8f13..996ca7949d 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -2358,6 +2358,10 @@ static gboolean gtkinit;
static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
{
+if (!gtkinit) {
+fprintf(stderr, "gtk initialization failed\n");
+exit(1);
+}
VirtualConsole *vc;
GtkDisplayState *s = g_malloc0(sizeof(*s));
@@ -2365,10 +2369,6 @@ static void gtk_display_init(DisplayState *ds,
DisplayOptions *opts)
GtkIconTheme *theme;
char *dir;
-if (!gtkinit) {
-fprintf(stderr, "gtk initialization failed\n");
-exit(1);
-}
assert(opts->type == DISPLAY_TYPE_GTK);
s->opts = opts;
--
2.34.1
