stdcxx-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Travis Vitek" <Travis.Vi...@roguewave.com>
Subject RE: svn commit: r647908 - in /stdcxx/trunk/tests: self/0.printf.cpp src/fmt_defs.h src/printf.cpp
Date Mon, 14 Apr 2008 21:55:54 GMT

Martin Sebor wrote:
>
> Travis Vitek wrote:
>> 
>> 
>> I just noticed that the code in fmt_bits.cpp uses rw_asnprintf()
>> expecting that the buffer reallocation done internally would free
>> the input buffer if necessary. Previously that might have caused 
>> the assert that my change was supposed to fix. Now that code
>> might leak the buffer. So I'm putting together a patch to fix
>> that now.
>
>Yeah, the code there (the "%{+}" directives) is kind of clunky.
>

Okay, I've opened a can of worms. Personally, I think what I've done is
the right way to fix this, but the test driver and tests depend on the
behavior that caused the problem in the first place.

I see code like this in both the test driver and the tests.

    size_t bufsize = 0;
    char *sequences = 0;

    // verify the length of each character
    for (std::size_t i = 0; i < mb_cur_max; ++i) {
        const std::size_t mb_len = std::strlen (mb_chars [i].mbchar);

        if (i + 1 != mb_len) {
            rw_assert (0, 0, __LINE__,
                       "unexpected multibyte character length: "
                       "%u, expected %u", mb_len, i + 1);
            return;
        }

        rw_asnprintf (&sequences, &bufsize,
                      "%{+}%s{ %{#lc}, %{#s} }",
                      i ? ", " : "",
                      mb_chars [i].wchar,
                      mb_chars [i].mbchar);
    }

    rw_info (0, 0, __LINE__,
             "locale (\"%s\") [libc-based encoding, "
             "MB_CUR_MAX = %u, multi-byte characters: %s]",
             locname, mb_cur_max, (const char*)sequences);

    std::free (sequences);

The problem with this is that rw_asnprintf() no longer frees buffers
that are passed in. The rw_asnprintf() call will allocate space to
append the given string, but it will no longer attempt to deallocate
that string. The n'th time through the loop the buffer may need to be
resized. A new buffer will be allocated, but the previous buffer won't
be deallocated. So we leak.

To fix this we need to modify the test to cache the a pointer to the old
buffer, make the call, and then check to see if a new buffer was
allocated. If it was, then we need to deallocate the cached buffer.

I also see code like this...

    char* buf = 0;
    size_t bufsz = 0;

    rw_asnprintf (&buf, &bufsz, ...);

    if (cond1)
        rw_asnprintf (&buf, &bufsz, "{+}...);
    else if (cond2)
        rw_asnprintf (&buf, &bufsz, "{+}...);

    rw_asnprintf (&buf, &bufsz, "{+}...);

It has the same problem. It needs to check to see if the buffer was
reallocated and cleanup if it was.

So now I'm asking myself the question. Should I just revert this change
and then 'fix' all of the existing code that uses static buffers to use
dynamic ones, or should I be fixing the code that is expecting
rw_asnprintf() to deallocate the buffer. I can't really use the
asprintf() as a reference because rw_asnprintf() behave very differently
in this respect. The asprintf() function always allocates a buffer of
the appropriate size and never looks at the contents of the provided
pointer.

In case it matters. I think I can fix the offending test driver/suite
code in about 3 hours.

Travis

Mime
View raw message