[PATCH] Fix crash while reading from mapped file

[PATCH] Fix crash while reading from mapped file

[Rafal Michalski]

After opening file from /var/lib/bluetooth/<bt_addr>/ and mapping to
memory as it is done in "textfile_foreach" function in textfile.c,
it may crash when size of file is equal to page size
(or it's multiplicity) since "strpbrk" function operates on string
so it expects zero at the end of buffer ("mmap" function zeroes
remaining memory when mapped only for a file which size is not a
multiple of the page size, so in this case "strpbrk" function can't
find null terminating character and goes out of bounds).
This patch provide buffer which contains null terminating character to
avoid crash.
---
 src/textfile.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/src/textfile.c b/src/textfile.c
index 2429cc7..393efb8 100644
--- a/src/textfile.c
+++ b/src/textfile.c
@@ -376,7 +376,7 @@ char *textfile_caseget(const char *pathname, const char *key)
 int textfile_foreach(const char *pathname, textfile_cb func, void *data)
 {
 	struct stat st;
-	char *map, *off, *end, *key, *value;
+	char *map, *off, *end, *key, *value, *buffer = NULL;
 	off_t size; size_t len;
 	int fd, err = 0;
 
@@ -404,6 +404,13 @@ int textfile_foreach(const char *pathname, textfile_cb func, void *data)
 
 	off = map;
 
+	if (!(size % getpagesize())) {
+		buffer = malloc(size + 1);
+		memset(buffer, 0, size + 1);
+		memcpy(buffer, map, size);
+		off = buffer;
+	}
+
 	while (1) {
 		end = strpbrk(off, " ");
 		if (!end) {
@@ -458,6 +465,7 @@ unlock:
 
 close:
 	close(fd);
+	free(buffer);
 	errno = err;
 
 	return 0;
-- 
1.6.3.3

Re: [PATCH] Fix crash while reading from mapped file

[Luiz Augusto von Dentz]

Hi,

On Fri, Dec 10, 2010 at 3:55 PM, Rafal Michalski
<michalski.raf@gmail.com> wrote:
> After opening file from /var/lib/bluetooth/<bt_addr>/ and mapping to
> memory as it is done in "textfile_foreach" function in textfile.c,
> it may crash when size of file is equal to page size
> (or it's multiplicity) since "strpbrk" function operates on string
> so it expects zero at the end of buffer ("mmap" function zeroes
> remaining memory when mapped only for a file which size is not a
> multiple of the page size, so in this case "strpbrk" function can't
> find null terminating character and goes out of bounds).
> This patch provide buffer which contains null terminating character to
> avoid crash.
> ---
>  src/textfile.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
>
> diff --git a/src/textfile.c b/src/textfile.c
> index 2429cc7..393efb8 100644
> --- a/src/textfile.c
> +++ b/src/textfile.c
> @@ -376,7 +376,7 @@ char *textfile_caseget(const char *pathname, const char *key)
>  int textfile_foreach(const char *pathname, textfile_cb func, void *data)
>  {
>        struct stat st;
> -       char *map, *off, *end, *key, *value;
> +       char *map, *off, *end, *key, *value, *buffer = NULL;
>        off_t size; size_t len;
>        int fd, err = 0;
>
> @@ -404,6 +404,13 @@ int textfile_foreach(const char *pathname, textfile_cb func, void *data)
>
>        off = map;
>
> +       if (!(size % getpagesize())) {
> +               buffer = malloc(size + 1);
> +               memset(buffer, 0, size + 1);
> +               memcpy(buffer, map, size);
> +               off = buffer;
> +       }

Isn't this doing exact the same as strncpy or g_strndup does? Actually
it is really too bad there is no str(n)pbrk variant.

>        while (1) {
>                end = strpbrk(off, " ");
>                if (!end) {
> @@ -458,6 +465,7 @@ unlock:
>
>  close:
>        close(fd);
> +       free(buffer);
>        errno = err;
>
>        return 0;

I also wonder why we didn't use g_strsplit for this, it seems it would
actually do the same, there is a little performance impact since we
would have to iterate on the vector generated to call the callback
while the current code do it in-place.


-- 
Luiz Augusto von Dentz
Computer Engineer

Re: [PATCH] Fix crash while reading from mapped file

[Lukasz Pawlik]

> I also wonder why we didn't use g_strsplit for this, it seems it would
> actually do the same, there is a little performance impact since we
> would have to iterate on the vector generated to call the callback
> while the current code do it in-place.

Marcel, Johan any thoughts about this ? Should we reimplement this
code using glib functions ?

Lukasz Pawlik

Re: [PATCH] Fix crash while reading from mapped file

[Johan Hedberg]

Hi Lukasz,

On Tue, Dec 14, 2010, Lukasz Pawlik wrote:
> > I also wonder why we didn't use g_strsplit for this, it seems it would
> > actually do the same, there is a little performance impact since we
> > would have to iterate on the vector generated to call the callback
> > while the current code do it in-place.
> 
> Marcel, Johan any thoughts about this ? Should we reimplement this
> code using glib functions ?

It'd be nice to get some comments from Marcel since he is the original
author of the textfile.c code. In general I'm not too happy about having
to allocate a duplicate amount of memory for this (which is completely
dependent on the file size), but I do agree that some fix is needed. I'd
also like to make sure that we get a fix upstream before the next BlueZ
release.

Johan

Re: [PATCH] Fix crash while reading from mapped file

[Bastien Nocera]

On Tue, 2010-12-14 at 17:34 +0200, Johan Hedberg wrote:
> Hi Lukasz,
> 
> On Tue, Dec 14, 2010, Lukasz Pawlik wrote:
> > > I also wonder why we didn't use g_strsplit for this, it seems it would
> > > actually do the same, there is a little performance impact since we
> > > would have to iterate on the vector generated to call the callback
> > > while the current code do it in-place.
> > 
> > Marcel, Johan any thoughts about this ? Should we reimplement this
> > code using glib functions ?
> 
> It'd be nice to get some comments from Marcel since he is the original
> author of the textfile.c code. In general I'm not too happy about having
> to allocate a duplicate amount of memory for this (which is completely
> dependent on the file size), but I do agree that some fix is needed. I'd
> also like to make sure that we get a fix upstream before the next BlueZ
> release.

If somebody can explain what that code is supposed to do, then writing a
glib-ish version should be trivial, without having to duplicate the
contents again. Then again, using something like:
g_mapped_file_new ();
g_strsplit ();
g_mapped_file_unref ();
wouldn't actually allocate more memory per se. You could also loop over
the content of each line, from the mmapped contents, but you'd be making
the code harder to read, with not much real world gain.

In a very worst case scenario, duping 50k of memory shouldn't be this
expensive. I would certainly understand the problem if we had megabyte
files though.

Cheers

Re: [PATCH] Fix crash while reading from mapped file

[Lukasz Pawlik]

Hi,

> If somebody can explain what that code is supposed to do, then writing a
> glib-ish version should be trivial, without having to duplicate the
> contents again. Then again, using something like:
> g_mapped_file_new ();
> g_strsplit ();
> g_mapped_file_unref ();

That won't fix anything since g_mapped_file_new uses mmap function so
contents may not be zero-terminated and we want use string function
next.

Lukasz Pawlik

Re: [PATCH] Fix crash while reading from mapped file

[Anderson Lizardo]

On Tue, Dec 14, 2010 at 2:20 PM, Lukasz Pawlik <lucas.pawlik@gmail.com> wrote:
> Hi,
>
>> If somebody can explain what that code is supposed to do, then writing a
>> glib-ish version should be trivial, without having to duplicate the
>> contents again. Then again, using something like:
>> g_mapped_file_new ();
>> g_strsplit ();
>> g_mapped_file_unref ();
>
> That won't fix anything since g_mapped_file_new uses mmap function so
> contents may not be zero-terminated and we want use string function
> next.

What about using g_strstr_len() instead of strpbrk() on the original
code? See http://library.gnome.org/devel/glib/unstable/glib-String-Utility-Functions.html#g-strstr-len

Regards,
-- 
Anderson Lizardo
OpenBossa Labs - INdT
Manaus - Brazil

Re: [PATCH] Fix crash while reading from mapped file

[Luiz Augusto von Dentz]

Hi,

On Wed, Dec 15, 2010 at 8:19 PM, Anderson Lizardo
<anderson.lizardo@openbossa.org> wrote:
> On Tue, Dec 14, 2010 at 2:20 PM, Lukasz Pawlik <lucas.pawlik@gmail.com> wrote:
>> Hi,
>>
>>> If somebody can explain what that code is supposed to do, then writing a
>>> glib-ish version should be trivial, without having to duplicate the
>>> contents again. Then again, using something like:
>>> g_mapped_file_new ();
>>> g_strsplit ();
>>> g_mapped_file_unref ();
>>
>> That won't fix anything since g_mapped_file_new uses mmap function so
>> contents may not be zero-terminated and we want use string function
>> next.
>
> What about using g_strstr_len() instead of strpbrk() on the original
> code? See http://library.gnome.org/devel/glib/unstable/glib-String-Utility-Functions.html#g-strstr-len

That looks to be a good replacement for strpbrk, probably 1 line patch.


-- 
Luiz Augusto von Dentz
Computer Engineer

Re: [PATCH] Fix crash while reading from mapped file

[Luiz Augusto von Dentz]

Hi,

On Thu, Dec 16, 2010 at 11:28 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi,
>
> On Wed, Dec 15, 2010 at 8:19 PM, Anderson Lizardo
> <anderson.lizardo@openbossa.org> wrote:
>> On Tue, Dec 14, 2010 at 2:20 PM, Lukasz Pawlik <lucas.pawlik@gmail.com> wrote:
>>> Hi,
>>>
>>>> If somebody can explain what that code is supposed to do, then writing a
>>>> glib-ish version should be trivial, without having to duplicate the
>>>> contents again. Then again, using something like:
>>>> g_mapped_file_new ();
>>>> g_strsplit ();
>>>> g_mapped_file_unref ();
>>>
>>> That won't fix anything since g_mapped_file_new uses mmap function so
>>> contents may not be zero-terminated and we want use string function
>>> next.
>>
>> What about using g_strstr_len() instead of strpbrk() on the original
>> code? See http://library.gnome.org/devel/glib/unstable/glib-String-Utility-Functions.html#g-strstr-len
>
> That looks to be a good replacement for strpbrk, probably 1 line patch.

I guess we cannot use any of glib functions here since textfile.c is
also used in some tools which doesn't link with glib, so if we don't
want to add this dependency to the than we need some other way to fix
it.

What about this:

diff --git a/src/textfile.c b/src/textfile.c
index 2429cc7..2e4c642 100644
--- a/src/textfile.c
+++ b/src/textfile.c
@@ -394,7 +394,7 @@ int textfile_foreach(const char *pathname,
textfile_cb func, void *data)
                goto unlock;
        }

-       size = st.st_size;
+       size = st.st_size + 1;

        map = mmap(NULL, size, PROT_READ, MAP_SHARED, fd, 0);
        if (!map || map == MAP_FAILED) {

It will probably use 1 more page if the file size is multiple of the
page size but it seems correct if you compare to e.g. malloc, well if
the possibility of the extra page is too much than we need our own
version of g_strstr_len/strpbrk_len like the following:

http://www.google.com/codesearch/p?hl=en#cZwlSNS7aEw/external/bluetooth/glib/glib/gstrfuncs.c&q=g_strstr_len&d=4

-- 
Luiz Augusto von Dentz
Computer Engineer

Re: [PATCH] Fix crash while reading from mapped file

[Luiz Augusto von Dentz]

Hi,

On Fri, Dec 17, 2010 at 11:29 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi,
>
> On Thu, Dec 16, 2010 at 11:28 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> Hi,
>>
>> On Wed, Dec 15, 2010 at 8:19 PM, Anderson Lizardo
>> <anderson.lizardo@openbossa.org> wrote:
>>> On Tue, Dec 14, 2010 at 2:20 PM, Lukasz Pawlik <lucas.pawlik@gmail.com> wrote:
>>>> Hi,
>>>>
>>>>> If somebody can explain what that code is supposed to do, then writing a
>>>>> glib-ish version should be trivial, without having to duplicate the
>>>>> contents again. Then again, using something like:
>>>>> g_mapped_file_new ();
>>>>> g_strsplit ();
>>>>> g_mapped_file_unref ();
>>>>
>>>> That won't fix anything since g_mapped_file_new uses mmap function so
>>>> contents may not be zero-terminated and we want use string function
>>>> next.
>>>
>>> What about using g_strstr_len() instead of strpbrk() on the original
>>> code? See http://library.gnome.org/devel/glib/unstable/glib-String-Utility-Functions.html#g-strstr-len
>>
>> That looks to be a good replacement for strpbrk, probably 1 line patch.
>
> I guess we cannot use any of glib functions here since textfile.c is
> also used in some tools which doesn't link with glib, so if we don't
> want to add this dependency to the than we need some other way to fix
> it.
>
> What about this:
>
> diff --git a/src/textfile.c b/src/textfile.c
> index 2429cc7..2e4c642 100644
> --- a/src/textfile.c
> +++ b/src/textfile.c
> @@ -394,7 +394,7 @@ int textfile_foreach(const char *pathname,
> textfile_cb func, void *data)
>                goto unlock;
>        }
>
> -       size = st.st_size;
> +       size = st.st_size + 1;
>
>        map = mmap(NULL, size, PROT_READ, MAP_SHARED, fd, 0);
>        if (!map || map == MAP_FAILED) {
>
> It will probably use 1 more page if the file size is multiple of the
> page size but it seems correct if you compare to e.g. malloc, well if
> the possibility of the extra page is too much than we need our own
> version of g_strstr_len/strpbrk_len like the following:
>
> http://www.google.com/codesearch/p?hl=en#cZwlSNS7aEw/external/bluetooth/glib/glib/gstrfuncs.c&q=g_strstr_len&d=4

The patch doesn't really work, it cause Non-existent physical address
error, anyway I have sent a patch that introduces strpbrk_len, but I
will probably be changing to strnpbrk to conform with some other strn*
variants.


-- 
Luiz Augusto von Dentz
Computer Engineer