[U-Boot,v3,2/6] fat: write: fix broken write at non-zero file offset
diff mbox series

Message ID 20191126081512.5138-3-m.szyprowski@samsung.com
State Superseded
Delegated to: Matthias Brugger
Headers show
Series
  • Raspberry Pi4: add support for DFU over USB
Related show

Commit Message

Marek Szyprowski Nov. 26, 2019, 8:15 a.m. UTC
Handling of the start file offset was broken in the current code. Although
the code skipped the needed clusters, it then tried to continue write with
current cluster set to EOF, what caused assertion. It also lacked adjusting
filesize in case of writing at the end of file and adjusting in-cluster
offset for partial overwrite.

This patch fixes all those issues.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 fs/fat/fat_write.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

Comments

Tom Rini Nov. 26, 2019, 4:57 p.m. UTC | #1
On Tue, Nov 26, 2019 at 09:15:08AM +0100, Marek Szyprowski wrote:

> Handling of the start file offset was broken in the current code. Although
> the code skipped the needed clusters, it then tried to continue write with
> current cluster set to EOF, what caused assertion. It also lacked adjusting
> filesize in case of writing at the end of file and adjusting in-cluster
> offset for partial overwrite.
> 
> This patch fixes all those issues.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  fs/fat/fat_write.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
> index 6cfa5b4565..7fb373589d 100644
> --- a/fs/fat/fat_write.c
> +++ b/fs/fat/fat_write.c
> @@ -756,14 +756,12 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
>  	/* go to cluster at pos */
>  	cur_pos = bytesperclust;
>  	while (1) {
> +		newclust = get_fatent(mydata, curclust);
>  		if (pos <= cur_pos)
>  			break;
> -		if (IS_LAST_CLUST(curclust, mydata->fatsize))
> +		if (IS_LAST_CLUST(newclust, mydata->fatsize))
>  			break;
> -
> -		newclust = get_fatent(mydata, curclust);
> -		if (!IS_LAST_CLUST(newclust, mydata->fatsize) &&
> -		    CHECK_CLUST(newclust, mydata->fatsize)) {
> +		if (CHECK_CLUST(newclust, mydata->fatsize)) {
>  			debug("curclust: 0x%x\n", curclust);
>  			debug("Invalid FAT entry\n");
>  			return -1;
> @@ -772,8 +770,8 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
>  		cur_pos += bytesperclust;
>  		curclust = newclust;
>  	}
> -	if (IS_LAST_CLUST(curclust, mydata->fatsize)) {
> -		assert(pos == cur_pos);
> +	if (pos == cur_pos && IS_LAST_CLUST(newclust, mydata->fatsize)) {
> +		filesize -= pos;
>  		goto set_clusters;
>  	}
>  
> @@ -814,6 +812,7 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
>  		else
>  			offset = pos - cur_pos;
>  		wsize = min_t(unsigned long long, actsize, filesize - cur_pos);
> +		wsize -= offset;
>  		if (get_set_cluster(mydata, curclust, offset,
>  				    buffer, wsize, &actsize)) {
>  			printf("Error get-and-setting cluster\n");

Adding in Heinrich and Akashi-san for more review on this, thanks!
AKASHI Takahiro Nov. 27, 2019, 3:13 a.m. UTC | #2
# I still need to understand the issues reported here.

On Tue, Nov 26, 2019 at 11:57:34AM -0500, Tom Rini wrote:
> On Tue, Nov 26, 2019 at 09:15:08AM +0100, Marek Szyprowski wrote:
> 
> > Handling of the start file offset was broken in the current code. Although
> > the code skipped the needed clusters, it then tried to continue write with
> > current cluster set to EOF, what caused assertion. It also lacked adjusting
> > filesize in case of writing at the end of file and adjusting in-cluster
> > offset for partial overwrite.
> > 
> > This patch fixes all those issues.

If those issues are logically independent from each other,
it would be nice to split this patch into small ones.

I would like to expect you to add more test cases, especially
against corner cases that you mentioned above, to test/py/tests/est_fs
as I did in test_ext.py.
Or at least please add more assertion checks.

> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > ---
> >  fs/fat/fat_write.c | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
> > index 6cfa5b4565..7fb373589d 100644
> > --- a/fs/fat/fat_write.c
> > +++ b/fs/fat/fat_write.c
> > @@ -756,14 +756,12 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
> >  	/* go to cluster at pos */
> >  	cur_pos = bytesperclust;
> >  	while (1) {
> > +		newclust = get_fatent(mydata, curclust);
> >  		if (pos <= cur_pos)

I think that we should change this condition as
        if (pos < cur_pos)
                break;
then modify the following code accordingly as well.

In this way, 'curclust' points to [cur_pos - bytesperclust, cur_pos)
and 'pos' is ensured to be in the middle after this 'while' unless
        (pos == cur_pos) && IS_LAST_CLUST(curclust,...).

Then the code will be expected to look better understandable.

Thanks,
-Takahiro Akashi


> >  			break;
> > -		if (IS_LAST_CLUST(curclust, mydata->fatsize))
> > +		if (IS_LAST_CLUST(newclust, mydata->fatsize))
> >  			break;
> > -
> > -		newclust = get_fatent(mydata, curclust);
> > -		if (!IS_LAST_CLUST(newclust, mydata->fatsize) &&
> > -		    CHECK_CLUST(newclust, mydata->fatsize)) {
> > +		if (CHECK_CLUST(newclust, mydata->fatsize)) {
> >  			debug("curclust: 0x%x\n", curclust);
> >  			debug("Invalid FAT entry\n");
> >  			return -1;
> > @@ -772,8 +770,8 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
> >  		cur_pos += bytesperclust;
> >  		curclust = newclust;
> >  	}
> > -	if (IS_LAST_CLUST(curclust, mydata->fatsize)) {
> > -		assert(pos == cur_pos);
> > +	if (pos == cur_pos && IS_LAST_CLUST(newclust, mydata->fatsize)) {
> > +		filesize -= pos;
> >  		goto set_clusters;
> >  	}
> >  
> > @@ -814,6 +812,7 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
> >  		else
> >  			offset = pos - cur_pos;
> >  		wsize = min_t(unsigned long long, actsize, filesize - cur_pos);
> > +		wsize -= offset;
> >  		if (get_set_cluster(mydata, curclust, offset,
> >  				    buffer, wsize, &actsize)) {
> >  			printf("Error get-and-setting cluster\n");
> 
> Adding in Heinrich and Akashi-san for more review on this, thanks!
> 
> -- 
> Tom
Marek Szyprowski Nov. 27, 2019, 1:34 p.m. UTC | #3
Hi

On 27.11.2019 04:13, AKASHI Takahiro wrote:
> # I still need to understand the issues reported here.
>
> On Tue, Nov 26, 2019 at 11:57:34AM -0500, Tom Rini wrote:
>> On Tue, Nov 26, 2019 at 09:15:08AM +0100, Marek Szyprowski wrote:
>>
>>> Handling of the start file offset was broken in the current code. Although
>>> the code skipped the needed clusters, it then tried to continue write with
>>> current cluster set to EOF, what caused assertion. It also lacked adjusting
>>> filesize in case of writing at the end of file and adjusting in-cluster
>>> offset for partial overwrite.
>>>
>>> This patch fixes all those issues.
> If those issues are logically independent from each other,
> it would be nice to split this patch into small ones.
>
> I would like to expect you to add more test cases, especially
> against corner cases that you mentioned above, to test/py/tests/est_fs
> as I did in test_ext.py.
> Or at least please add more assertion checks.

Okay, I will try to prepare some tests which show bugs fixed by this 
patch. I'm not sure I will manage to split this patch into patches 
fixing each single issue I've observed, because at least some of them 
were related.

I'm not familiar with py_test&co, but I will try to prepare some simple 
scripts for sandbox to reproduce the observed issues.

>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> ---
>>>   fs/fat/fat_write.c | 13 ++++++-------
>>>   1 file changed, 6 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
>>> index 6cfa5b4565..7fb373589d 100644
>>> --- a/fs/fat/fat_write.c
>>> +++ b/fs/fat/fat_write.c
>>> @@ -756,14 +756,12 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
>>>   	/* go to cluster at pos */
>>>   	cur_pos = bytesperclust;
>>>   	while (1) {
>>> +		newclust = get_fatent(mydata, curclust);
>>>   		if (pos <= cur_pos)
> I think that we should change this condition as
>          if (pos < cur_pos)
>                  break;
> then modify the following code accordingly as well.
>
> In this way, 'curclust' points to [cur_pos - bytesperclust, cur_pos)
> and 'pos' is ensured to be in the middle after this 'while' unless
>          (pos == cur_pos) && IS_LAST_CLUST(curclust,...).
>
> Then the code will be expected to look better understandable.
>
> Thanks,
> -Takahiro Akashi
>
>
>>>   			break;
>>> -		if (IS_LAST_CLUST(curclust, mydata->fatsize))
>>> +		if (IS_LAST_CLUST(newclust, mydata->fatsize))
>>>   			break;
>>> -
>>> -		newclust = get_fatent(mydata, curclust);
>>> -		if (!IS_LAST_CLUST(newclust, mydata->fatsize) &&
>>> -		    CHECK_CLUST(newclust, mydata->fatsize)) {
>>> +		if (CHECK_CLUST(newclust, mydata->fatsize)) {
>>>   			debug("curclust: 0x%x\n", curclust);
>>>   			debug("Invalid FAT entry\n");
>>>   			return -1;
>>> @@ -772,8 +770,8 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
>>>   		cur_pos += bytesperclust;
>>>   		curclust = newclust;
>>>   	}
>>> -	if (IS_LAST_CLUST(curclust, mydata->fatsize)) {
>>> -		assert(pos == cur_pos);
>>> +	if (pos == cur_pos && IS_LAST_CLUST(newclust, mydata->fatsize)) {
>>> +		filesize -= pos;
>>>   		goto set_clusters;
>>>   	}
>>>   
>>> @@ -814,6 +812,7 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
>>>   		else
>>>   			offset = pos - cur_pos;
>>>   		wsize = min_t(unsigned long long, actsize, filesize - cur_pos);
>>> +		wsize -= offset;
>>>   		if (get_set_cluster(mydata, curclust, offset,
>>>   				    buffer, wsize, &actsize)) {
>>>   			printf("Error get-and-setting cluster\n");
>> Adding in Heinrich and Akashi-san for more review on this, thanks!

Best regards

Patch
diff mbox series

diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index 6cfa5b4565..7fb373589d 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -756,14 +756,12 @@  set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
 	/* go to cluster at pos */
 	cur_pos = bytesperclust;
 	while (1) {
+		newclust = get_fatent(mydata, curclust);
 		if (pos <= cur_pos)
 			break;
-		if (IS_LAST_CLUST(curclust, mydata->fatsize))
+		if (IS_LAST_CLUST(newclust, mydata->fatsize))
 			break;
-
-		newclust = get_fatent(mydata, curclust);
-		if (!IS_LAST_CLUST(newclust, mydata->fatsize) &&
-		    CHECK_CLUST(newclust, mydata->fatsize)) {
+		if (CHECK_CLUST(newclust, mydata->fatsize)) {
 			debug("curclust: 0x%x\n", curclust);
 			debug("Invalid FAT entry\n");
 			return -1;
@@ -772,8 +770,8 @@  set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
 		cur_pos += bytesperclust;
 		curclust = newclust;
 	}
-	if (IS_LAST_CLUST(curclust, mydata->fatsize)) {
-		assert(pos == cur_pos);
+	if (pos == cur_pos && IS_LAST_CLUST(newclust, mydata->fatsize)) {
+		filesize -= pos;
 		goto set_clusters;
 	}
 
@@ -814,6 +812,7 @@  set_contents(fsdata *mydata, dir_entry *dentptr, loff_t pos, __u8 *buffer,
 		else
 			offset = pos - cur_pos;
 		wsize = min_t(unsigned long long, actsize, filesize - cur_pos);
+		wsize -= offset;
 		if (get_set_cluster(mydata, curclust, offset,
 				    buffer, wsize, &actsize)) {
 			printf("Error get-and-setting cluster\n");