[LTP] [PATCH] mmap01: initialize buffer in check_file()

[LTP] [PATCH] mmap01: initialize buffer in check_file()

[Sven Schnelle]

mmap01 reported random test failures. Turns out the
the temporary buffer in check_file() isn't initialized.
The SAFE_READ reads less then sizeof(buf) bytes so the
rest stays initialized and might contain bytes check_file()
is looking for.

Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
---
 testcases/kernel/syscalls/mmap/mmap01.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/testcases/kernel/syscalls/mmap/mmap01.c b/testcases/kernel/syscalls/mmap/mmap01.c
index c93c37ceda52..ff09bc635c74 100644
--- a/testcases/kernel/syscalls/mmap/mmap01.c
+++ b/testcases/kernel/syscalls/mmap/mmap01.c
@@ -36,6 +36,7 @@ static void check_file(void)
 	int i, fildes, buf_len = sizeof(STRING) + 3;
 	char buf[buf_len];
 
+	memset(buf, 0, sizeof(buf));
 	fildes = SAFE_OPEN(TEMPFILE, O_RDONLY);
 	SAFE_READ(0, fildes, buf, sizeof(buf));
 
-- 
2.47.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] mmap01: initialize buffer in check_file()

[Petr Vorel]

Hi Sven,

> mmap01 reported random test failures. Turns out the
> the temporary buffer in check_file() isn't initialized.
> The SAFE_READ reads less then sizeof(buf) bytes so the
> rest stays initialized and might contain bytes check_file()
> is looking for.

Looks reasonable to me.
Reviewed-by: Petr Vorel <pvorel@suse.cz>

@Cyril a candidate for merge before the release.

Kind regards,
Petr

> Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
> ---
>  testcases/kernel/syscalls/mmap/mmap01.c | 1 +
>  1 file changed, 1 insertion(+)

> diff --git a/testcases/kernel/syscalls/mmap/mmap01.c b/testcases/kernel/syscalls/mmap/mmap01.c
> index c93c37ceda52..ff09bc635c74 100644
> --- a/testcases/kernel/syscalls/mmap/mmap01.c
> +++ b/testcases/kernel/syscalls/mmap/mmap01.c
> @@ -36,6 +36,7 @@ static void check_file(void)
>  	int i, fildes, buf_len = sizeof(STRING) + 3;
>  	char buf[buf_len];

> +	memset(buf, 0, sizeof(buf));
>  	fildes = SAFE_OPEN(TEMPFILE, O_RDONLY);
>  	SAFE_READ(0, fildes, buf, sizeof(buf));

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] mmap01: initialize buffer in check_file()

[Cyril Hrubis]

Hi!
> > mmap01 reported random test failures. Turns out the
> > the temporary buffer in check_file() isn't initialized.
> > The SAFE_READ reads less then sizeof(buf) bytes so the
> > rest stays initialized and might contain bytes check_file()
> > is looking for.
> 
> Looks reasonable to me.
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> 
> @Cyril a candidate for merge before the release.

Alternatively we could use the return value from the SAFE_READ() instead
of the buf_len. Also I suppose that we could check that we read at least
something.

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] mmap01: initialize buffer in check_file()

[Sven Schnelle]

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> > mmap01 reported random test failures. Turns out the
>> > the temporary buffer in check_file() isn't initialized.
>> > The SAFE_READ reads less then sizeof(buf) bytes so the
>> > rest stays initialized and might contain bytes check_file()
>> > is looking for.
>> 
>> Looks reasonable to me.
>> Reviewed-by: Petr Vorel <pvorel@suse.cz>
>> 
>> @Cyril a candidate for merge before the release.
>
> Alternatively we could use the return value from the SAFE_READ() instead
> of the buf_len. Also I suppose that we could check that we read at least
> something.

I wonder whether a check that strlen(STRING) bytes was read is
sufficient, and if it's more just FAIL the test? My understanding of the
test is that the data written beyond file's end isn't yet synced, so
if we can read more bytes that would already be an error?

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] mmap01: initialize buffer in check_file()

[Cyril Hrubis]

Hi!
> >> > mmap01 reported random test failures. Turns out the
> >> > the temporary buffer in check_file() isn't initialized.
> >> > The SAFE_READ reads less then sizeof(buf) bytes so the
> >> > rest stays initialized and might contain bytes check_file()
> >> > is looking for.
> >> 
> >> Looks reasonable to me.
> >> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> >> 
> >> @Cyril a candidate for merge before the release.
> >
> > Alternatively we could use the return value from the SAFE_READ() instead
> > of the buf_len. Also I suppose that we could check that we read at least
> > something.
> 
> I wonder whether a check that strlen(STRING) bytes was read is
> sufficient, and if it's more just FAIL the test? My understanding of the
> test is that the data written beyond file's end isn't yet synced, so
> if we can read more bytes that would already be an error?

I would say yes, but it does not hurt to keep the check that the file
data were not corrupted by the write after the file end. So maybe we
just need to:

1. check that we read right size
2. check that the buffer has correct bytes

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] mmap01: initialize buffer in check_file()

[Sven Schnelle]

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> >> > mmap01 reported random test failures. Turns out the
>> >> > the temporary buffer in check_file() isn't initialized.
>> >> > The SAFE_READ reads less then sizeof(buf) bytes so the
>> >> > rest stays initialized and might contain bytes check_file()
>> >> > is looking for.
>> >> 
>> >> Looks reasonable to me.
>> >> Reviewed-by: Petr Vorel <pvorel@suse.cz>
>> >> 
>> >> @Cyril a candidate for merge before the release.
>> >
>> > Alternatively we could use the return value from the SAFE_READ() instead
>> > of the buf_len. Also I suppose that we could check that we read at least
>> > something.
>> 
>> I wonder whether a check that strlen(STRING) bytes was read is
>> sufficient, and if it's more just FAIL the test? My understanding of the
>> test is that the data written beyond file's end isn't yet synced, so
>> if we can read more bytes that would already be an error?
>
> I would say yes, but it does not hurt to keep the check that the file
> data were not corrupted by the write after the file end. So maybe we
> just need to:
>
> 1. check that we read right size
> 2. check that the buffer has correct bytes

Ok. I'll change the patch and send a V2 later.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH] mmap01: initialize buffer in check_file()

[Ricardo B. Marliere via ltp]

On Wed Jan 22, 2025 at 7:09 AM -03, Sven Schnelle wrote:
> mmap01 reported random test failures. Turns out the
> the temporary buffer in check_file() isn't initialized.
> The SAFE_READ reads less then sizeof(buf) bytes so the
> rest stays initialized and might contain bytes check_file()
> is looking for.
>
> Signed-off-by: Sven Schnelle <svens@linux.ibm.com>

Reviewed-by: Ricardo B. Marliere <rbm@suse.com>

> ---
>  testcases/kernel/syscalls/mmap/mmap01.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/testcases/kernel/syscalls/mmap/mmap01.c b/testcases/kernel/syscalls/mmap/mmap01.c
> index c93c37ceda52..ff09bc635c74 100644
> --- a/testcases/kernel/syscalls/mmap/mmap01.c
> +++ b/testcases/kernel/syscalls/mmap/mmap01.c
> @@ -36,6 +36,7 @@ static void check_file(void)
>  	int i, fildes, buf_len = sizeof(STRING) + 3;
>  	char buf[buf_len];
>  
> +	memset(buf, 0, sizeof(buf));
>  	fildes = SAFE_OPEN(TEMPFILE, O_RDONLY);
>  	SAFE_READ(0, fildes, buf, sizeof(buf));
>  


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp