339536 - string buffer: avoid frequent STRING_BUFFER_OK checks

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

js_GetToken from jsscan.c does not check for STRING_BUFFER_OK before accessing the string buffer via TOKENBUF_ macros after populating the buffer via  ADD_TO_TOKENBUF. This results in segmentation faults later instead of proper recovery from out-of-memory errors. I got such segfault during testing a patch to address bug 338653.

Comment 1

[Igor Bukanov]

Attachment: Fix

For simplicity I added the checks to ADD_TO_TOKENBUF macro itself rather then touching multiple places in jsscan.c. The branch prediction on modern CPU should take care of extra checks.

Comment 2

[Igor Bukanov]

Attachment: diff -p -U8 version of fix

Comment 3

[Igor Bukanov]

There are more places in jsscan.c that ignore checks for STRING_BUFFER_OK. For example, GetXMLEntity assumes that FastAppendChar never fails and similarly even with the patch applied js_GetToken continues to assume that js_AppendCString is always OK.

So a proper solution can either:

1. Remove STRING_BUFFER_OK etc. and use the standard way to report errors via return status. This would clatter the source with if checks but it would be simple to verify the end result.

2. Extend STRING_BUFFER_OK type of approach to its maximum. That is, pass a pointer to a status flag to all string buffer functions that can fail and set it to false on memory failure while keeping the buffer in the previous correct state and then check for the status when necessary.

To demonstrate a difference consider the following never-failed-sequence:

void a()
{
    ....
}     

void b()
{
    ....
}     

void c()
{
    a();
    a();
    b();
    somethingThatReallyCanNotFail();
}     

and lets transform it into a proper can-fail case. With the first approach one gets:

bool a()
{
    ....
    if (error)
        return false;
    return true;
}     

bool b()
{
    ....
    if (error)
        return false;
    return true;
}     

bool c()
{
    if (!a())
        return false; 
    if (!a())
        return false; 
    if (!b())
        return false; 
   somethingThatReallyCanNotFail();
   return true;
}     

while the second gives:

void a(bool *ok)
{
    if (!*ok)
        return;
    ....
    if (error)
        *ok = false;
}     

bool b()
{
    if (!*ok)
        return;
    ....
    if (error)
        *ok = false;
}     

void c(bool *ok)
{
    a(ok);
    a(ok);
    b(ok);
    if (!*ok)
        return;

    somethingThatReallyCanNotFail();
}     

For me the second version looks closer to the original version. Plus it contains fewer ifs as effectively checks per function call are replaced by checks per function. For this reason  that is why I would like to try it with jsscan.c

Comments?

Comment 4

[Brendan Eich [:brendan]]

Sure -- the idea was to minimize if statements.  If you can make operations on a failed stringbuffer or tokenbuffer harmless, so much the better.

/be

Comment 5

[Brendan Eich [:brendan]]

Code size and performance numbers for each approach would be great -- windows as well as gcc even better.

/be

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 223631 [details] [diff] [review]
Fix

It seems like there will be another patch coming. If I'm wrong, please re-request review.

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> (From update of attachment 223631 [details] [diff] [review] [edit])
> It seems like there will be another patch coming. If I'm wrong, please
> re-request review.

Right, I should clear the request as soon as it is clear that the patch is not ready.  

Comment 8

[Igor Bukanov]

Changing the bug title to reflect its new nature as OOM fixes would go to bug 324533.

Comment 9

[Igor Bukanov]

Attachment: Implementation v0

Here is a using-bugzilla-as-backup-store sort of attachment which I would like to test more before asking for review.

Comment 10

[Igor Bukanov]

Attachment: Implementation v1

Here is the list of changes:

1. The patch adds JSStringBuffer.errors flag which is set on memory-allocation and other unrecoverable errors. Since the buffer pointers are not modified, it allowed to check for the error state only if buffer does not have enough capacity to append more chars.

2. I replaced callback-type of memory allocation by an explicit flag to use either malloc and optimize for subsequent transfer of the buffer to JSString (jsxml case) or arena allocation (jsscan.c case). This avoids duplications to check for integer overflow on allocation etc. 

3. jsxml.c is changed to use the newly introduced js_CloseBufferAsString that hides all error processing on string exit from the user. It allowed to remove explicit management of buffer ownership from StringBuffer users. The price for that is dealing with closeOnReturn flag in few places to avoid buffer allocation if it would contain a copy of already existing JSString.

Comment 11

[Igor Bukanov]

Attachment: Implementation v2

Changes for this versionL

1. xml_toString_helper is switched to assemble the full string into the passed JSStringBuffer.

2. MakeXMLCDATAString, MakeXMLCommentString and MakeXMLPIString are replaced by calls to  js_MakeXMLSpecialString that takes a parameter describing the type of xml special source.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Igor, did you mean to request review on attachment 225003 [details] [diff] [review]?

Comment 13

[Igor Bukanov]

(In reply to comment #12)
> Igor, did you mean to request review on attachment 225003 [details] [diff] [review] [edit]?
> 

Not yet: I may try to extend the patch to use StringBuffer in FoldXMLConstants in jsparse.c and in the mean time bugzilla is a nice way to backup patches :)

Comment 14

[Igor Bukanov]

I am not working on the bug right now.

Comment 15

[Luke Wagner [:luke]]

STRING_BUFFER_OK was removed.