[U-Boot] [PATCH v2 1/2] fs: fat: dynamically allocate memory for temporary buffer

[Marek Vasut]

On 2/1/19 4:20 PM, Chee, Tien Fong wrote:
> On Fri, 2019-02-01 at 09:19 +0100, Marek Vasut wrote:
>> On 2/1/19 9:11 AM, Chee, Tien Fong wrote:
>>>
>>> On Thu, 2019-01-31 at 15:22 +0100, Marek Vasut wrote:
>>>>
>>>> On 1/31/19 1:42 PM, tien.fong.chee at intel.com wrote:
>>>>>
>>>>>
>>>>> From: Tien Fong Chee <tien.fong.chee at intel.com>
>>>>>
>>>>> Drop the statically allocated get_contents_vfatname_block and
>>>>> dynamically allocate a buffer only if required. This saves
>>>>> 64KiB of memory.
>>>>>
>>>>> Signed-off-by: Stefan Agner <stefan.ag... at toradex.com>
>>>>> Signed-off-by: Tien Fong Chee <tien.fong.chee at intel.com>
>>>>>
>>>>> ---
>>>>>
>>>>> changes for v2
>>>>> - Removed the change for debug message.
>>>>> - Set allocation based on actual required size instead of
>>>>> default
>>>>> max
>>>>>   cluster size
>>>>> ---
>>>>>  fs/fat/fat.c | 17 ++++++++++++-----
>>>>>  1 file changed, 12 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
>>>>> index ecfa255..347787e 100644
>>>>> --- a/fs/fat/fat.c
>>>>> +++ b/fs/fat/fat.c
>>>>> @@ -306,9 +306,6 @@ get_cluster(fsdata *mydata, __u32 clustnum,
>>>>> __u8 *buffer, unsigned long size)
>>>>>   * into 'buffer'.
>>>>>   * Update the number of bytes read in *gotsize or return -1 on
>>>>> fatal errors.
>>>>>   */
>>>>> -__u8 get_contents_vfatname_block[MAX_CLUSTSIZE]
>>>>> -	__aligned(ARCH_DMA_MINALIGN);
>>>>> -
>>>>>  static int get_contents(fsdata *mydata, dir_entry *dentptr,
>>>>> loff_t
>>>>> pos,
>>>>>  			__u8 *buffer, loff_t maxsize, loff_t
>>>>> *gotsize)
>>>>>  {
>>>>> @@ -351,15 +348,25 @@ static int get_contents(fsdata *mydata,
>>>>> dir_entry *dentptr, loff_t pos,
>>>>>  
>>>>>  	/* align to beginning of next cluster if any */
>>>>>  	if (pos) {
>>>>> +		__u8 *tmp_buffer;
>>>>> +
>>>>>  		actsize = min(filesize,
>>>>> (loff_t)bytesperclust);
>>>>> -		if (get_cluster(mydata, curclust,
>>>>> get_contents_vfatname_block,
>>>>> +		tmp_buffer = malloc_cache_aligned(actsize);
>>>>> +		if (!tmp_buffer) {
>>>>> +			debug("Error: allocating buffer\n");
>>>>> +			return -ENOMEM;
>>>>> +		}
>>>>> +
>>>>> +		if (get_cluster(mydata, curclust, tmp_buffer,
>>>>>  				(int)actsize) != 0) {
>>>> Is the cast of actsize needed ?
>>> Okay, i would remove it.
>>>>
>>>>
>>>>>
>>>>>
>>>>>  			printf("Error reading cluster\n");
>>>>> +			free(tmp_buffer);
>>>>>  			return -1;
>>>>>  		}
>>>>>  		filesize -= actsize;
>>>>>  		actsize -= pos;
>>>>> -		memcpy(buffer, get_contents_vfatname_block +
>>>>> pos,
>>>>> actsize);
>>>>> +		memcpy(buffer, tmp_buffer + pos, actsize);
>>>> Hmmm, tmp_buffer is actsize big , but you're memcpy-ing actsize
>>>> here,
>>>> so
>>>> this would mean you memcpy data past the end of tmp_buffer if pos
>>>>>
>>>> 0, no?
>>> This wouldn't happen because the pos and actsize are reset based on
>>> beginning of current cluster instead of beginning of a file. So,
>>> the
>>> memcpy would start at pos based on beginning of current cluster
>>> until
>>> the end of current cluster, that means the size it copies is still
>>> within a cluster size.
>> So pos is always 0 ?
> Nop, reset here means the pos offset would be adjusted accordingly when
> the base change from beginning of the file to beginning of the cluster
> where the pos resides in.
> 
> You know the driver getting the content based on cluster by cluster.

OK, reading the code again, after simplification we get:

tmp_buffer = malloc_cache_aligned(actsize);
...
actsize -= pos;
memcpy(buffer, tmp_buffer + pos, actsize);

so there shouldn't be any read past the end of buffer.

-- 
Best regards,
Marek Vasut