Closed Bug 682353 Opened 13 years ago Closed 7 years ago

Add missing cases to details in JS_GetTraceThingInfo

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED INCOMPLETE

People

(Reporter: mccr8, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
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...
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++.
Attachment #556120 - Attachment is obsolete: true
Attachment #557059 - Flags: review?(igor)
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.
Attachment #557059 - Flags: review?(igor) → review-
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.
(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.
Attachment #557059 - Flags: review- → review?(igor)
With the changes to this code in Bug 674251, this will also be a problem for shapes and type objects.
(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.
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.
(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>.
(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!
Summary: JS_PrintTraceThingInfo does not null terminate properly in one case → Add missing cases to details in JS_PrintTraceThingInfo
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.
Attachment #557059 - Attachment is obsolete: true
Attachment #557059 - Flags: review?(igor)
Assignee: continuation → general
No longer blocks: 680481
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.
Blocks: 709312
Blocks: 723783
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.
Status: ASSIGNED → NEW
Summary: Add missing cases to details in JS_PrintTraceThingInfo → Add missing cases to details in JS_GetTraceThingInfo
Assignee: general → nobody
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.