[Qemu-devel] [PATCH] tests/hd-geo-test: Don't pass NULL to unlink()

[Qemu-devel] [PATCH] tests/hd-geo-test: Don't pass NULL to unlink()

[Peter Maydell]

The unlink() function doesn't accept a NULL pointer, so
don't pass it one. Spotted by the clang sanitizer.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/hd-geo-test.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c
index 12ee392..6176e81 100644
--- a/tests/hd-geo-test.c
+++ b/tests/hd-geo-test.c
@@ -416,7 +416,9 @@ int main(int argc, char **argv)
     ret = g_test_run();
 
     for (i = 0; i < backend_last; i++) {
-        unlink(img_file_name[i]);
+        if (img_file_name[i]) {
+            unlink(img_file_name[i]);
+        }
     }
 
     return ret;
-- 
2.7.4

Re: [Qemu-devel] [PATCH] tests/hd-geo-test: Don't pass NULL to unlink()

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> The unlink() function doesn't accept a NULL pointer, so
> don't pass it one. Spotted by the clang sanitizer.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  tests/hd-geo-test.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c
> index 12ee392..6176e81 100644
> --- a/tests/hd-geo-test.c
> +++ b/tests/hd-geo-test.c
> @@ -416,7 +416,9 @@ int main(int argc, char **argv)
>      ret = g_test_run();
>  
>      for (i = 0; i < backend_last; i++) {
> -        unlink(img_file_name[i]);
> +        if (img_file_name[i]) {
> +            unlink(img_file_name[i]);
> +        }
>      }
>  
>      return ret;

And what terrible, terrible things unlink()'s going to do when passed a
null pointer?  Turns out the same scary terrible thing it has always
done: return -1 and set errno = EFAULT.

Re: [Qemu-devel] [PATCH] tests/hd-geo-test: Don't pass NULL to unlink()

[Peter Maydell]

On 5 August 2016 at 12:19, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> The unlink() function doesn't accept a NULL pointer, so
>> don't pass it one. Spotted by the clang sanitizer.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  tests/hd-geo-test.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c
>> index 12ee392..6176e81 100644
>> --- a/tests/hd-geo-test.c
>> +++ b/tests/hd-geo-test.c
>> @@ -416,7 +416,9 @@ int main(int argc, char **argv)
>>      ret = g_test_run();
>>
>>      for (i = 0; i < backend_last; i++) {
>> -        unlink(img_file_name[i]);
>> +        if (img_file_name[i]) {
>> +            unlink(img_file_name[i]);
>> +        }
>>      }
>>
>>      return ret;
>
> And what terrible, terrible things unlink()'s going to do when passed a
> null pointer?  Turns out the same scary terrible thing it has always
> done: return -1 and set errno = EFAULT.

Feel free to send a bug report to the glibc folks saying
they shouldn't have marked it as __nonnull((1)).

thanks
-- PMM

Re: [Qemu-devel] [PATCH] tests/hd-geo-test: Don't pass NULL to unlink()

[Peter Maydell]

Ping?

thanks
-- PMM


On 5 August 2016 at 11:03, Peter Maydell <peter.maydell@linaro.org> wrote:
> The unlink() function doesn't accept a NULL pointer, so
> don't pass it one. Spotted by the clang sanitizer.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  tests/hd-geo-test.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c
> index 12ee392..6176e81 100644
> --- a/tests/hd-geo-test.c
> +++ b/tests/hd-geo-test.c
> @@ -416,7 +416,9 @@ int main(int argc, char **argv)
>      ret = g_test_run();
>
>      for (i = 0; i < backend_last; i++) {
> -        unlink(img_file_name[i]);
> +        if (img_file_name[i]) {
> +            unlink(img_file_name[i]);
> +        }
>      }
>
>      return ret;
> --
> 2.7.4

Re: [Qemu-devel] [PATCH] tests/hd-geo-test: Don't pass NULL to unlink()

[John Snow]

On 09/06/2016 08:47 AM, Peter Maydell wrote:
> Ping?
>
> thanks
> -- PMM
>

Not sure who this belongs to; I can queue it up alongside some IDE and 
FDC stuff in the future if you like.

--js

>
> On 5 August 2016 at 11:03, Peter Maydell <peter.maydell@linaro.org> wrote:
>> The unlink() function doesn't accept a NULL pointer, so
>> don't pass it one. Spotted by the clang sanitizer.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  tests/hd-geo-test.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c
>> index 12ee392..6176e81 100644
>> --- a/tests/hd-geo-test.c
>> +++ b/tests/hd-geo-test.c
>> @@ -416,7 +416,9 @@ int main(int argc, char **argv)
>>      ret = g_test_run();
>>
>>      for (i = 0; i < backend_last; i++) {
>> -        unlink(img_file_name[i]);
>> +        if (img_file_name[i]) {
>> +            unlink(img_file_name[i]);
>> +        }
>>      }
>>
>>      return ret;
>> --
>> 2.7.4
>

-- 
—js

Re: [Qemu-devel] [PATCH] tests/hd-geo-test: Don't pass NULL to unlink()

[Peter Maydell]

On 6 September 2016 at 20:06, John Snow <jsnow@redhat.com> wrote:
> On 09/06/2016 08:47 AM, Peter Maydell wrote:
>>
>> Ping?

> Not sure who this belongs to; I can queue it up alongside some IDE and FDC
> stuff in the future if you like.

I can apply it directly; I'd just like review :-)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] tests/hd-geo-test: Don't pass NULL to unlink()

[John Snow]

On 09/06/2016 03:07 PM, Peter Maydell wrote:
> On 6 September 2016 at 20:06, John Snow <jsnow@redhat.com> wrote:
>> On 09/06/2016 08:47 AM, Peter Maydell wrote:
>>>
>>> Ping?
>
>> Not sure who this belongs to; I can queue it up alongside some IDE and FDC
>> stuff in the future if you like.
>
> I can apply it directly; I'd just like review :-)
>

lol! yes, of course you can...

> thanks
> -- PMM
>

Well, I know some have religious arguments against making sanitizers 
happy, but that's not my religion.

If some systems appear to think that unlink must take a nonnull 
argument, I think that's a bug -- but making the sanitizer happy doesn't 
cost us much either IMO.

(And it's /just/ a test.)

So, personally:

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH] tests/hd-geo-test: Don't pass NULL to unlink()

[Peter Maydell]

On 6 September 2016 at 20:15, John Snow <jsnow@redhat.com> wrote:
> Well, I know some have religious arguments against making sanitizers happy,
> but that's not my religion.
>
> If some systems appear to think that unlink must take a nonnull argument, I
> think that's a bug -- but making the sanitizer happy doesn't cost us much
> either IMO.

POSIX doesn't mandate that unlink() handles NULL, so it's just
a quality-of-implementation issue in the library to handle it
without barfing (or conversely a QoI issue to over-conservatively
mark it as requires-nonnull in the system headers, if you like).

In any case my general view with all these sanitizing/lint tools
is that it's usually worth taking a few unnecessary-looking
detours in order to get to zero-warnings so you can easily
flag up when new code triggers a warning (which could well
be a bug).

> (And it's /just/ a test.)
>
> So, personally:
>
> Reviewed-by: John Snow <jsnow@redhat.com>

Thanks!

-- PMM

Re: [Qemu-devel] [PATCH] tests/hd-geo-test: Don't pass NULL to unlink()

[Peter Maydell]

On 5 August 2016 at 11:03, Peter Maydell <peter.maydell@linaro.org> wrote:
> The unlink() function doesn't accept a NULL pointer, so
> don't pass it one. Spotted by the clang sanitizer.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  tests/hd-geo-test.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c
> index 12ee392..6176e81 100644
> --- a/tests/hd-geo-test.c
> +++ b/tests/hd-geo-test.c
> @@ -416,7 +416,9 @@ int main(int argc, char **argv)
>      ret = g_test_run();
>
>      for (i = 0; i < backend_last; i++) {
> -        unlink(img_file_name[i]);
> +        if (img_file_name[i]) {
> +            unlink(img_file_name[i]);
> +        }
>      }
>
>      return ret;

Applied to master, thanks.

-- PMM