[LTP] Bug report in read_all.c

[LTP] Bug report in read_all.c

[Xiang Li]

Hi,

I would like to report a bug I found lately in LTP testcase source code.
The bug is located at: https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/fs/read_all/read_all.c#L123
This bug may cause the read_all testcase terminated unexpectedly before the reading thread complete its job.

In the source code, at the end of the function queue_pop(), it stores i + 1 into the q->front to update the front indicator.
But under some circumstances it will store 16384 which is the default length of the queue size.
When this happens, the next time queue_pop() is called, it will perform a read action that overstep the array boundary which is q->data[16384].
If the value stored there is 0, the queue_pop() will immediately return 0 and the whole testcase is broken.
This happens when there is a whole file path stores exactly at the end of the data array. In this situation, i equals 16383 when while() ends.

Modifying i + 1 to (i + 1) % QUEUE_SIZE at the source code Line#123 can easily fix it.
This bug is not triggered on every machine because the files are different between target machine.
Adjust the length of the QUEUE_SIZE will help you reproduce this bug.


Regards,
Xiang

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20191021/5f61d5d0/attachment-0001.htm>

[LTP] Bug report in read_all.c

[Richard Palethorpe]

Hello,

Xiang Li <lixian@qti.qualcomm.com> writes:

> Hi,
>
> I would like to report a bug I found lately in LTP testcase source code.
> The bug is located at: https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/fs/read_all/read_all.c#L123
> This bug may cause the read_all testcase terminated unexpectedly before the reading thread complete its job.
>
> In the source code, at the end of the function queue_pop(), it stores i + 1 into the q->front to update the front indicator.
> But under some circumstances it will store 16384 which is the default length of the queue size.
> When this happens, the next time queue_pop() is called, it will perform a read action that overstep the array boundary which is q->data[16384].
> If the value stored there is 0, the queue_pop() will immediately return 0 and the whole testcase is broken.
> This happens when there is a whole file path stores exactly at the end of the data array. In this situation, i equals 16383 when while() ends.
>
> Modifying i + 1 to (i + 1) % QUEUE_SIZE at the source code Line#123 can easily fix it.
> This bug is not triggered on every machine because the files are different between target machine.
> Adjust the length of the QUEUE_SIZE will help you reproduce this bug.

Thanks! This looks correct. Also we can replace

if (++i >= QUEUE_SIZE)
   i = 0;

with

i = (i + 1) % QUEUE_SIZE;

for consistency
	

>
>
> Regards,
> Xiang


-- 
Thank you,
Richard.

[LTP] Bug report in read_all.c

[Cyril Hrubis]

Hi!
> Modifying i + 1 to (i + 1) % QUEUE_SIZE at the source code Line#123 can easily fix it.
> This bug is not triggered on every machine because the files are different between target machine.
> Adjust the length of the QUEUE_SIZE will help you reproduce this bug.

Can you send a patch that I can apply (ideally with the suggestion from
Ritchie as well)?

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] Bug report in read_all.c

[Xiang Li]

Hi,

Thanks for Richard's suggestion. I've put it in the patch.
Please check the attached patch file for review. 

Regards,
Xiang


-----Original Message-----
From: Cyril Hrubis <chrubis@suse.cz> 
Sent: Tuesday, October 22, 2019 8:12 PM
To: Xiang Li <lixian@qti.qualcomm.com>
Cc: ltp@lists.linux.it
Subject: [EXT] Re: [LTP] Bug report in read_all.c

Hi!
> Modifying i + 1 to (i + 1) % QUEUE_SIZE at the source code Line#123 can easily fix it.
> This bug is not triggered on every machine because the files are different between target machine.
> Adjust the length of the QUEUE_SIZE will help you reproduce this bug.

Can you send a patch that I can apply (ideally with the suggestion from Ritchie as well)?

--
Cyril Hrubis
chrubis@suse.cz
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Fix-memory-read-overflow-when-a-full-file-path-store.patch
Type: application/octet-stream
Size: 1234 bytes
Desc: 0001-Fix-memory-read-overflow-when-a-full-file-path-store.patch
URL: <http://lists.linux.it/pipermail/ltp/attachments/20191023/9572d1a1/attachment.obj>

[LTP] Bug report in read_all.c

[Richard Palethorpe]

Hello,

Xiang Li <lixian@qti.qualcomm.com> writes:

> Hi,
>
> Thanks for Richard's suggestion. I've put it in the patch.
> Please check the attached patch file for review.

Thanks, LGTM.

However please do not send patches as attachments; we like to respond
with review comments inline.

>
> Regards,
> Xiang
>
>
> -----Original Message-----
> From: Cyril Hrubis <chrubis@suse.cz> 
> Sent: Tuesday, October 22, 2019 8:12 PM
> To: Xiang Li <lixian@qti.qualcomm.com>
> Cc: ltp@lists.linux.it
> Subject: [EXT] Re: [LTP] Bug report in read_all.c
>
> Hi!
>> Modifying i + 1 to (i + 1) % QUEUE_SIZE at the source code Line#123 can easily fix it.
>> This bug is not triggered on every machine because the files are different between target machine.
>> Adjust the length of the QUEUE_SIZE will help you reproduce this bug.
>
> Can you send a patch that I can apply (ideally with the suggestion from Ritchie as well)?


-- 
Thank you,
Richard.

[LTP] Bug report in read_all.c

[Cyril Hrubis]

Hi!
> Thanks for Richard's suggestion. I've put it in the patch.
> Please check the attached patch file for review. 

Pushed.

I've adjusted the commit message and got rid of the trailing whitespaces
in the patch as well.

Also as Ritchie said, patches are usually send inline and not as
attached, and this could be done with git send-email.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] Bug report in read_all.c

[Xiang Li]

Okay, I'll keep that in mind.
Thanks for your reply.

Regards,
Xiang

-----Original Message-----
From: Cyril Hrubis <chrubis@suse.cz> 
Sent: Wednesday, October 23, 2019 6:06 PM
To: Xiang Li <lixian@qti.qualcomm.com>
Cc: ltp@lists.linux.it; Richard Palethorpe <rpalethorpe@suse.de>
Subject: [EXT] Re: [LTP] Bug report in read_all.c

Hi!
> Thanks for Richard's suggestion. I've put it in the patch.
> Please check the attached patch file for review. 

Pushed.

I've adjusted the commit message and got rid of the trailing whitespaces in the patch as well.

Also as Ritchie said, patches are usually send inline and not as attached, and this could be done with git send-email.

--
Cyril Hrubis
chrubis@suse.cz