| Message ID | 1470391392-28274-1-git-send-email-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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
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 >
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
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>
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
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 --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;
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(-)