1240790 - Null characters appear at then end of webrtc.org generated log messages

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Paul Kerr [:pkerr]]

Log lines generated from the webrtc.org code: media/webrtc/trunk/webrtc, have an ASCII null character (^@) on the end of each trace line where there should be a platform-specific line terminator. The result is a log file that reads as one continuous line.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Let's fix this asap and uplift it

Comment 2

[Paul Kerr [:pkerr]]

Attachment: Add newlines to WEBRTC_TRACE_FILE

Comment 3

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8719943 [details] [diff] [review]
Add newlines to WEBRTC_TRACE_FILE

Review of attachment 8719943 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good.

If interpreted as the number of bytes written to a file (including a terminating newline), the WEBRTC_TRACE_MAX_MESSAGE_SIZE is off by one as it also includes the c-string terminating null.

::: media/webrtc/trunk/webrtc/system_wrappers/source/trace_impl.cc
@@ +373,1 @@
>        length > WEBRTC_TRACE_MAX_MESSAGE_SIZE - written_so_far - 2) {

length is now including the newline character so should the check be against WEBRTC_TRACE_MAX_MESSAGE_SIZE - written_so_far - 1?

@@ +373,3 @@
>        length > WEBRTC_TRACE_MAX_MESSAGE_SIZE - written_so_far - 2) {
> +    length = -1;
> +    trace_message[written_so_far + 1] = 0;

I think the index should be written_so_far, here to add a terminating null. Additionally, if written_so_far == WEBRTC_TRACE_MAX_MESSAGE_SIZE - 1, it will pass the check on line 357 and overflow the bounds of trace_message on 375.

@@ -417,2 @@
>    if (row_count_text_ == 0) {
>      char message[WEBRTC_TRACE_MAX_MESSAGE_SIZE + 1];

Here the original author accounts for the \0 in the message dimensions.

@@ -435,4 @@
>    if (!TraceCheck(level))
>      return;
>  
>    char trace_message[WEBRTC_TRACE_MAX_MESSAGE_SIZE];

And here the original author does not account for the \0 in the message dimensions.

::: media/webrtc/trunk/webrtc/system_wrappers/source/trace_impl.h
@@ +35,5 @@
> +#define WEBRTC_TRACE_THREAD_ID_SIZE 12
> +#define WEBRTC_TRACE_LEVEL_SIZE 12
> +#define WEBRTC_TRACE_MODULE_ID_SIZE 25
> +#define WEBRTC_TRACE_TIMESTAMP_SIZE 22
> +

If this is only privately used by the trace_impl.cc, should it go in that file?

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8719943 [details] [diff] [review]
Add newlines to WEBRTC_TRACE_FILE

Review of attachment 8719943 [details] [diff] [review]:
-----------------------------------------------------------------

Lots of detail comments....

However: this is upstream code.  and this is a significant rewrite to avoid a failure to add \n's somewhere.

Perhaps there's a (much) simpler fix?

::: media/webrtc/trunk/webrtc/system_wrappers/source/trace_impl.cc
@@ +417,5 @@
>          return;
>        }
>      }
>    }
> +

don't add whitespace changes to upstream without a reason

@@ +422,4 @@
>    if (row_count_text_ == 0) {
>      char message[WEBRTC_TRACE_MAX_MESSAGE_SIZE + 1];
> +    int32_t len = AddDateTimeInfo(message);
> +    if (len != -1) {

Don't change the identifier for no reason in upstream code.  Also, if we're touching this, it should be < 0 like all the other AddFoo's

@@ +429,4 @@
>        row_count_text_++;
>      }
>    }
> +

avoid unnecessary whitespace changes to upstream code

@@ +443,5 @@
>  
>    char trace_message[WEBRTC_TRACE_MAX_MESSAGE_SIZE];
>    char* message_ptr = &trace_message[0];
>    int32_t len = AddLevel(message_ptr, level);
>    if (len == -1)

Ironically AddLevel's error return is 0

Comment 5

[Paul Kerr [:pkerr]]

I am torn between a quick fix and making this module less fragile. There are many assumptions that are shared between the message component generators. That being said, this module has been little changed for many releases. I will make pointed fix to the newline/null issue.

Comment 6

[Paul Kerr [:pkerr]]

Attachment: Add newlines to WEBRTC_TRACE_FILE

Comment 7

[Paul Kerr [:pkerr]]

Comment on attachment 8720440 [details] [diff] [review]
Add newlines to WEBRTC_TRACE_FILE

Going with the simple fix-up of double counting the null already handled by snprintf and adding in the '/n'.

Comment 8

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8720440 [details] [diff] [review]
Add newlines to WEBRTC_TRACE_FILE

Review of attachment 8720440 [details] [diff] [review]:
-----------------------------------------------------------------

This resolves my issues.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a74a55e4d17

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1a74a55e4d17