Bug on String::format_va under Linux

[Leandro Motta Barros]

Hello,

First, some kudos. After evaluating some logging libraries for a
project I am working on, I found that Logog is the best fit for our
needs. I had no trouble to integrate it with our codebase and even
implement some smallish extensions we needed. So, it seems you are
doind a nice job with Logog! Thanks a lot for it :-)

Second, some whining... :-) I tried to do something like this:

std::string theString("Hello, I am a very, very, very, very,
very, very, very, very, long string!");
LOGOG_WARN(" %s", theString.c_str());

This worked perfectly under Windows, but caused some memory corruption
errors under Linux (and didn't log the correct message, for that
matter).

And third, some debugging work:

It seems that the problem is in String::format_va(). Specifically, the
problem goes like this (we are on the non-unicode, posix path, the one
using vsnprintf()):

1. The "attempted size" is not large enough for the resulting string.
vsnprintf() reports this by returning the number of characters it
would have written to the buffer if it was large enough.

2. Therefore, at this moment we have nActualSize > nAttempedSize. We
rightly don't break out of the while loop; instead, we do
nAttemptedSize *= 2 and try again.

3. Hmm, not really. The while condition is not met. We get out of the
while loop, and the String ends up with bad data.

In a first look, it seems that simply changing the while loop from

while ( nActualSize < nAttemptedSize )

to

while ( true )

fixes the problem (after all, "right" condition was already tested
before, we would have "breaked" out of the loop if appropriate). That
said, I still haven't thought much about corner cases (when
nActualSize == nAttemptedSize, for instance). I also haven't checked
how would this affect the unicode codepath.

I'll look further into this issue, and report anything else I find.
Meanwhile, if you have any comment to share, I'd like to hear it.

Cheers,

LMB

[Leandro Motta Barros]

Hello again,

OK, I think I like the "while (true)" solution I proposed in thre
previous email. I'd just suggest one more little optimization. When
increasing nAttemptedSize for the next try, instead of doing

nAttemptedSize *= 2;

I think it is better to do

if (nActualSize > 0)
nAttemptedSize = nActualSize + 1;
else
nAttemptedSize *= 2;

This way, we are sure to allocate the right size, we know it.

What do you think about these two changes? I have looked only briefly
at the wide-character code paths, and I certainly not aware of any
other "big picture" issues these changes may cause.

Cheers,

LMB

> --
> You received this message because you are subscribed to the Google Groups "Logog Development" group.
> To post to this group, send email to logog...@googlegroups.com.
> To unsubscribe from this group, send email to logog-devel...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/logog-devel?hl=en.
>
>

[Leandro Motta Barros]

Me again...

One more thing: I believe that the calls to vsnprintf_s() and
vsnprintf() should take 'nAttemptedSize' (instead of 'nAttemptedSize -
1' as their second parameter. This parameter is the whole buffer size,
so there is no need to take the null terminator into account.

Keeping the '- 1' (together with this last optimization I sent) causes
a problem: the last character of the string is discarded whenever
nAttemptedSize is not large enough in the first try.

(I don't need to use the wide versions, so I haven't looked if the '-
1' is necessary for them.)

*Now* I think I have everything as I wish. Please let me know if you
see any problem with the changes I suggested. If you need more details
about them, just ask!

Cheers,

LMB

On Wed, Nov 23, 2011 at 11:24 AM, Leandro Motta Barros

[John Byrd]

Hi Leandro,

Thanks for all the review and consideration on the code. I'm going to
push an updated version of lstring.cpp that takes into account some of
your comments. Be aware that this area of code has been buggy in the
past, and there might be some corner cases I've not considered while
implementing your comments -- however, I've added a bunch of
clarifying comments that may help to sort out bugs as we continue to
find them.

Again, thanks for reviewing this code and let me know what you think
of the checkin.

jwb

On Wed, Nov 23, 2011 at 9:16 AM, Leandro Motta Barros

--
---

John Byrd
Gigantic Software
2102 Business Center Drive
Suite 144
Irvine, CA   92612-1001
http://www.giganticsoftware.com
T: (949) 892-3526 F: (206) 309-0850

[Leandro Motta Barros]

Hi John,

Thanks for considering (and applying!) the changes!

The checkin looks good for me. I just didn't understand this:

+
+/** Defining this allocates less space for the formatted strings; however, this
+ * optimization makes this function, which is called on every string format,
+ * into a linear-time function with respect to allocations and deallocations.
+ * Expect a large performance hit if you enable this optimization.
+ */
+#ifdef LOGOG_OPTIMIZE_FOR_SPACE
+ if (nActualSize > 0)
+ nAttemptedSize = nActualSize + 1;
+ else
+ nAttemptedSize *= 2;
+#else // LOGOG_OPTIMIZE_FOR_SPACE
nAttemptedSize *= 2;
+#endif // LOGOG_OPTIMIZE_FOR_SPACE
}

Why does this have a negative impact in performance? As far as I can
see, this would have a *positive* impact in performance, since we are
allocating a buffer that is surely big enough, instead of looping
doubling its size until it is big enough.

My idea was to improve the performance on platforms for which
*printf() tells us how much memory we need, but I may have missed
something.

Best regards,

LMB

[John Byrd]

Good point. I did change the optimization to add sizeof( LOGOG_CHAR )
to bytes to deal with the trailing NULL on Unicode platforms. Please
do a git pull and let me know if that is a little closer to your
intention. Again, thanks for all your eyes on the code.

jwb

On Mon, Nov 28, 2011 at 2:49 AM, Leandro Motta Barros

[Leandro Motta Barros]

Great! Now everything looks right for me. I updated my project to use
this latest Logog revision and so far everything is working perfectly
(Windows and Linux, no Unicode).

LMB

[John Byrd]

Good deal.  Out of curiosity, what's the nature of the project?

jwb

[Leandro Motta Barros]

Briefly, it is a system for visualization, manipulation and
interpretation of data captured by 3D (LIDAR) scanners.

LMB