| Message ID | 1418679306-39971-1-git-send-email-mike.holmes@linaro.org |
|---|---|
| State | Changes Requested |
| Headers | show |
On 15 December 2014 at 22:35, Mike Holmes <mike.holmes@linaro.org> wrote: > Termination of the string is not assured unless the string is > one char shorter than the buffer. > > Fixes coverity 83058 > > Signed-off-by: Mike Holmes <mike.holmes@linaro.org> > --- > platform/linux-generic/odp_packet_io.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c > index a03eeb1..7694236 100644 > --- a/platform/linux-generic/odp_packet_io.c > +++ b/platform/linux-generic/odp_packet_io.c > @@ -247,7 +247,7 @@ odp_pktio_t odp_pktio_open(const char *dev, odp_buffer_pool_t pool) > return ODP_PKTIO_INVALID; > > done: > - strncpy(pktio_entry->s.name, dev, IFNAMSIZ); > + strncpy(pktio_entry->s.name, dev, IFNAMSIZ - 1); strncpy() does not necessarily null-terminated the copied string (only if there is a null-char within the copied range of characters). So where is pktio_entry->s.name[IFNAMSIZ -1] cleared? We should be using something like strlcpy() instead. > unlock_entry_classifier(pktio_entry); > return id; > } > -- > 2.1.0 > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
On 15 December 2014 at 13:42, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > On 15 December 2014 at 22:35, Mike Holmes <mike.holmes@linaro.org> wrote: >> Termination of the string is not assured unless the string is >> one char shorter than the buffer. >> >> Fixes coverity 83058 >> >> Signed-off-by: Mike Holmes <mike.holmes@linaro.org> >> --- >> platform/linux-generic/odp_packet_io.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c >> index a03eeb1..7694236 100644 >> --- a/platform/linux-generic/odp_packet_io.c >> +++ b/platform/linux-generic/odp_packet_io.c >> @@ -247,7 +247,7 @@ odp_pktio_t odp_pktio_open(const char *dev, odp_buffer_pool_t pool) >> return ODP_PKTIO_INVALID; >> >> done: >> - strncpy(pktio_entry->s.name, dev, IFNAMSIZ);pktio_entry->s.name >> + strncpy(pktio_entry->s.name, dev, IFNAMSIZ - 1); > strncpy() does not necessarily null-terminated the copied string (only > if there is a null-char within the copied range of characters). So > where is pktio_entry->s.name[IFNAMSIZ -1] cleared? > > We should be using something like strlcpy() instead. It should be consistent across the whole project. In other places were strncpy is used, it is followed by code that adds 0 to last byte of target string. In this patch the following line should be added after strncpy. pktio_entry->s.name[IFNAMSIZ - 1] = 0; Also note strlcpy is BSD function. I don't see such in Linux. Thanks, Victor >> unlock_entry_classifier(pktio_entry); >> return id; >> } >> -- >> 2.1.0 >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
From the documentation for strlcpy():
*Also note that strlcpy() and strlcat() only operate on true
``C''strings.This means that for **strlcpy() src must be
NUL-terminated and for strlcat() both src and dst must be
NUL-terminated.*
We use strncpy() elsewhere, but always with an explicit null
termination added. See for example in odp_buffer_pool_create():
if (name == NULL) {
pool->s.name[0] = 0;
} else {
strncpy(pool->s.name, name,
ODP_BUFFER_POOL_NAME_LEN - 1);
pool->s.name[ODP_BUFFER_POOL_NAME_LEN - 1] = 0;
pool->s.flags.has_name = 1;
}
With the explicit null termination added this looks safer.
Bill
On Mon, Dec 15, 2014 at 3:42 PM, Ola Liljedahl <ola.liljedahl@linaro.org>
wrote:
>
> On 15 December 2014 at 22:35, Mike Holmes <mike.holmes@linaro.org> wrote:
> > Termination of the string is not assured unless the string is
> > one char shorter than the buffer.
> >
> > Fixes coverity 83058
> >
> > Signed-off-by: Mike Holmes <mike.holmes@linaro.org>
> > ---
> > platform/linux-generic/odp_packet_io.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/platform/linux-generic/odp_packet_io.c
> b/platform/linux-generic/odp_packet_io.c
> > index a03eeb1..7694236 100644
> > --- a/platform/linux-generic/odp_packet_io.c
> > +++ b/platform/linux-generic/odp_packet_io.c
> > @@ -247,7 +247,7 @@ odp_pktio_t odp_pktio_open(const char *dev,
> odp_buffer_pool_t pool)
> > return ODP_PKTIO_INVALID;
> >
> > done:
> > - strncpy(pktio_entry->s.name, dev, IFNAMSIZ);
> > + strncpy(pktio_entry->s.name, dev, IFNAMSIZ - 1);
> strncpy() does not necessarily null-terminated the copied string (only
> if there is a null-char within the copied range of characters). So
> where is pktio_entry->s.name[IFNAMSIZ -1] cleared?
>
> We should be using something like strlcpy() instead.
>
> > unlock_entry_classifier(pktio_entry);
> > return id;
> > }
> > --
> > 2.1.0
> >
> >
> > _______________________________________________
> > lng-odp mailing list
> > lng-odp@lists.linaro.org
> > http://lists.linaro.org/mailman/listinfo/lng-odp
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
On 15 December 2014 at 16:53, Victor Kamensky <victor.kamensky@linaro.org> wrote: > > On 15 December 2014 at 13:42, Ola Liljedahl <ola.liljedahl@linaro.org> > wrote: > > On 15 December 2014 at 22:35, Mike Holmes <mike.holmes@linaro.org> > wrote: > >> Termination of the string is not assured unless the string is > >> one char shorter than the buffer. > >> > >> Fixes coverity 83058 > >> > >> Signed-off-by: Mike Holmes <mike.holmes@linaro.org> > >> --- > >> platform/linux-generic/odp_packet_io.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/platform/linux-generic/odp_packet_io.c > b/platform/linux-generic/odp_packet_io.c > >> index a03eeb1..7694236 100644 > >> --- a/platform/linux-generic/odp_packet_io.c > >> +++ b/platform/linux-generic/odp_packet_io.c > >> @@ -247,7 +247,7 @@ odp_pktio_t odp_pktio_open(const char *dev, > odp_buffer_pool_t pool) > >> return ODP_PKTIO_INVALID; > >> > >> done: > >> - strncpy(pktio_entry->s.name, dev, IFNAMSIZ);pktio_entry->s.name > >> + strncpy(pktio_entry->s.name, dev, IFNAMSIZ - 1); > > strncpy() does not necessarily null-terminated the copied string (only > > if there is a null-char within the copied range of characters). So > > where is pktio_entry->s.name[IFNAMSIZ -1] cleared? > > > > We should be using something like strlcpy() instead. > > It should be consistent across the whole project. In other > places were strncpy is used, it is followed by code that > adds 0 to last byte of target string. In this patch the following > line should be added after strncpy. > > pktio_entry->s.name[IFNAMSIZ - 1] = 0; > Agree, failed to look for that - will fix > > Also note strlcpy is BSD function. I don't see such > in Linux. > > Thanks, > Victor > > >> unlock_entry_classifier(pktio_entry); > >> return id; > >> } > >> -- > >> 2.1.0 > >> > >> > >> _______________________________________________ > >> lng-odp mailing list > >> lng-odp@lists.linaro.org > >> http://lists.linaro.org/mailman/listinfo/lng-odp > > > > _______________________________________________ > > lng-odp mailing list > > lng-odp@lists.linaro.org > > http://lists.linaro.org/mailman/listinfo/lng-odp >
On 15 December 2014 at 22:53, Victor Kamensky <victor.kamensky@linaro.org> wrote: > On 15 December 2014 at 13:42, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: >> On 15 December 2014 at 22:35, Mike Holmes <mike.holmes@linaro.org> wrote: >>> Termination of the string is not assured unless the string is >>> one char shorter than the buffer. >>> >>> Fixes coverity 83058 >>> >>> Signed-off-by: Mike Holmes <mike.holmes@linaro.org> >>> --- >>> platform/linux-generic/odp_packet_io.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c >>> index a03eeb1..7694236 100644 >>> --- a/platform/linux-generic/odp_packet_io.c >>> +++ b/platform/linux-generic/odp_packet_io.c >>> @@ -247,7 +247,7 @@ odp_pktio_t odp_pktio_open(const char *dev, odp_buffer_pool_t pool) >>> return ODP_PKTIO_INVALID; >>> >>> done: >>> - strncpy(pktio_entry->s.name, dev, IFNAMSIZ);pktio_entry->s.name >>> + strncpy(pktio_entry->s.name, dev, IFNAMSIZ - 1); >> strncpy() does not necessarily null-terminated the copied string (only >> if there is a null-char within the copied range of characters). So >> where is pktio_entry->s.name[IFNAMSIZ -1] cleared? >> >> We should be using something like strlcpy() instead. > > It should be consistent across the whole project. In other > places were strncpy is used, it is followed by code that > adds 0 to last byte of target string. In this patch the following > line should be added after strncpy. > > pktio_entry->s.name[IFNAMSIZ - 1] = 0; > > Also note strlcpy is BSD function. I don't see such > in Linux. We could write our own. Continuing using strncpy will just enable this type of bug to reappear. > > Thanks, > Victor > >>> unlock_entry_classifier(pktio_entry); >>> return id; >>> } >>> -- >>> 2.1.0 >>> >>> >>> _______________________________________________ >>> lng-odp mailing list >>> lng-odp@lists.linaro.org >>> http://lists.linaro.org/mailman/listinfo/lng-odp >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp
On 12/16/2014 12:35 AM, Mike Holmes wrote: > Termination of the string is not assured unless the string is > one char shorter than the buffer. > > Fixes coverity 83058 > > Signed-off-by: Mike Holmes <mike.holmes@linaro.org> > --- > platform/linux-generic/odp_packet_io.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c > index a03eeb1..7694236 100644 > --- a/platform/linux-generic/odp_packet_io.c > +++ b/platform/linux-generic/odp_packet_io.c > @@ -247,7 +247,7 @@ odp_pktio_t odp_pktio_open(const char *dev, odp_buffer_pool_t pool) > return ODP_PKTIO_INVALID; > > done: > - strncpy(pktio_entry->s.name, dev, IFNAMSIZ); > + strncpy(pktio_entry->s.name, dev, IFNAMSIZ - 1); > unlock_entry_classifier(pktio_entry); > return id; > } This bug will never happen due to check for length of dev at the top of function. But fix will be ok to turn off coverity warning. Maxim.
On 15 December 2014 at 23:20, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 12/16/2014 12:35 AM, Mike Holmes wrote: >> >> Termination of the string is not assured unless the string is >> one char shorter than the buffer. >> >> Fixes coverity 83058 >> >> Signed-off-by: Mike Holmes <mike.holmes@linaro.org> >> --- >> platform/linux-generic/odp_packet_io.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/platform/linux-generic/odp_packet_io.c >> b/platform/linux-generic/odp_packet_io.c >> index a03eeb1..7694236 100644 >> --- a/platform/linux-generic/odp_packet_io.c >> +++ b/platform/linux-generic/odp_packet_io.c >> @@ -247,7 +247,7 @@ odp_pktio_t odp_pktio_open(const char *dev, >> odp_buffer_pool_t pool) >> return ODP_PKTIO_INVALID; >> done: >> - strncpy(pktio_entry->s.name, dev, IFNAMSIZ); >> + strncpy(pktio_entry->s.name, dev, IFNAMSIZ - 1); >> unlock_entry_classifier(pktio_entry); >> return id; >> } > > This bug will never happen due to check for length of dev at the top of > function. Such spread out (and undocumented?) dependencies are not good. This could break because of some future change. > But fix will be ok to turn off coverity warning. I think that all such sprintf(buf) + buf[len-1]=0 expressions at least should be kept on the same line so that you can grep for sprintf (which is a known problematic function) and then directly see that the code does the right thing. This would possibly bring the line length above 80 but verifiable correctness is more important than some arbitrary line length. > > Maxim. > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
On Tue, Dec 16, 2014 at 11:13 AM, Ola Liljedahl <ola.liljedahl@linaro.org> wrote: > On 15 December 2014 at 23:20, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: >> On 12/16/2014 12:35 AM, Mike Holmes wrote: >>> >>> Termination of the string is not assured unless the string is >>> one char shorter than the buffer. >>> >>> Fixes coverity 83058 >>> >>> Signed-off-by: Mike Holmes <mike.holmes@linaro.org> >>> --- >>> platform/linux-generic/odp_packet_io.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/platform/linux-generic/odp_packet_io.c >>> b/platform/linux-generic/odp_packet_io.c >>> index a03eeb1..7694236 100644 >>> --- a/platform/linux-generic/odp_packet_io.c >>> +++ b/platform/linux-generic/odp_packet_io.c >>> @@ -247,7 +247,7 @@ odp_pktio_t odp_pktio_open(const char *dev, >>> odp_buffer_pool_t pool) >>> return ODP_PKTIO_INVALID; >>> done: >>> - strncpy(pktio_entry->s.name, dev, IFNAMSIZ); >>> + strncpy(pktio_entry->s.name, dev, IFNAMSIZ - 1); >>> unlock_entry_classifier(pktio_entry); >>> return id; >>> } >> >> This bug will never happen due to check for length of dev at the top of >> function. > Such spread out (and undocumented?) dependencies are not good. This > could break because of some future change. > >> But fix will be ok to turn off coverity warning. > I think that all such sprintf(buf) + buf[len-1]=0 expressions at least > should be kept on the same line so that you can grep for sprintf > (which is a known problematic function) and then directly see that the > code does the right thing. This would possibly bring the line length > above 80 but verifiable correctness is more important than some > arbitrary line length. Why not use snprintf(pktio_entry->s.name, IFNAMSIZ, "%s", dev) instead? It writes at most size bytes, including the null terminator. > > >> >> Maxim. >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
On 17 December 2014 at 10:38, Ciprian Barbu <ciprian.barbu@linaro.org> wrote: > On Tue, Dec 16, 2014 at 11:13 AM, Ola Liljedahl > <ola.liljedahl@linaro.org> wrote: >> On 15 December 2014 at 23:20, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: >>> On 12/16/2014 12:35 AM, Mike Holmes wrote: >>>> >>>> Termination of the string is not assured unless the string is >>>> one char shorter than the buffer. >>>> >>>> Fixes coverity 83058 >>>> >>>> Signed-off-by: Mike Holmes <mike.holmes@linaro.org> >>>> --- >>>> platform/linux-generic/odp_packet_io.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/platform/linux-generic/odp_packet_io.c >>>> b/platform/linux-generic/odp_packet_io.c >>>> index a03eeb1..7694236 100644 >>>> --- a/platform/linux-generic/odp_packet_io.c >>>> +++ b/platform/linux-generic/odp_packet_io.c >>>> @@ -247,7 +247,7 @@ odp_pktio_t odp_pktio_open(const char *dev, >>>> odp_buffer_pool_t pool) >>>> return ODP_PKTIO_INVALID; >>>> done: >>>> - strncpy(pktio_entry->s.name, dev, IFNAMSIZ); >>>> + strncpy(pktio_entry->s.name, dev, IFNAMSIZ - 1); >>>> unlock_entry_classifier(pktio_entry); >>>> return id; >>>> } >>> >>> This bug will never happen due to check for length of dev at the top of >>> function. >> Such spread out (and undocumented?) dependencies are not good. This >> could break because of some future change. >> >>> But fix will be ok to turn off coverity warning. >> I think that all such sprintf(buf) + buf[len-1]=0 expressions at least >> should be kept on the same line so that you can grep for sprintf "grep for strncpy" >> (which is a known problematic function) and then directly see that the >> code does the right thing. This would possibly bring the line length >> above 80 but verifiable correctness is more important than some >> arbitrary line length. > > Why not use snprintf(pktio_entry->s.name, IFNAMSIZ, "%s", dev) > instead? It writes at most size bytes, including the null terminator. Good suggestion. A neat little trick. -- Ola > >> >> >>> >>> Maxim. >>> >>> >>> _______________________________________________ >>> lng-odp mailing list >>> lng-odp@lists.linaro.org >>> http://lists.linaro.org/mailman/listinfo/lng-odp >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> http://lists.linaro.org/mailman/listinfo/lng-odp
diff --git a/platform/linux-generic/odp_packet_io.c b/platform/linux-generic/odp_packet_io.c index a03eeb1..7694236 100644 --- a/platform/linux-generic/odp_packet_io.c +++ b/platform/linux-generic/odp_packet_io.c @@ -247,7 +247,7 @@ odp_pktio_t odp_pktio_open(const char *dev, odp_buffer_pool_t pool) return ODP_PKTIO_INVALID; done: - strncpy(pktio_entry->s.name, dev, IFNAMSIZ); + strncpy(pktio_entry->s.name, dev, IFNAMSIZ - 1); unlock_entry_classifier(pktio_entry); return id; }
Termination of the string is not assured unless the string is one char shorter than the buffer. Fixes coverity 83058 Signed-off-by: Mike Holmes <mike.holmes@linaro.org> --- platform/linux-generic/odp_packet_io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)