Bug 72017 – [EFL] Replace alloca to C++ new placement

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Grzegorz Czajkowski]

Adds NULL checks before access to pointer which is initialized by alloca.

Adds NULL check to ewk_window_features_new_from_core after creating a new instance of WebCore's WindowFeatures.
It prevents unexpected result while calling ewk_window_features_unref if WindowFeatures is NULL.

Adds NULL check to ewk_cookies_get_all which returns list of cookies.
Cookie won't be added If malloc returns NULL.

[Grzegorz Czajkowski]

Created attachment 114478 [details]
proposed patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 114478 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=114478&action=review

> Source/WebCore/platform/efl/ScrollbarEfl.cpp:160
> +    Edje_Message_Float_Set* message = static_cast<Edje_Message_Float_Set*>
> +        (alloca(sizeof(Edje_Message_Float_Set) + sizeof(float)));

I'm not a big fan of alloca myself, as it is not very portable. Now that you're on this part of the code, it'd be good to replace it with new+delete or a smart pointer.

> Source/WebKit/efl/ewk/ewk_cookies.cpp:104
> +        } else
> +            CRITICAL("Could not allocate Ewk_Cookie.");

Has this happened in your daily usage? I'm asking because every once in a while patches checking for memory allocations are sent and most of the time they are rejected for the same reason (there's not much point in checking for every allocation failure across WebKit/WebCore).

> Source/WebKit/efl/ewk/ewk_window_features.cpp:126
> +    if (!window_features->core) {
> +        CRITICAL("Could not allocate WebCore::WindowFeatures.");
> +        return 0;
> +    }

Ditto.

[Grzegorz Czajkowski]

(In reply to comment #2)
> (From update of attachment 114478 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114478&action=review
> 
> > Source/WebCore/platform/efl/ScrollbarEfl.cpp:160
> > +    Edje_Message_Float_Set* message = static_cast<Edje_Message_Float_Set*>
> > +        (alloca(sizeof(Edje_Message_Float_Set) + sizeof(float)));
> 
> I'm not a big fan of alloca myself, as it is not very portable. Now that you're on this part of the code, it'd be good to replace it with new+delete or a smart pointer.

Yes, you're right. As I know all new/delete and malloc/free calls will be replaced in one patch so it would be better to use new+delete here instead of alloca.

> 
> > Source/WebKit/efl/ewk/ewk_cookies.cpp:104
> > +        } else
> > +            CRITICAL("Could not allocate Ewk_Cookie.");
> 
> Has this happened in your daily usage? I'm asking because every once in a while patches checking for memory allocations are sent and most of the time they are rejected for the same reason (there's not much point in checking for every allocation failure across WebKit/WebCore).

No, it hasn't. But generally WebKit-EFL checks the most of allocation failure. And some patches have already been landed, for example
https://bugs.webkit.org/show_bug.cgi?id=64932
https://bugs.webkit.org/show_bug.cgi?id=65853

[Grzegorz Czajkowski]

Created attachment 117406 [details]
updated patch

Replaced alloca to new/delete in compliance with WebKit coding style.

[Grzegorz Czajkowski]

Created attachment 117408 [details]
updated patch

In the previous patch only pointer was freed but whole table should be deleted.

[Grzegorz Czajkowski]

Created attachment 117409 [details]
updated patch

[Grzegorz Czajkowski]

Hi Kubo,
I uploaded a new patch - alloca was changed to new/delete.
Could you review this patch again?

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 117409 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117409&action=review

I still think it is not needed to check for all these conditions. Speaking of other bug reports, https://bugs.webkit.org/show_bug.cgi?id=65408#c12 still makes a lot of sense (even though the patch ended up being committed).

> Source/WebCore/platform/efl/ScrollbarEfl.cpp:162
> +    Edje_Message_Float_Set* message = 0;
> +    char* mem = 0;
> +    if (mem = new char[sizeof(Edje_Message_Float_Set) + sizeof(double)])
> +        message = new(mem) Edje_Message_Float_Set;

This ended up looking quite ugly due to Edje's brain-dead way of manipulating this data structure. My suggestion is to just use
  Edje_Message_Float_Set* message = static_cast<Edje_Message_Float_Set*>(malloc(sizeof(Edje_Message_Float_set) + sizeof(double)));
  if (!message) {
    // yadda yadda
  }
and then free() message after sending it.

If a reviewer prefers the use of placement new here, I suggest the following:
  char* mem = new char[sizeof(Edje_Message_Float_Set) * sizeof(double)];
  Edje_Message_Float_Set* message = new(mem) Edje_Message_Float_Set;
in this case, I don't even think it makes sense to check if the allocation fails, as new never returns NULL.

> Source/WebKit/efl/ewk/ewk_cookies.cpp:104
> +        } else
> +            CRITICAL("Could not allocate Ewk_Cookie.");

I still wonder if this is really needed. Assuming the hypothetical case in which malloc fails in the middle of the loop, you nonetheless continue trying to allocate other Ewk_Cookies to add them to the list and then return. I don't think this helps the caller all that much.

> Source/WebKit/efl/ewk/ewk_window_features.cpp:126
> +    if (!window_features->core) {
> +        CRITICAL("Could not allocate WebCore::WindowFeatures.");
> +        return 0;
> +    }

new never returns NULL.

[Grzegorz Czajkowski]

(In reply to comment #8)
> (From update of attachment 117409 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117409&action=review
> 
> I still think it is not needed to check for all these conditions. Speaking of other bug reports, https://bugs.webkit.org/show_bug.cgi?id=65408#c12 still makes a lot of sense (even though the patch ended up being committed).
> 
> > Source/WebCore/platform/efl/ScrollbarEfl.cpp:162
> > +    Edje_Message_Float_Set* message = 0;
> > +    char* mem = 0;
> > +    if (mem = new char[sizeof(Edje_Message_Float_Set) + sizeof(double)])
> > +        message = new(mem) Edje_Message_Float_Set;
> 
> This ended up looking quite ugly due to Edje's brain-dead way of manipulating this data structure. My suggestion is to just use
>   Edje_Message_Float_Set* message = static_cast<Edje_Message_Float_Set*>(malloc(sizeof(Edje_Message_Float_set) + sizeof(double)));
>   if (!message) {
>     // yadda yadda
>   }
> and then free() message after sending it.
> 
> If a reviewer prefers the use of placement new here, I suggest the following:
>   char* mem = new char[sizeof(Edje_Message_Float_Set) * sizeof(double)];
>   Edje_Message_Float_Set* message = new(mem) Edje_Message_Float_Set;
> in this case, I don't even think it makes sense to check if the allocation fails, as new never returns NULL

Ok, I will skip NULL checks for pointers initialized by new. 

> 
> > Source/WebKit/efl/ewk/ewk_cookies.cpp:104
> > +        } else
> > +            CRITICAL("Could not allocate Ewk_Cookie.");
> 
> I still wonder if this is really needed. Assuming the hypothetical case in which malloc fails in the middle of the loop, you nonetheless continue trying to allocate other Ewk_Cookies to add them to the list and then return. I don't think this helps the caller all that much.


Do you prefer to stop loop in case of malloc failure and destroy (by free()) allocated memory? A similar solution in used in ewk_frame_source_get().

> 
> > Source/WebKit/efl/ewk/ewk_window_features.cpp:126
> > +    if (!window_features->core) {
> > +        CRITICAL("Could not allocate WebCore::WindowFeatures.");
> > +        return 0;
> > +    }
> 
> new never returns NULL.

Ok, thanks.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #9)
> (In reply to comment #8)
> > > Source/WebKit/efl/ewk/ewk_cookies.cpp:104
> > > +        } else
> > > +            CRITICAL("Could not allocate Ewk_Cookie.");
> > 
> > I still wonder if this is really needed. Assuming the hypothetical case in which malloc fails in the middle of the loop, you nonetheless continue trying to allocate other Ewk_Cookies to add them to the list and then return. I don't think this helps the caller all that much.
> 
> Do you prefer to stop loop in case of malloc failure and destroy (by free()) allocated memory?

Well, I'm generally against the idea of checking all those allocations :) You will end up doing a lot more work for a hypothetical situation in which you are already pretty much screwed up.

> A similar solution in used in ewk_frame_source_get().

Sorry, I couldn't find similar code in that function.

[Grzegorz Czajkowski]

Created attachment 120943 [details]
proposed patch

As a conclusion of above discussion NULL checking isn't needed in these cases.
I only replaced calling alloca to C++ new placement for three reasons:
1. alloca it is not very portable.
2. NULL checking isn't required as new never returns NULL,
3. in compliance with WebKit coding style.

[Raphael Kubo da Costa (:rakuco)]

Should be OK, albeit ugly :)

[Grzegorz Czajkowski]

Created attachment 121226 [details]
updated patch

Buffer is created on stack so we don't need to delete it.

[Andreas Kling]

Comment on attachment 121226 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=121226&action=review

r=me

> Source/WebCore/platform/efl/ScrollbarEfl.cpp:164
> -    Edje_Message_Float_Set* message = static_cast<Edje_Message_Float_Set*>
> -        (alloca(sizeof(Edje_Message_Float_Set) + sizeof(float)));
> +    char buffer[sizeof(Edje_Message_Float_Set) + sizeof(double)];
> +    Edje_Message_Float_Set* message = reinterpret_cast<Edje_Message_Float_Set*>(buffer);

Please note in the ChangeLog why you are changing from float to double.

[Grzegorz Czajkowski]

Created attachment 121235 [details]
updated patch

Added reason of changing float to double to ChangeLog.
Patch for landing.

[WebKit Review Bot]

Comment on attachment 121235 [details]
updated patch

Clearing flags on attachment: 121235

Committed r104163: <http://trac.webkit.org/changeset/104163>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.