C++ Standard Library
  1. C++ Standard Library
  2. STDCXX-765

Incorrect using rw_asnprintf() with %{+} format and not NUL-terminated buffer in _rw_fmtflags(), _rw_fmtevent(), _rw_fmtlc()

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.2.0
    • Fix Version/s: 4.2.1
    • Component/s: Test Driver
    • Labels:
      None
    • Environment:

      All

    • Severity:
      Incorrect Behavior

      Description

      The buf parameter of the _rw_fmtflags(), _rw_fmtevent() and _rw_fmtlc() functions contains not NUL-terminated data. The length of data in buf is stored in buf.endoff.
      The rw_asnprintf() function accepts the only buf.pbuf and buf.pbufsize, but not buf.endoff. When %{+} format is used, the length of data in buf calculated using strlen() function, but this length is incorrect due to data is not NUL-terminated.
      Another bug is that _rw_fmtflags(), _rw_fmtevent() and _rw_fmtlc() functions are returns len, but without updating the buf.endoff. Due to this the result of rw_asnprintf() is cutted in further processing.

      These problems are detected in 0.printf test after latest update.

      The schematic patch without error checking is below. This patch is not intended to apply. It's only shows how these bugs should be fixed.

      Index: tests/src/fmt_bits.cpp
      ===================================================================
      --- tests/src/fmt_bits.cpp	(revision 637399)
      +++ tests/src/fmt_bits.cpp	(working copy)
      @@ -204,9 +204,12 @@
       
       #endif   // _RWSTD_NO_EXT_BIN_IO
       
      -        len = rw_asnprintf (buf.pbuf, buf.pbufsize,
      -                            "%{+} | %{?}std::ios::%{;}base(%d)",
      -                            spec.fl_pound, base);
      +        (*buf.pbuf) [buf.endoff] = '\0';
      +        int res = rw_asnprintf (buf.pbuf, buf.pbufsize,
      +                                "%{+} | %{?}std::ios::%{;}base(%d)",
      +                                spec.fl_pound, base);
      +        buf.endoff += res;
      +        len += res;
           }
       
           return len;
      @@ -303,9 +306,12 @@
               : std::ios::erase_event   == event ? "erase_event"
               : 0;
       
      -    return rw_asnprintf (buf.pbuf, buf.pbufsize,
      -                         "%{+}%{?}std::ios::%{;}%{?}%s%{:}event(%d)%{;}",
      -                         spec.fl_pound, 0 != str, str, event);
      +    (*buf.pbuf) [buf.endoff] = '\0';
      +    int len = rw_asnprintf (buf.pbuf, buf.pbufsize,
      +                            "%{+}%{?}std::ios::%{;}%{?}%s%{:}event(%d)%{;}",
      +                            spec.fl_pound, 0 != str, str, event);
      +    buf.endoff += len;
      +    return len;
       }
       
       /********************************************************************/
      @@ -329,8 +335,12 @@
       
           }
       
      -    if (str)
      -        return rw_asnprintf (buf.pbuf, buf.pbufsize, "%{+}%s", str);
      +    if (str) {
      +        (*buf.pbuf) [buf.endoff] = '\0';
      +        int len = rw_asnprintf (buf.pbuf, buf.pbufsize, "%{+}%s", str);
      +        buf.endoff += len;
      +        return len;
      +    }
       
           static const Bitnames names [] = {
               BITNAME (std::locale, all),
      
      1. stdcxx-765.diff
        3 kB
        Martin Sebor

        Activity

        Hide
        Martin Sebor added a comment -

        Hmm, 0.printf fails 90 assertions on trunk. I tried to find the change to printf.cpp that's responsible for this but couldn't. Are you saying you know what caused this?

        Show
        Martin Sebor added a comment - Hmm, 0.printf fails 90 assertions on trunk. I tried to find the change to printf.cpp that's responsible for this but couldn't. Are you saying you know what caused this?
        Hide
        Martin Sebor added a comment -

        Attached a patch.

        ChangeLog:

        2008-03-16  Martin Sebor  <sebor@roguewave.com>
        
        	STDCXX-765
        	* tests/src/fmt_bits.cpp (_rw_bmpfmt): NUL-terminated string so that
        	it can be appended to using the %{+} directive, taking care not to
        	make the NUL a part of the formatted string.
        	(_rw_fmtflags): Same.
        	(_rw_fmtevent): Same.
        	(_rw_fmtlc): Same.
        
        Show
        Martin Sebor added a comment - Attached a patch. ChangeLog: 2008-03-16 Martin Sebor <sebor@roguewave.com> STDCXX-765 * tests/src/fmt_bits.cpp (_rw_bmpfmt): NUL-terminated string so that it can be appended to using the %{+} directive, taking care not to make the NUL a part of the formatted string. (_rw_fmtflags): Same. (_rw_fmtevent): Same. (_rw_fmtlc): Same.
        Hide
        Martin Sebor added a comment -

        I stepped through the code and I think I've fixed all the occurrences of the problem – the 0.printf.cpp test passes all assertions with the attached patch applied. Let me know if it looks good to you. I don't think the code in fmt_bits.cpp has changed recently so I'm still curious what was masking the bug and what change exposed it...

        Show
        Martin Sebor added a comment - I stepped through the code and I think I've fixed all the occurrences of the problem – the 0.printf.cpp test passes all assertions with the attached patch applied. Let me know if it looks good to you. I don't think the code in fmt_bits.cpp has changed recently so I'm still curious what was masking the bug and what change exposed it...
        Hide
        Martin Sebor added a comment -

        Btw., about your proposed patch. I think the approach is correct (my patch does essentially the same thing); the one problem that jumps out at me is that it writes the terminating NUL directly to the buffer without making sure there's sufficient room.

        Show
        Martin Sebor added a comment - Btw., about your proposed patch. I think the approach is correct (my patch does essentially the same thing); the one problem that jumps out at me is that it writes the terminating NUL directly to the buffer without making sure there's sufficient room.
        Hide
        Farid Zaripov added a comment - - edited

        The %

        {If} and %{Ie} format's along with %{Lc} format weren't exercised in 0.printf until this commit.

        I found this bug when I worked on 22.locale.num.put test and one of the rw_assertion's with %{If}

        format directive has failed.

        Show
        Farid Zaripov added a comment - - edited The % {If} and %{Ie} format's along with %{Lc} format weren't exercised in 0.printf until this commit. I found this bug when I worked on 22.locale.num.put test and one of the rw_assertion's with %{If} format directive has failed.
        Hide
        Farid Zaripov added a comment -

        As for the my patch. It's not a patch, it's just light's the required steps to fix this bug.

        Show
        Farid Zaripov added a comment - As for the my patch. It's not a patch, it's just light's the required steps to fix this bug.
        Hide
        Farid Zaripov added a comment -

        I have reviewed your patch and patch is ok.

        Show
        Farid Zaripov added a comment - I have reviewed your patch and patch is ok.
        Hide
        Martin Sebor added a comment -

        Patch committed in r638391.
        Will close after integrating to 4.2.x in the next bulk merge.

        Show
        Martin Sebor added a comment - Patch committed in r638391 . Will close after integrating to 4.2.x in the next bulk merge.
        Hide
        Farid Zaripov added a comment -
        Show
        Farid Zaripov added a comment - Merged in 4.2.x branch thus: http://svn.apache.org/viewvc?view=rev&revision=648752

          People

          • Assignee:
            Martin Sebor
            Reporter:
            Farid Zaripov
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development