Size the buffer used for the metrics log text properly....

Size the buffer used for the metrics log text properly.

The existing code was abusing the WriteInto interface (base/string_util.h).
The size passed to WriteInto should be the size of the string plus one for
a terminating NUL byte.

This bug caused the final newline in the log to be clipped.  c_str users
(there probably weren't any) would see the final newline which wouldn't be
wouldn't be followed by a NUL byte.

TEST=not really any
BUG=21733
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=26280

[Mark Mentovai, 2009-09-15 20:50:39]

Jim, when you get a sec, could you review this simple change?

I wrote:
> Reviewers: jar,
>
> Description:
> Size the buffer used for the metrics log text properly.
>
> The existing code was abusing the WriteInto interface (base/string_util.h=
).
> The size passed to WriteInto should be the size of the string plus one fo=
r
> a terminating NUL byte.
>
> This bug caused the final newline in the log to be clipped. =A0c_str user=
s
> (there probably weren't any) would see the final newline which wouldn't b=
e
> wouldn't be followed by a NUL byte.
>
> TEST=3Dnot really any
> BUG=3D21733
>
> Please review this at http://codereview.chromium.org/194099
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src/
>
> Affected files:
> =A0M =A0 =A0 chrome/browser/metrics/metrics_service.cc
>
>
> Index: chrome/browser/metrics/metrics_service.cc
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- chrome/browser/metrics/metrics_service.cc =A0 (revision 26002)
> +++ chrome/browser/metrics/metrics_service.cc =A0 (working copy)
> @@ -405,7 +405,7 @@
> =A0 =A0 =A0 server_permits_upload_(true),
> =A0 =A0 =A0 state_(INITIALIZED),
> =A0 =A0 =A0 pending_log_(NULL),
> - =A0 =A0 =A0pending_log_text_(""),
> + =A0 =A0 =A0pending_log_text_(),
> =A0 =A0 =A0 current_fetch_(NULL),
> =A0 =A0 =A0 current_log_(NULL),
> =A0 =A0 =A0 idle_since_last_transmission_(false),
> @@ -1205,9 +1205,11 @@
> =A0 DCHECK(pending_log());
> =A0 if (!pending_log_text_.empty())
> =A0 =A0 return;
> - =A0int original_size =3D pending_log_->GetEncodedLogSize();
> - =A0pending_log_->GetEncodedLog(WriteInto(&pending_log_text_, original_s=
ize),
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0original_siz=
e);
> + =A0int text_size =3D pending_log_->GetEncodedLogSize();
> +
> + =A0// Leave room for the NUL terminator.
> + =A0pending_log_->GetEncodedLog(WriteInto(&pending_log_text_, text_size =
+ 1),
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0text_size);
> =A0}
>
> =A0void MetricsService::PrepareFetchWithPendingLog() {

[jar (doing other things), 2009-09-15 22:17:35]

I wasn't familiar with this code, but the change LGTM.

I'll try to look around in our codebase and see if this is happening elsewhere. 
I don't especially like the API, and suspect it is vulnerable to this class of
mistake elsewhere.