682353 - Add missing cases to details in JS_GetTraceThingInfo

Status: RESOLVED INCOMPLETE

(Core :: JavaScript Engine, defect)

Description

[Andrew McCreight [:mccr8]]

I noticed this while looking at JS_DumpHeap output.

Doing this:
        strcpy(dtrc.buffer, "XXXXXXXXXXXXXXXXXXXXXXX");
        JS_PrintTraceThingInfo(dtrc.buffer, sizeof(dtrc.buffer),
                               &dtrc, dci.node, dci.kind, JS_TRUE);
Produces this output:
Function XXXXXXXXXXXXXX

There should be a null terminator after the "Function ".  Not a safety problem, because JS_PrintTraceThingInfo puts a null terminator at the very end of the buffer, so no worry about running off into space.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: print a null string to the end of the buffer in the missing case

I kept around the extra writing of a null char as that's the only thing that kept us from corrupting memory in this case...

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: don't stomp on the null terminator

This fixes what I think is really the underlying problem: the line "*buf++ = ' '" overwrites the existing null-terminator, and does not put a new one in.  This only matters in one subcase, but if additional subcases are introduced that don't copy strings into the buffer, then they will also be affected.  With this approach, we don't have to worry about that.

If the JS_snprintf with a single character string seems heavyweight to you, then I could do
*buf++ = ' ';
*buf = '\0';
instead of the printf followed by buf++.

Comment 3

[Igor Bukanov]

Comment on attachment 557059 [details] [diff] [review]
don't stomp on the null terminator

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

::: js/src/jsapi.cpp
@@ +2262,5 @@
>      buf += n;
>      bufsize -= n;
>  
>      if (details && bufsize > 2) {
> +        JS_snprintf(buf, bufsize, " ");

The issue here is that we have missing JSTRACE_TYPE_OBJECT in the selector, not that we have missed to null-terminate the string.

Comment 4

[Andrew McCreight [:mccr8]]

That's a separate bug.  This one was present before TI went in.

This is what happens for a object that is a function in JS_PrintTraceThingInfo:
- the buffer contains characters like "** UNKNOWN SLOT 10**\0somerandomjunk" from the last time we called the printer.
- "Function\0" is copied into the buffer at the start.  The buffer now contains "Function\0N SLOT 10**\0somerandomjunk"
- the null terminator of the string is overwritten (line 2266, *buf++ = ' ';), so the buffer now contains "Function N SLOT 10**\0somerandomjunk"

In most cases, another string is now copied into the end of the buffer, and this string contains a null terminator, so the characters we placed into the buffer ends up null-terminated, so it is fine.  But in the case of an object that is a function, with a non-null function private, where fun == obj, and fun is not an atom, nothing else is put into the buffer at this point, so the string is still not terminated.
- in line 2323, a null terminator is placed at the very end of the buffer, ensuring that there is a null terminator, so the buffer ends up containing  "Function N SLOT 10**\0somerandomjun\0".
- we go to print it the buffer, and it produces this:
Function N SLOT 10**
The "N SLOT 10**" stuff is of course something we don't want.

With this patch, the buffer will end up containing "Function \0 SLOT 10**\0somerandomjunk", and we'll end up printing
Function 
as expected.

Comment 5

[Igor Bukanov]

(In reply to Andrew McCreight [:mccr8] from comment #4)
>  But in the case of an object
> that is a function, with a non-null function private, where fun == obj, and
> fun is not an atom, nothing else is put into the buffer at this point, so
> the string is still not terminated.

So the bug is at that place. We should print something like <noname> for atomless functions. As the code is for debugging my preference not to do extra protection but rather force the update of those lines. So we should add an assert that something is written to the buffer in all cases.

Comment 6

[Andrew McCreight [:mccr8]]

With the changes to this code in Bug 674251, this will also be a problem for shapes and type objects.

Comment 7

[Igor Bukanov]

(In reply to Andrew McCreight [:mccr8] from comment #6)
> With the changes to this code in Bug 674251, this will also be a problem for
> shapes and type objects.

That is why we need an assert that something useful is *always* printed so the code would be maintained. For the script it can be filename/linenumber, and surely there is some short info for the type.

Comment 8

[Andrew McCreight [:mccr8]]

Adding the line
  *buf = '\0';
after
  *buf++ = ' ';
is enough to ensure that the strings don't contain any garbage even if one of the cases doesn't write anything into the string.  We can remove the \0 store at the end, so it doesn't even cost any more operations.

Alternatively, I could just write a \0 into buf in those cases where we don't write anything else.

Do either of those options sound good to you?

In terms of filling out cases that don't currently have anything:

- type objects: it looks like js::types::TypeObjectString would work

- shapes: I don't see anything that could go here.  Shape::dump is the closest I can see, but it prints output to a file instead of to a string, and looks like it might have too much information in any event.

- function objects: I don't know if there's anything useful that can be done with non-atom function objects where fun == obj, but I don't really know what that means.

I'll try asking around if anybody has ideas for these latter two.

Comment 9

[Igor Bukanov]

(In reply to Andrew McCreight [:mccr8] from comment #8)
> Adding the line
>   *buf = '\0';
> after
>   *buf++ = ' ';
> is enough to ensure that the strings don't contain any garbage even if one
> of the cases doesn't write anything into the string.

I would prefer to put *buf = 0 under DEBUG and add assert that something is written into buf after the switch.

> In terms of filling out cases that don't currently have anything:
> 
> - type objects: it looks like js::types::TypeObjectString would work
> 
> - shapes: I don't see anything that could go here.  Shape::dump is the
> closest I can see, but it prints output to a file instead of to a string,
> and looks like it might have too much information in any event.

Just output JSShape::id.

> - function objects: I don't know if there's anything useful that can be done
> with non-atom function objects where fun == obj, but I don't really know
> what that means.

Print <noname>.

Comment 10

[Andrew McCreight [:mccr8]]

(In reply to Igor Bukanov from comment #9)
> I would prefer to put *buf = 0 under DEBUG and add assert that something is
> written into buf after the switch.
Well, the whole function is DEBUG only, but I can add an assert at the end.

> Just output JSShape::id.
> Print <noname>.

Great, thanks!

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: WIP patch to fix print trace thing

Here's my work-in-progress patch.  TypeObjectString is actually not very useful.  It just prints the address with a different kind of bracket depending on something or other that isn't obvious.  So maybe some other printing should be added.  I don't really have the time to deal with this right now, so somebody else can pick it up if they want.

Comment 12

[Andrew McCreight [:mccr8]]

Another thing that can be cleaned up in this function is #ifdef HAVE_XPCONNECT, which appears in the function and right before it.  This doesn't appear to be defined anywhere, so "#include "dump_xpc.h"" (this file does not appear to exist) and the block of code in the function proper can probably be removed.

Comment 13

[Andrew McCreight [:mccr8]]

The null thing has been fixed.  There are now a number of cases that don't have any details printed, but it is all internal JS engine stuff, so probably not a big deal.