lkml.org 
[lkml]   [2022]   [Nov]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] staging: r8188eu: Use flexible-array for one length array member
On Fri, Oct 28, 2022 at 05:43:03PM +0300, Dan Carpenter wrote:
> On Fri, Oct 28, 2022 at 07:41:54PM +0530, Deepak R Varma wrote:
> > On Fri, Oct 28, 2022 at 05:03:25PM +0300, Pavel Skripkin wrote:
> > > Hi Deepak R,
> > >
> > > Deepak R Varma <drv@mailo.com> says:
> > > > Flexible-array member should be used instead of one or zero member to
> > > > meet the need for having a dynamically sized trailing elements in a
> > > > structure. Refer to links [1] and [2] for detailed guidance on this
> > > > suggestion.
> > > >
> > > > [1] https://en.wikipedia.org/wiki/Flexible_array_member
> > > > [2] https://www.kernel.org/doc/html/v5.16/process/deprecated.html#zero-length-and-one-element-arrays
> > > >
> > > > Issue identified using coccicheck.
> > > >
> > > > Signed-off-by: Deepak R Varma <drv@mailo.com>
> > > > ---
> > > > drivers/staging/r8188eu/include/odm.h | 2 +-
> > > > drivers/staging/r8188eu/include/wlan_bssdef.h | 6 +++---
> > > > 2 files changed, 4 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/r8188eu/include/odm.h b/drivers/staging/r8188eu/include/odm.h
> > > > index 89b01dd614ba..e2a9de5b9323 100644
> > > > --- a/drivers/staging/r8188eu/include/odm.h
> > > > +++ b/drivers/staging/r8188eu/include/odm.h
> > > > @@ -166,7 +166,7 @@ struct odm_ra_info {
> > > >
> > > > struct ijk_matrix_regs_set {
> > > > bool bIQKDone;
> > > > - s32 Value[1][IQK_Matrix_REG_NUM];
> > > > + s32 Value[][IQK_Matrix_REG_NUM];
> > > > };
> > > >
> > >
> > > you are changing the actual size of the struct. Wondering if you have tested
> > > this patch somehow
> >
> > Hello Pavel,
> > Thank you for reviewing the patch. I build the module post making the changes an
> > ensured that the build is successful. However, I am not sure how to check the
> > changes I am proposing. Could you please direct me to some information on how to
> > test patches? Is there some documentation generic/driver-specific that I can
> > refer to?
>
> You just have to look at every place where it is used and especially
> look at where it is allocated. It is only used in one place:
>
> struct ijk_matrix_regs_set IQKMatrixRegSetting;
>
> But that is in the middle of a struct and generally it doesn't make
> sense to have a flex array in the middle of a struct. So investigating
> further, we see that it really is a one element array:
>
> Do a grep:
>
> pDM_Odm->RFCalibrateInfo.IQKMatrixRegSetting[ChannelMappedIndex].Value[0][1]);
>
> The first element is always zero. So this patch introduces memory
> corruption.
>
> The code is messy and should be cleaned up, of course.

Hello Pavel and Dan,
I looked at the surround code and places where the proposed patch will have an
impact. As you rightly pointed out this array can't be treated as a flexible
array since it fits right in the middle of another struct.

I am wondering if ijk_matrix_regs_set (or IQKMatrixRegSetting) is a one element
array, why is it declared as a two dimensional array? Can this be a simple one
dimensional array instead? Is this the cleanup you are referring to when you say
"code is messy and should be cleaned up"?


Thank you,
./drv

>
> regards,
> dan carpenter
>


\
 
 \ /
  Last update: 2022-11-02 20:20    [W:0.126 / U:0.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site