| Message ID | BLU436-SMTP20286881B2FE27D93086DC397A70@phx.gbl |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Feb 26, 2016 at 05:34:50PM +0800, Haojian Zhuang wrote: > Since there's percentage calcution, multiply on 32bit variable > will cause overflow. So fix the variables as 64bit. So, this is not technically what it does - it changes the variables to be native integer size, meaning this does not fix anything on AArch32. Given that the existing code already populates one of these by calling AsciiStrHexToUint64, it would seem the correct fix would be to follow the commit message and actually change them to UINT64. Regards, Leif > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> > Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org> > --- > EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c > index f380817..91de365 100644 > --- a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c > +++ b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c > @@ -45,9 +45,9 @@ typedef enum { > STATIC ANDROID_FASTBOOT_STATE mState = ExpectCmdState; > > // When in ExpectDataState, the number of bytes of data to expect: > -STATIC UINT32 mNumDataBytes; > +STATIC UINTN mNumDataBytes; > // .. and the number of bytes so far received this data phase > -STATIC UINT32 mBytesReceivedSoFar; > +STATIC UINTN mBytesReceivedSoFar; > // .. and the buffer to save data into > STATIC UINT8 *mDataBuffer = NULL; > > -- > 1.9.1 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
在 02/27/2016 04:42 AM, Leif Lindholm 写道: > On Fri, Feb 26, 2016 at 05:34:50PM +0800, Haojian Zhuang wrote: >> Since there's percentage calcution, multiply on 32bit variable >> will cause overflow. So fix the variables as 64bit. > > So, this is not technically what it does - it changes the variables to > be native integer size, meaning this does not fix anything on AArch32. > > Given that the existing code already populates one of these by calling > AsciiStrHexToUint64, it would seem the correct fix would be to follow > the commit message and actually change them to UINT64. mNumDataBytes = AsciiStrHexToUnit64 (NumBytesString); Although AsciiStrHexToUnit64 () is used, the value is still stored as 32-bit long. Since mNumDataBytes is declared as UINT32. I don't agree that it doesn't fix on AArch32. UINT64 is also supported in AArch32. For example, unsigned long long int is 64-bit long on AArch32, and we could do 64-bit calculation on AArch32. Regards Haojian
On Mon, Feb 29, 2016 at 04:36:00PM +0800, Haojian Zhuang wrote: > 在 02/27/2016 04:42 AM, Leif Lindholm 写道: > >On Fri, Feb 26, 2016 at 05:34:50PM +0800, Haojian Zhuang wrote: > >>Since there's percentage calcution, multiply on 32bit variable > >>will cause overflow. So fix the variables as 64bit. > > > >So, this is not technically what it does - it changes the variables to > >be native integer size, meaning this does not fix anything on AArch32. > > > >Given that the existing code already populates one of these by calling > >AsciiStrHexToUint64, it would seem the correct fix would be to follow > >the commit message and actually change them to UINT64. > > mNumDataBytes = AsciiStrHexToUnit64 (NumBytesString); > Although AsciiStrHexToUnit64 () is used, the value is still stored as 32-bit > long. Since mNumDataBytes is declared as UINT32. > > I don't agree that it doesn't fix on AArch32. UINT64 is also supported > in AArch32. For example, unsigned long long int is 64-bit long on AArch32, > and we could do 64-bit calculation on AArch32. Yes, UINT64 is also supported on AArch32 - but the patch turns the variables into UINTN, not UINT64. UINTN is 32-bit on AArch32. Regards, Leif
On 29 February 2016 at 17:53, Leif Lindholm <leif.lindholm@linaro.org> wrote: > On Mon, Feb 29, 2016 at 04:36:00PM +0800, Haojian Zhuang wrote: >> 在 02/27/2016 04:42 AM, Leif Lindholm 写道: >> >On Fri, Feb 26, 2016 at 05:34:50PM +0800, Haojian Zhuang wrote: >> >>Since there's percentage calcution, multiply on 32bit variable >> >>will cause overflow. So fix the variables as 64bit. >> > >> >So, this is not technically what it does - it changes the variables to >> >be native integer size, meaning this does not fix anything on AArch32. >> > >> >Given that the existing code already populates one of these by calling >> >AsciiStrHexToUint64, it would seem the correct fix would be to follow >> >the commit message and actually change them to UINT64. >> >> mNumDataBytes = AsciiStrHexToUnit64 (NumBytesString); >> Although AsciiStrHexToUnit64 () is used, the value is still stored as 32-bit >> long. Since mNumDataBytes is declared as UINT32. >> >> I don't agree that it doesn't fix on AArch32. UINT64 is also supported >> in AArch32. For example, unsigned long long int is 64-bit long on AArch32, >> and we could do 64-bit calculation on AArch32. > > Yes, UINT64 is also supported on AArch32 - but the patch turns the > variables into UINTN, not UINT64. UINTN is 32-bit on AArch32. > Oh, yes. I'll fix it with a second version. Thanks for clarify it. Regards Haojian
diff --git a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c index f380817..91de365 100644 --- a/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c +++ b/EmbeddedPkg/Application/AndroidFastboot/AndroidFastbootApp.c @@ -45,9 +45,9 @@ typedef enum { STATIC ANDROID_FASTBOOT_STATE mState = ExpectCmdState; // When in ExpectDataState, the number of bytes of data to expect: -STATIC UINT32 mNumDataBytes; +STATIC UINTN mNumDataBytes; // .. and the number of bytes so far received this data phase -STATIC UINT32 mBytesReceivedSoFar; +STATIC UINTN mBytesReceivedSoFar; // .. and the buffer to save data into STATIC UINT8 *mDataBuffer = NULL;