bad pointer operation in UiCustomizeFrontPageBanner (Bugzilla Bug 2285)

[tianocore-issues]

This issue was created automatically with bugzilla2github

Bugzilla Bug 2285

Date: 2019-10-18T05:12:17+00:00
From: Colin Ian King <<colin.king>>
To: newexplorerj
CC: guomin.jiang, nobody

Last updated: 2020-03-25T22:52:17+00:00

[tianocore-issues]

Comment 10097

Date: 2019-10-18 05:12:17 +0000
From: Colin Ian King <<colin.king>>

• Industry Specification: ---
• Target OS: ---
• Bugzilla Assignee(s): newexplorerj

In source MdeModulePkg/Application/UiApp/FrontPageCustomizedUi.c, function UiCustomizeFrontPageBanner() there is the following hunk of code:

129 if ((LineIndex == 5) && LeftOrRight) {
130 // Update STR_CUSTOMIZE_BANNER_LINE5_LEFT
131 if (PcdGetBool(PcdTestKeyUsed)) {
132 if (BannerStr != NULL) {
133 FreePool(*BannerStr);
134 }
135 *BannerStr = HiiGetString(gFrontPagePrivate.HiiHandle, STRING_TOKEN(STR_TEST_KEY_USED), NULL);
136 }
137 }

The null check on BannerStr looks incorrect. I believe it should be:

 if (*BannerStr != NULL)

If it isn't, and one really is checking that BannerStr is not NULL then this implies BannerStr could be null, in which case one will have a null pointer deference on line 135 with the assignment to *BannerStr. Either way, this code looks wrong.

[tianocore-issues]

Comment 10275

Date: 2019-11-01 02:57:54 +0000
From: nobody <>

Shenglei: please check it.

[tianocore-issues]

Comment 11324

Date: 2020-02-19 22:05:29 +0000
From: shenglei.zhang

Liming will take it.

[tianocore-issues]

Comment 11326

Date: 2020-02-19 23:13:15 +0000
From: nobody <>

please check

[tianocore-issues]

Comment 11329

Date: 2020-02-20 03:40:48 +0000
From: newexplorerj

The UiCustomizeFrontPageBanner routine will work only for line 5, the current caller is UpdateFrontPageBannerStrings routine.

When UiCustomizeFrontPageBanner is invoked, the HiiGetString routine will be invoked first, HiiGetString will use STRING_TOKEN.

In current implementation, the AutoGen.StrGather require that the STRING_TOKEN must be present, it mean that the STRING_TOKEN(STR_CUSTOMIZE_BANNER_LINE5_LEFT) will always be present, otherwise build error will happened.

So BannerStr of UiCustomizeFrontPageBanner will not be NULL and *BannerStr will not be NULL as well.

I think condition (BannerStr != NULL) of UiCustomizeFrontPageBanner will always be TRUE and FreePool(*BannerStr) will always be run. it is correct, because UiCustomizeFrontPageBanner will get new string immediately so it need free old unnecessary allocated pool.

So in my opinion, the code is ok.

[tianocore-issues]

Comment 11549

Date: 2020-03-05 00:30:36 +0000
From: newexplorerj

Update status

[tianocore-issues]

Comment 11805

Date: 2020-03-24 09:16:00 +0000
From: newexplorerj

Hi Colin,

Can you help update the status?

[tianocore-issues]

Comment 11806

Date: 2020-03-24 09:45:35 +0000
From: Colin Ian King <<colin.king>>

Maybe I didn't explain my point clearly enough.

132 if (BannerStr != NULL) {
133 FreePool(*BannerStr);
134 }

When BannerStr is not-null, the FreePool() call is referencing *BannerStr. I was supposing *BannerStr is an address, so what happens if that address is null, e.g. when:

BannerStr is not-null
*BannerStr is null
so what does FreePool(NULL) do?

Colin

[tianocore-issues]

Comment 11837

Date: 2020-03-25 11:10:54 +0000
From: guomin.jiang

Colin,

If you only focus on the function rather than context, it seem like a issue.

But when you pay attention to the context, you will see UpdateFrontPageBannerStrings() -> UiCustomizeFrontPageBanner() and the *BannerStr will always not be NULL.

You may consider any other body will use it directly, but UiCustomizeFrontPageBanner is not common API, and i think it is meaningless that just use it but no care context.

[tianocore-issues]

Comment 11838

Date: 2020-03-25 11:32:34 +0000
From: Colin Ian King <<colin.king>>

OK, lets close this issue then. Thanks for your time in investigating this.

[tianocore-issues]

Comment 11853

Date: 2020-03-25 22:52:17 +0000
From: guomin.jiang

It is specific internal routine for specific module, and the case won't happen in this case.

it may make sense that open a topic about common interface and provide a specification or guide, i think it can refer stdlib design.