[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Requesting freeze exception for cwidget



  Hi,

  I'd like to request a freeze exemption for the cwidget library so
that 0.5.12-3 can replace 0.5.12-1.

  The 0.5.12-3 upload fixes a nasty string-handling bug in "swsprintf",
a safe sprintf replacement that's used all over cwidget and aptitude.
As far as I know the impact is limited to adding garbage at the end of
long output strings, but it's already caused problems for users of
aptitude in the Russian locale. (#496119)

  I also fixed ssprintf in some cases where it failed to call va_end();
from the manpage, I think this was at least a potential memory leak.

  I've attached a debdiff between the two versions.

--- cwidget-0.5.12/debian/control
+++ cwidget-0.5.12/debian/control
@@ -2,7 +2,7 @@
 Priority: extra
 Section: libs
 Homepage: http://cwidget.alioth.debian.org
-Vcs-Git: git://git.debian.org/cwidget/debian
+Vcs-Git: git://git.debian.org/git/cwidget/debian
 Vcs-Browser: http://git.debian.org/?p=cwidget/debian/.git;a=summary
 Maintainer: Daniel Burrows <dburrows@debian.org>
 Build-Depends: debhelper (>= 5.0.0), libsigc++-2.0-dev,

  This fixes the URL to the Git repository.

--- cwidget-0.5.12.orig/tests/test_ssprintf.cc
+++ cwidget-0.5.12/tests/test_ssprintf.cc
  [snip]

  A test case has been added to verify that swsprintf can correctly
generate long output strings and empty output strings:

+  void test_swsprintf()
+  {
+    // Test that inserting very long strings via ssprintf actually works.
+    std::wstring horriblelongthing = L"abcdefghijklmnopqrstuvwxyz";
+    while(horriblelongthing.size() < 4096)
+      horriblelongthing += horriblelongthing;
+
+    CPPUNIT_ASSERT_EQUAL(horriblelongthing + L" 20", swsprintf(L"%ls %d", horriblelongthing.c_str(), 20));
+
+    // Test that we can generate empty strings.
+    CPPUNIT_ASSERT_EQUAL(std::wstring(), swsprintf(L"%s", ""));
+  }

--- cwidget-0.5.12.orig/src/cwidget/generic/util/ssprintf.cc
+++ cwidget-0.5.12/src/cwidget/generic/util/ssprintf.cc
@@ -51,7 +51,10 @@
       const int amt = vsnprintf(buf, initbufsize, format, ap);
 
       if(amt < initbufsize)
-       return buf;
+       {
+         va_end(ap2);
+         return buf;
+       }
       else
        {
          const int buf2size = amt + 1;

  Call va_end() on a copied argument list before returning.

@@ -64,6 +67,7 @@
          string rval(buf2, amt2);
 
          delete[] buf2;
+         va_end(ap2);
 
          return rval;
        }

  The same.

  The last hunk is essentially a complete rewrite of the vswsprintf
routine.  For your convenience, here's the new code:

    wstring vswsprintf(const wchar_t *format, va_list ap)
    {
      // Unlike sprintf, swsprintf just returns -1 when there's an
      // overflow.  So we have to keep making bigger buffers until we
      // have one that's big enough.

      bool ok = false;

      wstring rval;
      int bufsize = initbufsize;

      while(!ok)
        {
          va_list ap2;
          va_copy(ap2, ap);

          wchar_t *buf = new wchar_t[bufsize];

          int amt = vswprintf(buf, bufsize, format, ap2);

          if(amt >= 0 && amt < bufsize)
            {
              rval = buf;
              ok = true;
            }
          else
            bufsize *= 2;

          delete[] buf;

          va_end(ap2);
        }

      return rval;
    }

  The old version looked like this:

    wstring vswsprintf(const wchar_t *format, va_list ap)
    {
      wchar_t buf[initbufsize];
      int amt = vswprintf(buf, initbufsize, format, ap);

      if(amt < initbufsize)
        return buf;
      else
        {
          wchar_t *buf2 = new wchar_t[amt+1];

          int amt2 = vswprintf(buf2, initbufsize, format, ap);

          eassert(amt2 < amt+1);

          wstring rval(buf2, amt2);

          delete[] buf2;

          return rval;
        }
    }

  The basic problem was that I somehow got the idea that vswprintf
worked like vsprintf, which returns the desired buffer size if there
wasn't enough room.  No such luck.  vswprintf actually returns -1 when
it fails; the only way to find the right buffer size is to try again
with a bigger buffer.  The old code was, as you can see, just returning
immediately with a partially filled buffer; since the buffer didn't get
null-terminated, that had the effect of returning a bunch of garbage
concatenated onto the end of a truncated string at the 512-byte mark.

    Thanks,
  Daniel
diff -u cwidget-0.5.12/debian/changelog cwidget-0.5.12/debian/changelog
--- cwidget-0.5.12/debian/changelog
+++ cwidget-0.5.12/debian/changelog
@@ -1,3 +1,11 @@
+cwidget (0.5.12-2) unstable; urgency=low
+
+  * Backport a fix for a string truncation bug from HEAD (Closes: #496119).
+
+  * Fix the URL to the packaging VCS (Closes: #492584).
+
+ -- Daniel Burrows <dburrows@debian.org>  Sun, 31 Aug 2008 22:01:53 -0700
+
 cwidget (0.5.12-1) unstable; urgency=low
 
   * New upstream version.
diff -u cwidget-0.5.12/debian/control cwidget-0.5.12/debian/control
--- cwidget-0.5.12/debian/control
+++ cwidget-0.5.12/debian/control
@@ -2,7 +2,7 @@
 Priority: extra
 Section: libs
 Homepage: http://cwidget.alioth.debian.org
-Vcs-Git: git://git.debian.org/cwidget/debian
+Vcs-Git: git://git.debian.org/git/cwidget/debian
 Vcs-Browser: http://git.debian.org/?p=cwidget/debian/.git;a=summary
 Maintainer: Daniel Burrows <dburrows@debian.org>
 Build-Depends: debhelper (>= 5.0.0), libsigc++-2.0-dev,
only in patch2:
unchanged:
--- cwidget-0.5.12.orig/tests/test_ssprintf.cc
+++ cwidget-0.5.12/tests/test_ssprintf.cc
@@ -1,6 +1,6 @@
 // Tests for generic/util/ssprintf.
 //
-//   Copyright (C) 2007 Daniel Burrows
+//   Copyright (C) 2007-2008 Daniel Burrows
 //
 //   This program is free software; you can redistribute it and/or
 //   modify it under the terms of the GNU General Public License as
@@ -19,12 +19,36 @@
 
 #include <cppunit/extensions/HelperMacros.h>
 
+#include <cppunit/TestAssert.h>
+
 #include <cwidget/generic/util/ssprintf.h>
 
+// For displaying assertion failures.
+#include <cwidget/generic/util/transcode.h>
+
 #include <errno.h>
 #include <string.h>
 
 using cwidget::util::ssprintf;
+using cwidget::util::swsprintf;
+
+CPPUNIT_NS_BEGIN
+
+template <>
+struct assertion_traits<std::wstring>
+{
+  static bool equal(const std::wstring &x, const std::wstring &y)
+  {
+    return x == y;
+  }
+
+  static std::string toString(const std::wstring &x)
+  {
+    return cwidget::util::transcode(x);
+  }
+};
+
+CPPUNIT_NS_END
 
 class SSPrintfTest : public CppUnit::TestFixture
 {
@@ -32,6 +56,7 @@
 
   CPPUNIT_TEST(test_sstrerror);
   CPPUNIT_TEST(test_ssprintf);
+  CPPUNIT_TEST(test_swsprintf);
 
   CPPUNIT_TEST_SUITE_END();
 private:
@@ -62,6 +87,16 @@
 
     CPPUNIT_ASSERT_EQUAL(horriblelongthing + " 20", ssprintf("%s %d", horriblelongthing.c_str(), 20));
   }
+
+  void test_swsprintf()
+  {
+    // Test that inserting very long strings via ssprintf actually works.
+    std::wstring horriblelongthing = L"abcdefghijklmnopqrstuvwxyz";
+    while(horriblelongthing.size() < 4096)
+      horriblelongthing += horriblelongthing;
+
+    CPPUNIT_ASSERT_EQUAL(horriblelongthing + L" 20", swsprintf(L"%ls %d", horriblelongthing.c_str(), 20));
+  }
 };
 
 CPPUNIT_TEST_SUITE_REGISTRATION(SSPrintfTest);
only in patch2:
unchanged:
--- cwidget-0.5.12.orig/src/cwidget/generic/util/ssprintf.cc
+++ cwidget-0.5.12/src/cwidget/generic/util/ssprintf.cc
@@ -51,7 +51,10 @@
       const int amt = vsnprintf(buf, initbufsize, format, ap);
 
       if(amt < initbufsize)
-	return buf;
+	{
+	  va_end(ap2);
+	  return buf;
+	}
       else
 	{
 	  const int buf2size = amt + 1;
@@ -64,6 +67,7 @@
 	  string rval(buf2, amt2);
 
 	  delete[] buf2;
+	  va_end(ap2);
 
 	  return rval;
 	}
@@ -82,25 +86,38 @@
 
     wstring vswsprintf(const wchar_t *format, va_list ap)
     {
-      wchar_t buf[initbufsize];
-      int amt = vswprintf(buf, initbufsize, format, ap);
+      // Unlike sprintf, swsprintf just returns -1 when there's an
+      // overflow.  So we have to keep making bigger buffers until we
+      // have one that's big enough.
 
-      if(amt < initbufsize)
-	return buf;
-      else
+      bool ok = false;
+
+      wstring rval;
+      int bufsize = initbufsize;
+
+      while(!ok)
 	{
-	  wchar_t *buf2 = new wchar_t[amt+1];
+	  va_list ap2;
+	  va_copy(ap2, ap);
 
-	  int amt2 = vswprintf(buf2, initbufsize, format, ap);
+	  wchar_t *buf = new wchar_t[bufsize];
 
-	  eassert(amt2 < amt+1);
+	  int amt = vswprintf(buf, bufsize, format, ap2);
 
-	  wstring rval(buf2, amt2);
+	  if(amt > 0 && amt < bufsize)
+	    {
+	      rval = buf;
+	      ok = true;
+	    }
+	  else
+	    bufsize *= 2;
 
-	  delete[] buf2;
+	  delete[] buf;
 
-	  return rval;
+	  va_end(ap2);
 	}
+
+      return rval;
     }
 
     // There are two variants of strerror_r to watch out for.  The GNU

Reply to: