2019-05-28 00:49:12

by Gen Zhang

[permalink] [raw]
Subject: [PATCH v3] vt: Fix a missing-check bug in con_init()

In function con_init(), the pointer variable vc_cons[currcons].d, vc and
vc->vc_screenbuf is allocated by kzalloc(). And they are used in the
following codes. However, kzalloc() returns NULL when fails, and null
pointer dereference may happen. And it will cause the kernel to crash.
Therefore, we should check the return value and handle the error.

Further, since the allcoation is in a loop, we should free all the
allocated memory in a loop.

Signed-off-by: Gen Zhang <[email protected]>
Reviewed-by: Nicolas Pitre <[email protected]>
---
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index fdd12f8..d50f68f 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3350,10 +3350,14 @@ static int __init con_init(void)

for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), GFP_NOWAIT);
+ if (!vc)
+ goto fail1;
INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
tty_port_init(&vc->port);
visual_init(vc, currcons, 1);
vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT);
+ if (!vc->vc_screenbuf)
+ goto fail2;
vc_init(vc, vc->vc_rows, vc->vc_cols,
currcons || !vc->vc_sw->con_save_screen);
}
@@ -3375,6 +3379,16 @@ static int __init con_init(void)
register_console(&vt_console_driver);
#endif
return 0;
+fail1:
+ while (currcons > 0) {
+ currcons--;
+ kfree(vc_cons[currcons].d->vc_screenbuf);
+fail2:
+ kfree(vc_cons[currcons].d);
+ vc_cons[currcons].d = NULL;
+ }
+ console_unlock();
+ return -ENOMEM;
}
console_initcall(con_init);

---


2019-06-08 16:03:12

by Gen Zhang

[permalink] [raw]
Subject: Re: [PATCH v3] vt: Fix a missing-check bug in con_init()

On Tue, May 28, 2019 at 08:45:29AM +0800, Gen Zhang wrote:
> In function con_init(), the pointer variable vc_cons[currcons].d, vc and
> vc->vc_screenbuf is allocated by kzalloc(). And they are used in the
> following codes. However, kzalloc() returns NULL when fails, and null
> pointer dereference may happen. And it will cause the kernel to crash.
> Therefore, we should check the return value and handle the error.
>
> Further, since the allcoation is in a loop, we should free all the
> allocated memory in a loop.
>
> Signed-off-by: Gen Zhang <[email protected]>
> Reviewed-by: Nicolas Pitre <[email protected]>
> ---
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index fdd12f8..d50f68f 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -3350,10 +3350,14 @@ static int __init con_init(void)
>
> for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
> vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), GFP_NOWAIT);
> + if (!vc)
> + goto fail1;
> INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
> tty_port_init(&vc->port);
> visual_init(vc, currcons, 1);
> vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT);
> + if (!vc->vc_screenbuf)
> + goto fail2;
> vc_init(vc, vc->vc_rows, vc->vc_cols,
> currcons || !vc->vc_sw->con_save_screen);
> }
> @@ -3375,6 +3379,16 @@ static int __init con_init(void)
> register_console(&vt_console_driver);
> #endif
> return 0;
> +fail1:
> + while (currcons > 0) {
> + currcons--;
> + kfree(vc_cons[currcons].d->vc_screenbuf);
> +fail2:
> + kfree(vc_cons[currcons].d);
> + vc_cons[currcons].d = NULL;
> + }
> + console_unlock();
> + return -ENOMEM;
> }
> console_initcall(con_init);
>
> ---
Can anyone look into this patch? It's already reviewed by Nicolas Pitre
<[email protected]>.

Thanks
Gen

2019-06-08 16:23:04

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH v3] vt: Fix a missing-check bug in con_init()

On Sun, Jun 09, 2019 at 12:01:38AM +0800, Gen Zhang wrote:
> On Tue, May 28, 2019 at 08:45:29AM +0800, Gen Zhang wrote:
> > In function con_init(), the pointer variable vc_cons[currcons].d, vc and
> > vc->vc_screenbuf is allocated by kzalloc(). And they are used in the
> > following codes. However, kzalloc() returns NULL when fails, and null
> > pointer dereference may happen. And it will cause the kernel to crash.
> > Therefore, we should check the return value and handle the error.
> >
> > Further, since the allcoation is in a loop, we should free all the
> > allocated memory in a loop.
> >
> > Signed-off-by: Gen Zhang <[email protected]>
> > Reviewed-by: Nicolas Pitre <[email protected]>
> > ---
> > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > index fdd12f8..d50f68f 100644
> > --- a/drivers/tty/vt/vt.c
> > +++ b/drivers/tty/vt/vt.c
> > @@ -3350,10 +3350,14 @@ static int __init con_init(void)
> >
> > for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
> > vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), GFP_NOWAIT);
> > + if (!vc)
> > + goto fail1;
> > INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
> > tty_port_init(&vc->port);
> > visual_init(vc, currcons, 1);
> > vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT);
> > + if (!vc->vc_screenbuf)
> > + goto fail2;
> > vc_init(vc, vc->vc_rows, vc->vc_cols,
> > currcons || !vc->vc_sw->con_save_screen);
> > }
> > @@ -3375,6 +3379,16 @@ static int __init con_init(void)
> > register_console(&vt_console_driver);
> > #endif
> > return 0;
> > +fail1:
> > + while (currcons > 0) {
> > + currcons--;
> > + kfree(vc_cons[currcons].d->vc_screenbuf);
> > +fail2:
> > + kfree(vc_cons[currcons].d);
> > + vc_cons[currcons].d = NULL;
> > + }
> > + console_unlock();
> > + return -ENOMEM;
> > }
> > console_initcall(con_init);
> >
> > ---
> Can anyone look into this patch? It's already reviewed by Nicolas Pitre
> <[email protected]>.

It's in my queue. But note, given the previous history of your patches,
it's really low on my piority list at the moment :(

greg k-h

2019-06-08 16:24:38

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH v3] vt: Fix a missing-check bug in con_init()

On Sun, Jun 09, 2019 at 12:01:38AM +0800, Gen Zhang wrote:
> On Tue, May 28, 2019 at 08:45:29AM +0800, Gen Zhang wrote:
> > In function con_init(), the pointer variable vc_cons[currcons].d, vc and
> > vc->vc_screenbuf is allocated by kzalloc(). And they are used in the
> > following codes. However, kzalloc() returns NULL when fails, and null
> > pointer dereference may happen. And it will cause the kernel to crash.
> > Therefore, we should check the return value and handle the error.
> >
> > Further, since the allcoation is in a loop, we should free all the
> > allocated memory in a loop.
> >
> > Signed-off-by: Gen Zhang <[email protected]>
> > Reviewed-by: Nicolas Pitre <[email protected]>
> > ---
> > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > index fdd12f8..d50f68f 100644
> > --- a/drivers/tty/vt/vt.c
> > +++ b/drivers/tty/vt/vt.c
> > @@ -3350,10 +3350,14 @@ static int __init con_init(void)
> >
> > for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
> > vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), GFP_NOWAIT);
> > + if (!vc)
> > + goto fail1;
> > INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
> > tty_port_init(&vc->port);
> > visual_init(vc, currcons, 1);
> > vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT);
> > + if (!vc->vc_screenbuf)
> > + goto fail2;
> > vc_init(vc, vc->vc_rows, vc->vc_cols,
> > currcons || !vc->vc_sw->con_save_screen);
> > }
> > @@ -3375,6 +3379,16 @@ static int __init con_init(void)
> > register_console(&vt_console_driver);
> > #endif
> > return 0;
> > +fail1:
> > + while (currcons > 0) {
> > + currcons--;
> > + kfree(vc_cons[currcons].d->vc_screenbuf);
> > +fail2:
> > + kfree(vc_cons[currcons].d);
> > + vc_cons[currcons].d = NULL;
> > + }

Wait, will that even work? You can jump into the middle of a while
loop?

Ugh, that's beyond ugly. And please provide "real" names for the
labels, "fail1" and "fail2" do not tell anything here.

Also, have you actually be able to trigger this?

thanks,

greg k-h

2019-06-08 16:29:26

by Gen Zhang

[permalink] [raw]
Subject: Re: [PATCH v3] vt: Fix a missing-check bug in con_init()

On Sat, Jun 08, 2019 at 06:21:27PM +0200, Greg KH wrote:
> On Sun, Jun 09, 2019 at 12:01:38AM +0800, Gen Zhang wrote:
> > On Tue, May 28, 2019 at 08:45:29AM +0800, Gen Zhang wrote:
> > > In function con_init(), the pointer variable vc_cons[currcons].d, vc and
> > > vc->vc_screenbuf is allocated by kzalloc(). And they are used in the
> > > following codes. However, kzalloc() returns NULL when fails, and null
> > > pointer dereference may happen. And it will cause the kernel to crash.
> > > Therefore, we should check the return value and handle the error.
> > >
> > > Further, since the allcoation is in a loop, we should free all the
> > > allocated memory in a loop.
> > >
> > > Signed-off-by: Gen Zhang <[email protected]>
> > > Reviewed-by: Nicolas Pitre <[email protected]>
> > > ---
> > > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > > index fdd12f8..d50f68f 100644
> > > --- a/drivers/tty/vt/vt.c
> > > +++ b/drivers/tty/vt/vt.c
> > > @@ -3350,10 +3350,14 @@ static int __init con_init(void)
> > >
> > > for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
> > > vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), GFP_NOWAIT);
> > > + if (!vc)
> > > + goto fail1;
> > > INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
> > > tty_port_init(&vc->port);
> > > visual_init(vc, currcons, 1);
> > > vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT);
> > > + if (!vc->vc_screenbuf)
> > > + goto fail2;
> > > vc_init(vc, vc->vc_rows, vc->vc_cols,
> > > currcons || !vc->vc_sw->con_save_screen);
> > > }
> > > @@ -3375,6 +3379,16 @@ static int __init con_init(void)
> > > register_console(&vt_console_driver);
> > > #endif
> > > return 0;
> > > +fail1:
> > > + while (currcons > 0) {
> > > + currcons--;
> > > + kfree(vc_cons[currcons].d->vc_screenbuf);
> > > +fail2:
> > > + kfree(vc_cons[currcons].d);
> > > + vc_cons[currcons].d = NULL;
> > > + }
> > > + console_unlock();
> > > + return -ENOMEM;
> > > }
> > > console_initcall(con_init);
> > >
> > > ---
> > Can anyone look into this patch? It's already reviewed by Nicolas Pitre
> > <[email protected]>.
>
> It's in my queue. But note, given the previous history of your patches,
> it's really low on my piority list at the moment :(
>
> greg k-h
What? All the patches were revised iteratively according to the
maintainers' or reviewers' advice. I don't think you should look down
the patches from me. It seems not fair enough. :(

Thanks
Gen

2019-06-08 16:31:57

by Gen Zhang

[permalink] [raw]
Subject: Re: [PATCH v3] vt: Fix a missing-check bug in con_init()

On Sat, Jun 08, 2019 at 06:22:19PM +0200, Greg KH wrote:
> On Sun, Jun 09, 2019 at 12:01:38AM +0800, Gen Zhang wrote:
> > On Tue, May 28, 2019 at 08:45:29AM +0800, Gen Zhang wrote:
> > > In function con_init(), the pointer variable vc_cons[currcons].d, vc and
> > > vc->vc_screenbuf is allocated by kzalloc(). And they are used in the
> > > following codes. However, kzalloc() returns NULL when fails, and null
> > > pointer dereference may happen. And it will cause the kernel to crash.
> > > Therefore, we should check the return value and handle the error.
> > >
> > > Further, since the allcoation is in a loop, we should free all the
> > > allocated memory in a loop.
> > >
> > > Signed-off-by: Gen Zhang <[email protected]>
> > > Reviewed-by: Nicolas Pitre <[email protected]>
> > > ---
> > > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > > index fdd12f8..d50f68f 100644
> > > --- a/drivers/tty/vt/vt.c
> > > +++ b/drivers/tty/vt/vt.c
> > > @@ -3350,10 +3350,14 @@ static int __init con_init(void)
> > >
> > > for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
> > > vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), GFP_NOWAIT);
> > > + if (!vc)
> > > + goto fail1;
> > > INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
> > > tty_port_init(&vc->port);
> > > visual_init(vc, currcons, 1);
> > > vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT);
> > > + if (!vc->vc_screenbuf)
> > > + goto fail2;
> > > vc_init(vc, vc->vc_rows, vc->vc_cols,
> > > currcons || !vc->vc_sw->con_save_screen);
> > > }
> > > @@ -3375,6 +3379,16 @@ static int __init con_init(void)
> > > register_console(&vt_console_driver);
> > > #endif
> > > return 0;
> > > +fail1:
> > > + while (currcons > 0) {
> > > + currcons--;
> > > + kfree(vc_cons[currcons].d->vc_screenbuf);
> > > +fail2:
> > > + kfree(vc_cons[currcons].d);
> > > + vc_cons[currcons].d = NULL;
> > > + }
>
> Wait, will that even work? You can jump into the middle of a while
> loop?
I felt like the same when I saw reviewer's advice to write in this way.
But I tested, and it did work.
>
> Ugh, that's beyond ugly. And please provide "real" names for the
> labels, "fail1" and "fail2" do not tell anything here.

Sure, I can revise that if needed.

Thanks
Gen
>
> Also, have you actually be able to trigger this?
>
> thanks,
>
> greg k-h

2019-06-08 16:36:59

by Gen Zhang

[permalink] [raw]
Subject: Re: [PATCH v3] vt: Fix a missing-check bug in con_init()

On Sat, Jun 08, 2019 at 06:21:27PM +0200, Greg KH wrote:
> On Sun, Jun 09, 2019 at 12:01:38AM +0800, Gen Zhang wrote:
> > On Tue, May 28, 2019 at 08:45:29AM +0800, Gen Zhang wrote:
> > > In function con_init(), the pointer variable vc_cons[currcons].d, vc and
> > > vc->vc_screenbuf is allocated by kzalloc(). And they are used in the
> > > following codes. However, kzalloc() returns NULL when fails, and null
> > > pointer dereference may happen. And it will cause the kernel to crash.
> > > Therefore, we should check the return value and handle the error.
> > >
> > > Further, since the allcoation is in a loop, we should free all the
> > > allocated memory in a loop.
> > >
> > > Signed-off-by: Gen Zhang <[email protected]>
> > > Reviewed-by: Nicolas Pitre <[email protected]>
> > > ---
> > > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > > index fdd12f8..d50f68f 100644
> > > --- a/drivers/tty/vt/vt.c
> > > +++ b/drivers/tty/vt/vt.c
> > > @@ -3350,10 +3350,14 @@ static int __init con_init(void)
> > >
> > > for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
> > > vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), GFP_NOWAIT);
> > > + if (!vc)
> > > + goto fail1;
> > > INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
> > > tty_port_init(&vc->port);
> > > visual_init(vc, currcons, 1);
> > > vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT);
> > > + if (!vc->vc_screenbuf)
> > > + goto fail2;
> > > vc_init(vc, vc->vc_rows, vc->vc_cols,
> > > currcons || !vc->vc_sw->con_save_screen);
> > > }
> > > @@ -3375,6 +3379,16 @@ static int __init con_init(void)
> > > register_console(&vt_console_driver);
> > > #endif
> > > return 0;
> > > +fail1:
> > > + while (currcons > 0) {
> > > + currcons--;
> > > + kfree(vc_cons[currcons].d->vc_screenbuf);
> > > +fail2:
> > > + kfree(vc_cons[currcons].d);
> > > + vc_cons[currcons].d = NULL;
> > > + }
> > > + console_unlock();
> > > + return -ENOMEM;
> > > }
> > > console_initcall(con_init);
> > >
> > > ---
> > Can anyone look into this patch? It's already reviewed by Nicolas Pitre
> > <[email protected]>.
>
> It's in my queue. But note, given the previous history of your patches,
> it's really low on my piority list at the moment :(
>
> greg k-h
Anyway, I should be honored to be remembered by Greg K-H. :-)

Thanks
Gen

2019-06-09 00:17:31

by Nicolas Pitre

[permalink] [raw]
Subject: Re: [PATCH v3] vt: Fix a missing-check bug in con_init()

On Sat, 8 Jun 2019, Greg KH wrote:

> On Sun, Jun 09, 2019 at 12:01:38AM +0800, Gen Zhang wrote:
> > On Tue, May 28, 2019 at 08:45:29AM +0800, Gen Zhang wrote:
> > > In function con_init(), the pointer variable vc_cons[currcons].d, vc and
> > > vc->vc_screenbuf is allocated by kzalloc(). And they are used in the
> > > following codes. However, kzalloc() returns NULL when fails, and null
> > > pointer dereference may happen. And it will cause the kernel to crash.
> > > Therefore, we should check the return value and handle the error.
> > >
> > > Further, since the allcoation is in a loop, we should free all the
> > > allocated memory in a loop.
> > >
> > > Signed-off-by: Gen Zhang <[email protected]>
> > > Reviewed-by: Nicolas Pitre <[email protected]>
> > > ---
> > > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > > index fdd12f8..d50f68f 100644
> > > --- a/drivers/tty/vt/vt.c
> > > +++ b/drivers/tty/vt/vt.c
> > > @@ -3350,10 +3350,14 @@ static int __init con_init(void)
> > >
> > > for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
> > > vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), GFP_NOWAIT);
> > > + if (!vc)
> > > + goto fail1;
> > > INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
> > > tty_port_init(&vc->port);
> > > visual_init(vc, currcons, 1);
> > > vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT);
> > > + if (!vc->vc_screenbuf)
> > > + goto fail2;
> > > vc_init(vc, vc->vc_rows, vc->vc_cols,
> > > currcons || !vc->vc_sw->con_save_screen);
> > > }
> > > @@ -3375,6 +3379,16 @@ static int __init con_init(void)
> > > register_console(&vt_console_driver);
> > > #endif
> > > return 0;
> > > +fail1:
> > > + while (currcons > 0) {
> > > + currcons--;
> > > + kfree(vc_cons[currcons].d->vc_screenbuf);
> > > +fail2:
> > > + kfree(vc_cons[currcons].d);
> > > + vc_cons[currcons].d = NULL;
> > > + }
>
> Wait, will that even work? You can jump into the middle of a while
> loop?

Absolutely.

> Ugh, that's beyond ugly.

That was me who suggested to do it like that. To me, this is nicer than
the proposed alternatives. For an error path that is rather unlikely to
happen, I think this is a very concise and eleguant way to do it.

> And please provide "real" names for the
> labels, "fail1" and "fail2" do not tell anything here.

That I agree with.


Nicolas

2019-06-10 06:46:47

by Gen Zhang

[permalink] [raw]
Subject: Re: [PATCH v3] vt: Fix a missing-check bug in con_init()

On Sat, Jun 08, 2019 at 08:15:46PM -0400, Nicolas Pitre wrote:
> On Sat, 8 Jun 2019, Greg KH wrote:
>
> > On Sun, Jun 09, 2019 at 12:01:38AM +0800, Gen Zhang wrote:
> > > On Tue, May 28, 2019 at 08:45:29AM +0800, Gen Zhang wrote:
> > > > In function con_init(), the pointer variable vc_cons[currcons].d, vc and
> > > > vc->vc_screenbuf is allocated by kzalloc(). And they are used in the
> > > > following codes. However, kzalloc() returns NULL when fails, and null
> > > > pointer dereference may happen. And it will cause the kernel to crash.
> > > > Therefore, we should check the return value and handle the error.
> > > >
> > > > Further, since the allcoation is in a loop, we should free all the
> > > > allocated memory in a loop.
> > > >
> > > > Signed-off-by: Gen Zhang <[email protected]>
> > > > Reviewed-by: Nicolas Pitre <[email protected]>
> > > > ---
> > > > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > > > index fdd12f8..d50f68f 100644
> > > > --- a/drivers/tty/vt/vt.c
> > > > +++ b/drivers/tty/vt/vt.c
> > > > @@ -3350,10 +3350,14 @@ static int __init con_init(void)
> > > >
> > > > for (currcons = 0; currcons < MIN_NR_CONSOLES; currcons++) {
> > > > vc_cons[currcons].d = vc = kzalloc(sizeof(struct vc_data), GFP_NOWAIT);
> > > > + if (!vc)
> > > > + goto fail1;
> > > > INIT_WORK(&vc_cons[currcons].SAK_work, vc_SAK);
> > > > tty_port_init(&vc->port);
> > > > visual_init(vc, currcons, 1);
> > > > vc->vc_screenbuf = kzalloc(vc->vc_screenbuf_size, GFP_NOWAIT);
> > > > + if (!vc->vc_screenbuf)
> > > > + goto fail2;
> > > > vc_init(vc, vc->vc_rows, vc->vc_cols,
> > > > currcons || !vc->vc_sw->con_save_screen);
> > > > }
> > > > @@ -3375,6 +3379,16 @@ static int __init con_init(void)
> > > > register_console(&vt_console_driver);
> > > > #endif
> > > > return 0;
> > > > +fail1:
> > > > + while (currcons > 0) {
> > > > + currcons--;
> > > > + kfree(vc_cons[currcons].d->vc_screenbuf);
> > > > +fail2:
> > > > + kfree(vc_cons[currcons].d);
> > > > + vc_cons[currcons].d = NULL;
> > > > + }
> >
> > Wait, will that even work? You can jump into the middle of a while
> > loop?
>
> Absolutely.
>
> > Ugh, that's beyond ugly.
>
> That was me who suggested to do it like that. To me, this is nicer than
> the proposed alternatives. For an error path that is rather unlikely to
> happen, I think this is a very concise and eleguant way to do it.
>
> > And please provide "real" names for the
> > labels, "fail1" and "fail2" do not tell anything here.
>
> That I agree with.
>
>
> Nicolas
Thanks for your comments. Then am I supposed to revise the patch and
send a v4 version?

Thanks
Gen

2019-06-10 07:11:44

by Jiri Slaby

[permalink] [raw]
Subject: Re: [PATCH v3] vt: Fix a missing-check bug in con_init()

On 09. 06. 19, 2:15, Nicolas Pitre wrote:
>>>> +fail1:
>>>> + while (currcons > 0) {
>>>> + currcons--;
>>>> + kfree(vc_cons[currcons].d->vc_screenbuf);
>>>> +fail2:
>>>> + kfree(vc_cons[currcons].d);
>>>> + vc_cons[currcons].d = NULL;
>>>> + }
>>
>> Wait, will that even work? You can jump into the middle of a while
>> loop?
>
> Absolutely.

In C99, exceptions are only blocks with variable-sized declarations
(like "int a[b]").

thanks,
--
js
suse labs

2019-06-10 15:59:53

by Nicolas Pitre

[permalink] [raw]
Subject: Re: [PATCH v3] vt: Fix a missing-check bug in con_init()

On Sun, 9 Jun 2019, Gen Zhang wrote:

> On Sat, Jun 08, 2019 at 08:15:46PM -0400, Nicolas Pitre wrote:
> > On Sat, 8 Jun 2019, Greg KH wrote:
> >
> > > And please provide "real" names for the
> > > labels, "fail1" and "fail2" do not tell anything here.
> >
> > That I agree with.
> >
> >
> > Nicolas
> Thanks for your comments. Then am I supposed to revise the patch and
> send a v4 version?

Yes.


Nicolas