[ovs-dev] dynamic-string: Fix a bug that leads to assertion fail

Message ID 1532446628-13331-1-git-send-email-pkusunyifeng@gmail.com
State New
Headers show
Series
  • [ovs-dev] dynamic-string: Fix a bug that leads to assertion fail
Related show

Commit Message

Yifeng Sun July 24, 2018, 3:37 p.m.
'needed' should be size of needed memory space beyond allocated.

Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
Reported-by: Yun Zhou <yunz@nvidia.com>
Reported-by: Girish Moodalbail <gmoodalbail@gmail.com>
---
 lib/dynamic-string.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ben Pfaff July 24, 2018, 9:47 p.m. | #1
On Tue, Jul 24, 2018 at 08:37:08AM -0700, Yifeng Sun wrote:
> 'needed' should be size of needed memory space beyond allocated.
> 
> Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
> Reported-by: Yun Zhou <yunz@nvidia.com>
> Reported-by: Girish Moodalbail <gmoodalbail@gmail.com>

I don't see a bug here.  Can you explain why you think that there is a
bug?

(I note that this code dates back to before 2008.)

Thanks,

Ben.
Yifeng Sun July 24, 2018, 10:31 p.m. | #2
Hi Ben,

vsnprintf returns the size that was truncated. So we need at least
ds->allocated + needed bytes to print the full string.
        needed = vsnprintf(&ds->string[ds->length], available, format,
args);

So ds_reserve should make sure ds contains at least ds->allocated + needed
bytes.
        ds_reserve(ds, ds->allocated + needed);

For example, if ds starts with:
length = 4, allocated = 8
Assume the to-be-printed string length = 10, then we got needed = 2
In current code, ds_reserve(4 + 2 = 6) is called, if go into ds_reserve(),
since (6 < 8), ds_reserve actually does nothing.

Thanks,
Yifeng

On Tue, Jul 24, 2018 at 2:47 PM, Ben Pfaff <blp@ovn.org> wrote:

> On Tue, Jul 24, 2018 at 08:37:08AM -0700, Yifeng Sun wrote:
> > 'needed' should be size of needed memory space beyond allocated.
> >
> > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
> > Reported-by: Yun Zhou <yunz@nvidia.com>
> > Reported-by: Girish Moodalbail <gmoodalbail@gmail.com>
>
> I don't see a bug here.  Can you explain why you think that there is a
> bug?
>
> (I note that this code dates back to before 2008.)
>
> Thanks,
>
> Ben.
>
Ben Pfaff July 24, 2018, 10:55 p.m. | #3
Some pre-ANSI C99 implementations of (v)snprintf() returned -1 if the
output was truncated, but C99 and SUSv3 require it to return the number
of bytes that would have been written if the buffer was large enough,
and for at least the last 10 years or so glibc has implemented it that
way.

Try building and running this program:

    #include <stdio.h>
    int
    main(void)
    {
       char x[9];
       return snprintf(x, sizeof x, "0123456789");
    }

The exit status is 10, not 1.

The glibc manual talks about this:

 -- Function: int snprintf (char *S, size_t SIZE, const char *TEMPLATE,
          ...)
     Preliminary: | MT-Safe locale | AS-Unsafe heap | AC-Unsafe mem |
     *Note POSIX Safety Concepts::.

     The 'snprintf' function is similar to 'sprintf', except that the
     SIZE argument specifies the maximum number of characters to
     produce.  The trailing null character is counted towards this
     limit, so you should allocate at least SIZE characters for the
     string S.  If SIZE is zero, nothing, not even the null byte, shall
     be written and S may be a null pointer.

     The return value is the number of characters which would be
     generated for the given input, excluding the trailing null.  If
     this value is greater or equal to SIZE, not all characters from the
     result have been stored in S.  You should try again with a bigger
     output string.  Here is an example of doing this:

          /* Construct a message describing the value of a variable
             whose name is NAME and whose value is VALUE. */
          char *
          make_message (char *name, char *value)
          {
            /* Guess we need no more than 100 chars of space. */
            int size = 100;
            char *buffer = (char *) xmalloc (size);
            int nchars;
            if (buffer == NULL)
              return NULL;

           /* Try to print in the allocated space. */
            nchars = snprintf (buffer, size, "value of %s is %s",
          		     name, value);
            if (nchars >= size)
              {
                /* Reallocate buffer now that we know
          	 how much space is needed. */
                size = nchars + 1;
                buffer = (char *) xrealloc (buffer, size);

                if (buffer != NULL)
          	/* Try again. */
          	snprintf (buffer, size, "value of %s is %s",
          		  name, value);
              }
            /* The last call worked, return the string. */
            return buffer;
          }

     In practice, it is often easier just to use 'asprintf', below.

     *Attention:* In versions of the GNU C Library prior to 2.1 the
     return value is the number of characters stored, not including the
     terminating null; unless there was not enough space in S to store
     the result in which case '-1' is returned.  This was changed in
     order to comply with the ISO C99 standard.

On Tue, Jul 24, 2018 at 03:31:12PM -0700, Yifeng Sun wrote:
> Hi Ben,
> 
> vsnprintf returns the size that was truncated. So we need at least
> ds->allocated + needed bytes to print the full string.
>         needed = vsnprintf(&ds->string[ds->length], available, format,
> args);
> 
> So ds_reserve should make sure ds contains at least ds->allocated + needed
> bytes.
>         ds_reserve(ds, ds->allocated + needed);
> 
> For example, if ds starts with:
> length = 4, allocated = 8
> Assume the to-be-printed string length = 10, then we got needed = 2
> In current code, ds_reserve(4 + 2 = 6) is called, if go into ds_reserve(),
> since (6 < 8), ds_reserve actually does nothing.
> 
> Thanks,
> Yifeng
> 
> On Tue, Jul 24, 2018 at 2:47 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> > On Tue, Jul 24, 2018 at 08:37:08AM -0700, Yifeng Sun wrote:
> > > 'needed' should be size of needed memory space beyond allocated.
> > >
> > > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
> > > Reported-by: Yun Zhou <yunz@nvidia.com>
> > > Reported-by: Girish Moodalbail <gmoodalbail@gmail.com>
> >
> > I don't see a bug here.  Can you explain why you think that there is a
> > bug?
> >
> > (I note that this code dates back to before 2008.)
> >
> > Thanks,
> >
> > Ben.
> >
Yifeng Sun July 24, 2018, 11:09 p.m. | #4
On ubuntu 16.04, vsnprintf shows below:

 The functions snprintf() and vsnprintf() do not write more than size bytes
(including the ter‐
       minating null byte ('\0')).  If the output was truncated due to this
limit,  then  the  return
       value  is the number of characters (excluding the terminating null
byte) which would have been
       written to the final string if enough space had been available.
Thus, a return value of  size
       or more means that the output was truncated.  (See also below under
NOTES.)

The glibc implementation of the functions snprintf() and vsnprintf()
conforms to the C99 stan‐
       dard,  that  is, behaves as described above, since glibc version
2.1.  Until glibc 2.0.6, they
       would return -1 when the output was truncated.

In this case, we need to check -1. There is definitely a bug here. I will
create another patch.
Thanks for the review.
Yifeng

On Tue, Jul 24, 2018 at 3:55 PM, Ben Pfaff <blp@ovn.org> wrote:

> Some pre-ANSI C99 implementations of (v)snprintf() returned -1 if the
> output was truncated, but C99 and SUSv3 require it to return the number
> of bytes that would have been written if the buffer was large enough,
> and for at least the last 10 years or so glibc has implemented it that
> way.
>
> Try building and running this program:
>
>     #include <stdio.h>
>     int
>     main(void)
>     {
>        char x[9];
>        return snprintf(x, sizeof x, "0123456789");
>     }
>
> The exit status is 10, not 1.
>
> The glibc manual talks about this:
>
>  -- Function: int snprintf (char *S, size_t SIZE, const char *TEMPLATE,
>           ...)
>      Preliminary: | MT-Safe locale | AS-Unsafe heap | AC-Unsafe mem |
>      *Note POSIX Safety Concepts::.
>
>      The 'snprintf' function is similar to 'sprintf', except that the
>      SIZE argument specifies the maximum number of characters to
>      produce.  The trailing null character is counted towards this
>      limit, so you should allocate at least SIZE characters for the
>      string S.  If SIZE is zero, nothing, not even the null byte, shall
>      be written and S may be a null pointer.
>
>      The return value is the number of characters which would be
>      generated for the given input, excluding the trailing null.  If
>      this value is greater or equal to SIZE, not all characters from the
>      result have been stored in S.  You should try again with a bigger
>      output string.  Here is an example of doing this:
>
>           /* Construct a message describing the value of a variable
>              whose name is NAME and whose value is VALUE. */
>           char *
>           make_message (char *name, char *value)
>           {
>             /* Guess we need no more than 100 chars of space. */
>             int size = 100;
>             char *buffer = (char *) xmalloc (size);
>             int nchars;
>             if (buffer == NULL)
>               return NULL;
>
>            /* Try to print in the allocated space. */
>             nchars = snprintf (buffer, size, "value of %s is %s",
>                              name, value);
>             if (nchars >= size)
>               {
>                 /* Reallocate buffer now that we know
>                  how much space is needed. */
>                 size = nchars + 1;
>                 buffer = (char *) xrealloc (buffer, size);
>
>                 if (buffer != NULL)
>                 /* Try again. */
>                 snprintf (buffer, size, "value of %s is %s",
>                           name, value);
>               }
>             /* The last call worked, return the string. */
>             return buffer;
>           }
>
>      In practice, it is often easier just to use 'asprintf', below.
>
>      *Attention:* In versions of the GNU C Library prior to 2.1 the
>      return value is the number of characters stored, not including the
>      terminating null; unless there was not enough space in S to store
>      the result in which case '-1' is returned.  This was changed in
>      order to comply with the ISO C99 standard.
>
> On Tue, Jul 24, 2018 at 03:31:12PM -0700, Yifeng Sun wrote:
> > Hi Ben,
> >
> > vsnprintf returns the size that was truncated. So we need at least
> > ds->allocated + needed bytes to print the full string.
> >         needed = vsnprintf(&ds->string[ds->length], available, format,
> > args);
> >
> > So ds_reserve should make sure ds contains at least ds->allocated +
> needed
> > bytes.
> >         ds_reserve(ds, ds->allocated + needed);
> >
> > For example, if ds starts with:
> > length = 4, allocated = 8
> > Assume the to-be-printed string length = 10, then we got needed = 2
> > In current code, ds_reserve(4 + 2 = 6) is called, if go into
> ds_reserve(),
> > since (6 < 8), ds_reserve actually does nothing.
> >
> > Thanks,
> > Yifeng
> >
> > On Tue, Jul 24, 2018 at 2:47 PM, Ben Pfaff <blp@ovn.org> wrote:
> >
> > > On Tue, Jul 24, 2018 at 08:37:08AM -0700, Yifeng Sun wrote:
> > > > 'needed' should be size of needed memory space beyond allocated.
> > > >
> > > > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
> > > > Reported-by: Yun Zhou <yunz@nvidia.com>
> > > > Reported-by: Girish Moodalbail <gmoodalbail@gmail.com>
> > >
> > > I don't see a bug here.  Can you explain why you think that there is a
> > > bug?
> > >
> > > (I note that this code dates back to before 2008.)
> > >
> > > Thanks,
> > >
> > > Ben.
> > >
>
Ben Pfaff July 24, 2018, 11:18 p.m. | #5
glibc 2.1 was released in 1999.

On Tue, Jul 24, 2018 at 04:09:00PM -0700, Yifeng Sun wrote:
> On ubuntu 16.04, vsnprintf shows below:
> 
>  The functions snprintf() and vsnprintf() do not write more than size bytes
> (including the ter‐
>        minating null byte ('\0')).  If the output was truncated due to this
> limit,  then  the  return
>        value  is the number of characters (excluding the terminating null
> byte) which would have been
>        written to the final string if enough space had been available.
> Thus, a return value of  size
>        or more means that the output was truncated.  (See also below under
> NOTES.)
> 
> The glibc implementation of the functions snprintf() and vsnprintf()
> conforms to the C99 stan‐
>        dard,  that  is, behaves as described above, since glibc version
> 2.1.  Until glibc 2.0.6, they
>        would return -1 when the output was truncated.
> 
> In this case, we need to check -1. There is definitely a bug here. I will
> create another patch.
> Thanks for the review.
> Yifeng
> 
> On Tue, Jul 24, 2018 at 3:55 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> > Some pre-ANSI C99 implementations of (v)snprintf() returned -1 if the
> > output was truncated, but C99 and SUSv3 require it to return the number
> > of bytes that would have been written if the buffer was large enough,
> > and for at least the last 10 years or so glibc has implemented it that
> > way.
> >
> > Try building and running this program:
> >
> >     #include <stdio.h>
> >     int
> >     main(void)
> >     {
> >        char x[9];
> >        return snprintf(x, sizeof x, "0123456789");
> >     }
> >
> > The exit status is 10, not 1.
> >
> > The glibc manual talks about this:
> >
> >  -- Function: int snprintf (char *S, size_t SIZE, const char *TEMPLATE,
> >           ...)
> >      Preliminary: | MT-Safe locale | AS-Unsafe heap | AC-Unsafe mem |
> >      *Note POSIX Safety Concepts::.
> >
> >      The 'snprintf' function is similar to 'sprintf', except that the
> >      SIZE argument specifies the maximum number of characters to
> >      produce.  The trailing null character is counted towards this
> >      limit, so you should allocate at least SIZE characters for the
> >      string S.  If SIZE is zero, nothing, not even the null byte, shall
> >      be written and S may be a null pointer.
> >
> >      The return value is the number of characters which would be
> >      generated for the given input, excluding the trailing null.  If
> >      this value is greater or equal to SIZE, not all characters from the
> >      result have been stored in S.  You should try again with a bigger
> >      output string.  Here is an example of doing this:
> >
> >           /* Construct a message describing the value of a variable
> >              whose name is NAME and whose value is VALUE. */
> >           char *
> >           make_message (char *name, char *value)
> >           {
> >             /* Guess we need no more than 100 chars of space. */
> >             int size = 100;
> >             char *buffer = (char *) xmalloc (size);
> >             int nchars;
> >             if (buffer == NULL)
> >               return NULL;
> >
> >            /* Try to print in the allocated space. */
> >             nchars = snprintf (buffer, size, "value of %s is %s",
> >                              name, value);
> >             if (nchars >= size)
> >               {
> >                 /* Reallocate buffer now that we know
> >                  how much space is needed. */
> >                 size = nchars + 1;
> >                 buffer = (char *) xrealloc (buffer, size);
> >
> >                 if (buffer != NULL)
> >                 /* Try again. */
> >                 snprintf (buffer, size, "value of %s is %s",
> >                           name, value);
> >               }
> >             /* The last call worked, return the string. */
> >             return buffer;
> >           }
> >
> >      In practice, it is often easier just to use 'asprintf', below.
> >
> >      *Attention:* In versions of the GNU C Library prior to 2.1 the
> >      return value is the number of characters stored, not including the
> >      terminating null; unless there was not enough space in S to store
> >      the result in which case '-1' is returned.  This was changed in
> >      order to comply with the ISO C99 standard.
> >
> > On Tue, Jul 24, 2018 at 03:31:12PM -0700, Yifeng Sun wrote:
> > > Hi Ben,
> > >
> > > vsnprintf returns the size that was truncated. So we need at least
> > > ds->allocated + needed bytes to print the full string.
> > >         needed = vsnprintf(&ds->string[ds->length], available, format,
> > > args);
> > >
> > > So ds_reserve should make sure ds contains at least ds->allocated +
> > needed
> > > bytes.
> > >         ds_reserve(ds, ds->allocated + needed);
> > >
> > > For example, if ds starts with:
> > > length = 4, allocated = 8
> > > Assume the to-be-printed string length = 10, then we got needed = 2
> > > In current code, ds_reserve(4 + 2 = 6) is called, if go into
> > ds_reserve(),
> > > since (6 < 8), ds_reserve actually does nothing.
> > >
> > > Thanks,
> > > Yifeng
> > >
> > > On Tue, Jul 24, 2018 at 2:47 PM, Ben Pfaff <blp@ovn.org> wrote:
> > >
> > > > On Tue, Jul 24, 2018 at 08:37:08AM -0700, Yifeng Sun wrote:
> > > > > 'needed' should be size of needed memory space beyond allocated.
> > > > >
> > > > > Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
> > > > > Reported-by: Yun Zhou <yunz@nvidia.com>
> > > > > Reported-by: Girish Moodalbail <gmoodalbail@gmail.com>
> > > >
> > > > I don't see a bug here.  Can you explain why you think that there is a
> > > > bug?
> > > >
> > > > (I note that this code dates back to before 2008.)
> > > >
> > > > Thanks,
> > > >
> > > > Ben.
> > > >
> >

Patch

diff --git a/lib/dynamic-string.c b/lib/dynamic-string.c
index 6f7b610a9908..4564e420544d 100644
--- a/lib/dynamic-string.c
+++ b/lib/dynamic-string.c
@@ -158,7 +158,7 @@  ds_put_format_valist(struct ds *ds, const char *format, va_list args_)
     if (needed < available) {
         ds->length += needed;
     } else {
-        ds_reserve(ds, ds->length + needed);
+        ds_reserve(ds, ds->allocated + needed);
 
         va_copy(args, args_);
         available = ds->allocated - ds->length + 1;