[systemd-devel] [PATCH] journal: fix assert against (theoretical) undefined behavior
While all the libc implementations I know return NULL when memchr's size parameter is 0: C11 7.24.1p2: Where an argument declared as size_t n specifies the length of the array for a function, n can have the value zero on a call to that function. Unless explicitly stated otherwise in the description of a particular function in this subclause, pointer arguments on such a call shall still have valid values, as described in 7.1.4. On such a call, a function that locates a character finds no occurrence, a function that compares two character sequences returns zero, and a function that copies characters copies zero characters. see http://llvm.org/bugs/show_bug.cgi?id=18247 --- src/journal/journal-file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/journal/journal-file.c b/src/journal/journal-file.c index 4009b29..59b2829 100644 --- a/src/journal/journal-file.c +++ b/src/journal/journal-file.c @@ -956,7 +956,7 @@ static int journal_file_append_data( const void *eq; assert(f); -assert(data || size == 0); +assert(data); hash = hash64(data, size); -- 1.8.5.1 ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] journal: fix assert against (theoretical) undefined behavior
On Mon, 16.12.13 09:20, Shawn Landden (sh...@churchofgit.com) wrote: While all the libc implementations I know return NULL when memchr's size parameter is 0: C11 7.24.1p2: Where an argument declared as size_t n specifies the length of the array for a function, n can have the value zero on a call to that function. Unless explicitly stated otherwise in the description of a particular function in this subclause, pointer arguments on such a call shall still have valid values, as described in 7.1.4. On such a call, a function that locates a character finds no occurrence, a function that compares two character sequences returns zero, and a function that copies characters copies zero characters. see http://llvm.org/bugs/show_bug.cgi?id=18247 Hmm? But what does that have to do with the requirements we make on our own internal functions? --- src/journal/journal-file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/journal/journal-file.c b/src/journal/journal-file.c index 4009b29..59b2829 100644 --- a/src/journal/journal-file.c +++ b/src/journal/journal-file.c @@ -956,7 +956,7 @@ static int journal_file_append_data( const void *eq; assert(f); -assert(data || size == 0); +assert(data); hash = hash64(data, size); Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] journal: fix assert against (theoretical) undefined behavior
On Mon, Dec 16, 2013 at 10:36 AM, Lennart Poettering lenn...@poettering.net wrote: On Mon, 16.12.13 09:20, Shawn Landden (sh...@churchofgit.com) wrote: While all the libc implementations I know return NULL when memchr's size parameter is 0: C11 7.24.1p2: Where an argument declared as size_t n specifies the length of the array for a function, n can have the value zero on a call to that function. Unless explicitly stated otherwise in the description of a particular function in this subclause, pointer arguments on such a call shall still have valid values, as described in 7.1.4. On such a call, a function that locates a character finds no occurrence, a function that compares two character sequences returns zero, and a function that copies characters copies zero characters. see http://llvm.org/bugs/show_bug.cgi?id=18247 Hmm? But what does that have to do with the requirements we make on our own internal functions? Well, it makes clang's scan-build shut up :), which is how i initially came across it. --- src/journal/journal-file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/journal/journal-file.c b/src/journal/journal-file.c index 4009b29..59b2829 100644 --- a/src/journal/journal-file.c +++ b/src/journal/journal-file.c @@ -956,7 +956,7 @@ static int journal_file_append_data( const void *eq; assert(f); -assert(data || size == 0); +assert(data); hash = hash64(data, size); Lennart -- Lennart Poettering, Red Hat ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] journal: fix assert against (theoretical) undefined behavior
On Mon, Dec 16, 2013 at 7:50 PM, Shawn Landden sh...@churchofgit.com wrote: On Mon, Dec 16, 2013 at 10:36 AM, Lennart Poettering lenn...@poettering.net wrote: On Mon, 16.12.13 09:20, Shawn Landden (sh...@churchofgit.com) wrote: While all the libc implementations I know return NULL when memchr's size parameter is 0: C11 7.24.1p2: Where an argument declared as size_t n specifies the length of the array for a function, n can have the value zero on a call to that function. Unless explicitly stated otherwise in the description of a particular function in this subclause, pointer arguments on such a call shall still have valid values, as described in 7.1.4. On such a call, a function that locates a character finds no occurrence, a function that compares two character sequences returns zero, and a function that copies characters copies zero characters. see http://llvm.org/bugs/show_bug.cgi?id=18247 Hmm? But what does that have to do with the requirements we make on our own internal functions? Well, it makes clang's scan-build shut up :), which is how i initially came across it. Unfortunately there are quite a lot of false positives in scan-build. I have been cleaning it up for a while. I did come across this one earlier. Isn't the issue here that scan-build makes the wrong assumption that data can be null at that point? Generally it gets confused sometimes about functions that return a non-negative value only when setting some data succeeded. At least I think that was the conclusion I came to about the report. ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] journal: fix assert against (theoretical) undefined behavior
On Mon, Dec 16, 2013 at 11:06 AM, Thomas H.P. Andersen pho...@gmail.com wrote: On Mon, Dec 16, 2013 at 7:50 PM, Shawn Landden sh...@churchofgit.com wrote: On Mon, Dec 16, 2013 at 10:36 AM, Lennart Poettering lenn...@poettering.net wrote: On Mon, 16.12.13 09:20, Shawn Landden (sh...@churchofgit.com) wrote: While all the libc implementations I know return NULL when memchr's size parameter is 0: C11 7.24.1p2: Where an argument declared as size_t n specifies the length of the array for a function, n can have the value zero on a call to that function. Unless explicitly stated otherwise in the description of a particular function in this subclause, pointer arguments on such a call shall still have valid values, as described in 7.1.4. On such a call, a function that locates a character finds no occurrence, a function that compares two character sequences returns zero, and a function that copies characters copies zero characters. see http://llvm.org/bugs/show_bug.cgi?id=18247 Hmm? But what does that have to do with the requirements we make on our own internal functions? Well, it makes clang's scan-build shut up :), which is how i initially came across it. Unfortunately there are quite a lot of false positives in scan-build. I have been cleaning it up for a while. I did come across this one earlier. Isn't the issue here that scan-build makes the wrong assumption that data can be null at that point? Generally it gets confused sometimes about functions that return a non-negative value only when setting some data succeeded. At least I think that was the conclusion I came to about the report. While there are certainly those (thinking walking a linked-list is use-after free, _cleanup_ issues: http://llvm.org/bugs/show_bug.cgi?id=3888 ) this was a real bug that scan_build found, but which I don't totally agree is important. If cause that there wasn't assert() against the first argument to memchr() being NULL, instead there was an assert against the first argument not being NULL OR size being 0, which (I was corrected to accept) can invoke undefined behavior. ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] journal: fix assert against (theoretical) undefined behavior
On Mon, Dec 16, 2013 at 8:15 PM, Shawn Landden sh...@churchofgit.com wrote: On Mon, Dec 16, 2013 at 11:06 AM, Thomas H.P. Andersen pho...@gmail.com wrote: On Mon, Dec 16, 2013 at 7:50 PM, Shawn Landden sh...@churchofgit.com wrote: On Mon, Dec 16, 2013 at 10:36 AM, Lennart Poettering lenn...@poettering.net wrote: On Mon, 16.12.13 09:20, Shawn Landden (sh...@churchofgit.com) wrote: While all the libc implementations I know return NULL when memchr's size parameter is 0: C11 7.24.1p2: Where an argument declared as size_t n specifies the length of the array for a function, n can have the value zero on a call to that function. Unless explicitly stated otherwise in the description of a particular function in this subclause, pointer arguments on such a call shall still have valid values, as described in 7.1.4. On such a call, a function that locates a character finds no occurrence, a function that compares two character sequences returns zero, and a function that copies characters copies zero characters. see http://llvm.org/bugs/show_bug.cgi?id=18247 Hmm? But what does that have to do with the requirements we make on our own internal functions? Well, it makes clang's scan-build shut up :), which is how i initially came across it. Unfortunately there are quite a lot of false positives in scan-build. I have been cleaning it up for a while. I did come across this one earlier. Isn't the issue here that scan-build makes the wrong assumption that data can be null at that point? Generally it gets confused sometimes about functions that return a non-negative value only when setting some data succeeded. At least I think that was the conclusion I came to about the report. While there are certainly those (thinking walking a linked-list is use-after free, _cleanup_ issues: http://llvm.org/bugs/show_bug.cgi?id=3888 ) this was a real bug that scan_build found, but which I don't totally agree is important. If cause that there wasn't assert() against the first argument to memchr() being NULL, instead there was an assert against the first argument not being NULL OR size being 0, which (I was corrected to accept) can invoke undefined behavior. After rereading it I see that I was mistaken. I had thought that data would already be set at that point if size == 0. Sorry for the confusion. Would it be enough to just add: if (data == NULL size == 0) eq = false; else eq = memchr(data, '=', size); ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
Re: [systemd-devel] [PATCH] journal: fix assert against (theoretical) undefined behavior
On Mon, Dec 16, 2013 at 12:19 PM, Thomas H.P. Andersen pho...@gmail.com wrote: On Mon, Dec 16, 2013 at 8:15 PM, Shawn Landden sh...@churchofgit.com wrote: On Mon, Dec 16, 2013 at 11:06 AM, Thomas H.P. Andersen pho...@gmail.com wrote: On Mon, Dec 16, 2013 at 7:50 PM, Shawn Landden sh...@churchofgit.com wrote: On Mon, Dec 16, 2013 at 10:36 AM, Lennart Poettering lenn...@poettering.net wrote: On Mon, 16.12.13 09:20, Shawn Landden (sh...@churchofgit.com) wrote: While all the libc implementations I know return NULL when memchr's size parameter is 0: C11 7.24.1p2: Where an argument declared as size_t n specifies the length of the array for a function, n can have the value zero on a call to that function. Unless explicitly stated otherwise in the description of a particular function in this subclause, pointer arguments on such a call shall still have valid values, as described in 7.1.4. On such a call, a function that locates a character finds no occurrence, a function that compares two character sequences returns zero, and a function that copies characters copies zero characters. see http://llvm.org/bugs/show_bug.cgi?id=18247 Hmm? But what does that have to do with the requirements we make on our own internal functions? Well, it makes clang's scan-build shut up :), which is how i initially came across it. Unfortunately there are quite a lot of false positives in scan-build. I have been cleaning it up for a while. I did come across this one earlier. Isn't the issue here that scan-build makes the wrong assumption that data can be null at that point? Generally it gets confused sometimes about functions that return a non-negative value only when setting some data succeeded. At least I think that was the conclusion I came to about the report. While there are certainly those (thinking walking a linked-list is use-after free, _cleanup_ issues: http://llvm.org/bugs/show_bug.cgi?id=3888 ) this was a real bug that scan_build found, but which I don't totally agree is important. If cause that there wasn't assert() against the first argument to memchr() being NULL, instead there was an assert against the first argument not being NULL OR size being 0, which (I was corrected to accept) can invoke undefined behavior. After rereading it I see that I was mistaken. I had thought that data would already be set at that point if size == 0. Sorry for the confusion. Would it be enough to just add: if (data == NULL size == 0) || eq = false; NULL else eq = memchr(data, '=', size); or keep the existing assert() and add if (size == 0) eq = NULL; else eq = memchr(data, '=', size); ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel ___ systemd-devel mailing list systemd-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/systemd-devel
