From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C152181F6C for ; Thu, 26 Jan 2017 09:05:28 -0800 (PST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga101.fm.intel.com with ESMTP; 26 Jan 2017 09:05:28 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,290,1477983600"; d="scan'208";a="813654740" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by FMSMGA003.fm.intel.com with ESMTP; 26 Jan 2017 09:05:28 -0800 Received: from fmsmsx114.amr.corp.intel.com (10.18.116.8) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 26 Jan 2017 09:05:28 -0800 Received: from fmsmsx103.amr.corp.intel.com ([169.254.2.47]) by FMSMSX114.amr.corp.intel.com ([10.18.116.8]) with mapi id 14.03.0248.002; Thu, 26 Jan 2017 09:05:27 -0800 From: "Carsey, Jaben" To: "Witt, Sebastian" , "edk2-devel@lists.01.org" , "Ni, Ruiyu" CC: "Carsey, Jaben" Thread-Topic: [PATCH] Fix edit on screens with more than 200 columns Thread-Index: AdJ2ODiu1mdD8bRPQUiM1SL7j7EOggACzgCQAAbPCjAAAIdv4AAB3u+wAFJSmmAAES2P8A== Date: Thu, 26 Jan 2017 17:05:26 +0000 Message-ID: References: <5964EF557D87964BB107B86316EE26D21E0F4BB8@DEFTHW99EK3MSX.ww902.siemens.net> <5964EF557D87964BB107B86316EE26D21E0F4EA0@DEFTHW99EK3MSX.ww902.siemens.net> <5964EF557D87964BB107B86316EE26D21E0F7B8D@DEFTHW99EK3MSX.ww902.siemens.net> In-Reply-To: <5964EF557D87964BB107B86316EE26D21E0F7B8D@DEFTHW99EK3MSX.ww902.siemens.net> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNDM0OTFjOGUtY2Y3Yy00ZjBhLWFmZTctOTg3ZGVlZmJlZDgzIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6InVoanl5dGVqQm5pQlZyQVwveVYyZUF0RWdKKzVpKzF0aXRmUmxqalNvWXZVPSJ9 x-ctpclassification: CTP_IC x-originating-ip: [10.1.200.107] MIME-Version: 1.0 Subject: Re: [PATCH] Fix edit on screens with more than 200 columns X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 26 Jan 2017 17:05:28 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable That seems good to me. Reviewed-by: Jaben Carsey Ray, what do you think? -Jaben > -----Original Message----- > From: Witt, Sebastian [mailto:sebastian.witt@siemens.com] > Sent: Thursday, January 26, 2017 1:01 AM > To: Carsey, Jaben ; edk2-devel@lists.01.org > Subject: RE: [PATCH] Fix edit on screens with more than 200 columns > Importance: High >=20 > Like this? >=20 > If the shell edit command is used on a screen with more than > 200 columns, we get a buffer overflow. This always allocates a pool for t= he > columns. >=20 > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Sebastian Witt >=20 > --- > .../UefiShellDebug1CommandsLib.c | 15 +++++++++= +++++- > 1 file changed, 9 insertions(+), 1 deletion(-) >=20 > diff --git > a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman > dsLib.c > b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman > dsLib.c > index 6ebf002..d81dd01 100644 > --- > a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman > dsLib.c > +++ > b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman > dsLib.c > @@ -302,12 +302,21 @@ EditorClearLine ( > IN UINTN LastRow > ) > { > - CHAR16 Line[200]; > + CHAR16 *Line; >=20 > if (Row =3D=3D 0) { > Row =3D 1; > } >=20 > + // Allocate buffer for columns > + Line =3D AllocateZeroPool (LastCol*(sizeof(CHAR16) + 1)); > + if (Line =3D=3D NULL) { > + return; > + } > + > // > // prepare a blank line > // > @@ -326,6 +335,10 @@ EditorClearLine ( > // print out the blank line > // > ShellPrintEx (0, ((INT32)Row) - 1, Line); > + > + FreePool (Line); > } >=20 > /** > -- > 2.1.4 >=20 >=20 > -----Original Message----- > From: Carsey, Jaben [mailto:jaben.carsey@intel.com] > Sent: Dienstag, 24. Januar 2017 18:36 > To: Witt, Sebastian (DF FA AS DH KHE 1); edk2-devel@lists.01.org > Cc: Carsey, Jaben > Subject: RE: [PATCH] Fix edit on screens with more than 200 columns >=20 > I didn't mean a 400 static, I meant start by allocating 400 and simplify = the end > of the function... >=20 > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > > Witt, Sebastian > > Sent: Tuesday, January 24, 2017 8:48 AM > > To: Carsey, Jaben ; edk2-devel@lists.01.org > > Subject: Re: [edk2] [PATCH] Fix edit on screens with more than 200 > > columns > > Importance: High > > > > Only performance. But I haven't measured if there is a big difference > > between static buffer and AllocateZeroPool. I wouldn't use a fixed valu= e. > > There may be a display device with more than 400 columns. Otherwise > > one can always allocate the [LastCol + 1] buffer. > > > > -----Original Message----- > > From: Carsey, Jaben [mailto:jaben.carsey@intel.com] > > Sent: Dienstag, 24. Januar 2017 17:27 > > To: Witt, Sebastian (DF FA AS DH KHE 1); edk2-devel@lists.01.org > > Cc: Carsey, Jaben > > Subject: RE: [PATCH] Fix edit on screens with more than 200 columns > > > > Is there a reason to not just always start with allocating the 400 and > > then we don't need to complicate the end to conditionally free the buff= er? > > > > > -----Original Message----- > > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf > > > Of Witt, Sebastian > > > Sent: Tuesday, January 24, 2017 5:14 AM > > > To: edk2-devel@lists.01.org > > > Subject: [edk2] [PATCH] Fix edit on screens with more than 200 > > > columns > > > Importance: High > > > > > > If the shell edit command is used on a screen with more than > > > 200 columns, we get a buffer overflow. This increases the default > > > buffer size to > > > 400 columns and allocates a pool when this is not enough. > > > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > > Signed-off-by: Sebastian Witt > > > > > > --- > > > .../UefiShellDebug1CommandsLib.c | 15 +++++= +++++++++- > > > 1 file changed, 14 insertions(+), 1 deletion(-) > > > > > > diff --git > > > > > > a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman > > dsL > > > i > > > b.c > > > > > > b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman > > dsL > > > i > > > b.c > > > index 6ebf002..d81dd01 100644 > > > --- > > > > > > a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman > > dsL > > > i > > > b.c > > > +++ > > > > > > b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman > > d > > > +++ sLib.c > > > @@ -302,12 +302,21 @@ EditorClearLine ( > > > IN UINTN LastRow > > > ) > > > { > > > - CHAR16 Line[200]; > > > + CHAR16 Buffer[400]; > > > + CHAR16 *Line =3D Buffer; > > > > > > if (Row =3D=3D 0) { > > > Row =3D 1; > > > } > > > > > > + // If there are more columns than our buffer can contain, > > > + allocate new buffer if (LastCol >=3D (sizeof (Buffer) / sizeof (CH= AR16))) { > > > + Line =3D AllocateZeroPool (LastCol*(sizeof(CHAR16) + 1)); > > > + if (Line =3D=3D NULL) { > > > + return; > > > + } > > > + } > > > + > > > // > > > // prepare a blank line > > > // > > > @@ -326,6 +335,10 @@ EditorClearLine ( > > > // print out the blank line > > > // > > > ShellPrintEx (0, ((INT32)Row) - 1, Line); > > > + > > > + // Free if allocated > > > + if (Line !=3D Buffer) > > > + FreePool (Line); > > > } > > > > > > /** > > > -- > > > 2.1.4 > > > > > > With best regards, > > > Sebastian Witt > > > > > > Siemens AG > > > Digital Factory Division > > > Factory Automation > > > Automation Products and Systems > > > DF FA AS DH KHE 1 > > > Oestliche Rheinbrueckenstr. 50 > > > 76187 Karlsruhe, Germany > > > Tel.: +49 721 595-5326 > > > mailto:sebastian.witt@siemens.com > > > > > > www.siemens.com/ingenuityforlife > > > > > > Siemens Aktiengesellschaft: Chairman of the Supervisory Board: > > > Gerhard Cromme; Managing Board: Joe Kaeser, Chairman, President and > > > Chief Executive Officer; Roland Busch, Lisa Davis, Klaus Helmrich, > > > Janina Kugel, Siegfried Russwurm, Ralf P. Thomas; Registered > > > offices: Berlin and Munich, Germany; Commercial registries: Berlin > > > Charlottenburg, HRB 12300, Munich, HRB 6684; WEEE-Reg.-No. DE > > > 23691322 _______________________________________________ > > > edk2-devel mailing list > > > edk2-devel@lists.01.org > > > https://lists.01.org/mailman/listinfo/edk2-devel > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel