[LTP] [PATCH] lib/tst_tmpdir: tst_get_tmpdir() add error handing

[LTP] [PATCH] lib/tst_tmpdir: tst_get_tmpdir() add error handing

[zhanglianjie]

Signed-off-by: zhanglianjie <zhanglianjie@uniontech.com>

diff --git a/lib/tst_tmpdir.c b/lib/tst_tmpdir.c
index 0c39eb89f..f006e4893 100644
--- a/lib/tst_tmpdir.c
+++ b/lib/tst_tmpdir.c
@@ -108,12 +108,18 @@ int tst_tmpdir_created(void)

 char *tst_get_tmpdir(void)
 {
+	char *ret = NULL;
+
 	if (TESTDIR == NULL) {
 		tst_brkm(TBROK, NULL, "you must call tst_tmpdir() first");
 		return NULL;
 	}

-	return strdup(TESTDIR);
+	ret = strdup(TESTDIR);
+	if (!ret)
+		tst_brkm(TBROK, NULL, "strdup() failed");
+
+	return ret;
 }

 const char *tst_get_startwd(void)
--
2.20.1

[LTP] [PATCH] lib/tst_tmpdir: tst_get_tmpdir() add error handing

[Joerg Vehlow]

Hi

On 8/6/2021 5:21 AM, zhanglianjie wrote:
> Signed-off-by: zhanglianjie <zhanglianjie@uniontech.com>
>
> diff --git a/lib/tst_tmpdir.c b/lib/tst_tmpdir.c
> index 0c39eb89f..f006e4893 100644
> --- a/lib/tst_tmpdir.c
> +++ b/lib/tst_tmpdir.c
> @@ -108,12 +108,18 @@ int tst_tmpdir_created(void)
>
>   char *tst_get_tmpdir(void)
>   {
> +	char *ret = NULL;
> +
>   	if (TESTDIR == NULL) {
>   		tst_brkm(TBROK, NULL, "you must call tst_tmpdir() first");
>   		return NULL;
>   	}
>
> -	return strdup(TESTDIR);
> +	ret = strdup(TESTDIR);
Is a failing strdup here really a thing? The only reason strdup should 
be able to fail is with ENOMEM.
The only way tst_brkm will work, if strdup fails here is, if TESTDIR is 
an extremely huge string (the NULL case is already handled above).
> +	if (!ret)
> +		tst_brkm(TBROK, NULL, "strdup() failed");
> +
> +	return ret;
>   }
>
>   const char *tst_get_startwd(void)
> --
> 2.20.1
>
>
Joerg

[LTP] [PATCH] lib/tst_tmpdir: tst_get_tmpdir() add error handing

[zhanglianjie]

Hi,
> On 8/6/2021 5:21 AM, zhanglianjie wrote:
>> Signed-off-by: zhanglianjie <zhanglianjie@uniontech.com>
>>
>> diff --git a/lib/tst_tmpdir.c b/lib/tst_tmpdir.c
>> index 0c39eb89f..f006e4893 100644
>> --- a/lib/tst_tmpdir.c
>> +++ b/lib/tst_tmpdir.c
>> @@ -108,12 +108,18 @@ int tst_tmpdir_created(void)
>>
>> ? char *tst_get_tmpdir(void)
>> ? {
>> +??? char *ret = NULL;
>> +
>> ????? if (TESTDIR == NULL) {
>> ????????? tst_brkm(TBROK, NULL, "you must call tst_tmpdir() first");
>> ????????? return NULL;
>> ????? }
>>
>> -??? return strdup(TESTDIR);
>> +??? ret = strdup(TESTDIR);
> Is a failing strdup here really a thing? The only reason strdup should 
> be able to fail is with ENOMEM.
> The only way tst_brkm will work, if strdup fails here is, if TESTDIR is 
> an extremely huge string (the NULL case is already handled above).
If don?t consider a very huge string, you don?t need to modify it here. 
This is a habitual judgment.
Thank you for your review.
>> +??? if (!ret)
>> +??????? tst_brkm(TBROK, NULL, "strdup() failed");
>> +
>> +??? return ret;
>> ? }
>>
>> ? const char *tst_get_startwd(void)
>> -- 
>> 2.20.1
>>
>>
> Joerg
> 

-- 
Regards,
Zhang Lianjie

[LTP] [PATCH] lib/tst_tmpdir: tst_get_tmpdir() add error handing

[Cyril Hrubis]

Hi!
> > -	return strdup(TESTDIR);
> > +	ret = strdup(TESTDIR);
> Is a failing strdup here really a thing? The only reason strdup should 
> be able to fail is with ENOMEM.
> The only way tst_brkm will work, if strdup fails here is, if TESTDIR is 
> an extremely huge string (the NULL case is already handled above).

It's unlikely, but it may happen if:

* Someone runs memeater on the baground along with LTP
* And kernel is set not to overcommit

And in a case of the test lirary I would rather see it written so that
we check everything, even the unlikely errors.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] lib/tst_tmpdir: tst_get_tmpdir() add error handing

[Cyril Hrubis]

Hi!
Pushed, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz