diff mbox

tests/hd-geo-test: Don't pass NULL to unlink()

Message ID 1470391392-28274-1-git-send-email-peter.maydell@linaro.org
State Accepted
Commit 0813cbf913b04c856bffa87aa005423082325ac1
Headers show

Commit Message

Peter Maydell Aug. 5, 2016, 10:03 a.m. UTC
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(-)

-- 
2.7.4

Comments

Peter Maydell Aug. 5, 2016, 11:57 a.m. UTC | #1
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
Peter Maydell Sept. 6, 2016, 12:47 p.m. UTC | #2
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
Peter Maydell Sept. 6, 2016, 7:07 p.m. UTC | #3
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
Peter Maydell Sept. 6, 2016, 7:52 p.m. UTC | #4
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
Peter Maydell Sept. 8, 2016, 10:27 a.m. UTC | #5
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
diff mbox

Patch

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;