Range check error in cef_dialog_handler_on_file_dialog

[hvassbotn]

[Note: This has been posted as an Issue in the github project site as well]

We were testing our CEF4Delphi-based embedded browser on slack.com, trying to upload a local file from the attachment command, when it crashed with a range check error, here:

Code: Select all

   function cef_dialog_handler_on_file_dialog(self: PCefDialogHandler; browser: PCefBrowser;
     mode: TCefFileDialogMode; const title, default_file_path: PCefString;
     accept_filters: TCefStringList; selected_accept_filter: Integer;
     callback: PCefFileDialogCallback): Integer; stdcall;
   var
     list: TStringList;
     i: Integer;
     str: TCefString;
   begin
     list := TStringList.Create;
     try
       for i := 0 to cef_string_list_size(accept_filters) - 1 do

The reason for the crash is that cef_string_list_size returns 0, and it is defined as returning an unsigned integer (NativeUInt). When the compiler subtracts 1, it wraps around to $FFFFFFFF, triggering the range check error, because it does not fit in a signed integer.

The best fix is to update the declaration of cef_string_list_size in uCEFLibFunctions.pas, like this:

Code: Select all

   //  cef_string_list_size   : function(list: TCefStringList): NativeUInt; {$IFDEF CPUX64}stdcall{$ELSE}cdecl{$ENDIF};
    // HV: Changed NativeUInt to Integer to avoid range error when using for i := 0 to cef_string_list_size(X)-1 and size is 0
    cef_string_list_size   : function(list: TCefStringList): Integer; {$IFDEF CPUX64}stdcall{$ELSE}cdecl{$ENDIF};

There are many other places in the CEF4Delphi code that uses cef_string_list_size the same way, and thus exposed to this bug.

This change allows us to keep the clean, logical for loop. I can see that this probably has tripped up code elsewhere that uses this function, as it has been written as a clunky while-do loop, for instance:

uCEFMiscFunctions.pas, CefStringListToStringList:

Code: Select all

  procedure CefStringListToStringList(var aSrcSL : TCefStringList; var aDstSL : TStrings);
  var
    i, j : NativeUInt;
    TempString : TCefString;
  begin
    if (aSrcSL <> nil) and (aDstSL <> nil) then
      begin
        i := 0;
        j := pred(cef_string_list_size(aSrcSL));

        while (i < j) do
          begin
            FillChar(TempString, SizeOf(TempString), 0);
            cef_string_list_value(aSrcSL, i, @TempString);
            aDstSL.Add(CefStringClearAndGet(TempString));
            inc(i);
          end;
      end;
  end;

After this fix, this code can probably be rewritten using a simpler for-loop.

/Hallvard

[salvadordf]

Thanks for the bug report!

I'll take a look as soon as I can.

[hvassbotn]

Very good!

The same problem applies to these three signatures, really:

Code: Select all

  cef_string_map_size   : function(map: TCefStringMap): NativeUInt; {$IFDEF CPUX64}stdcall{$ELSE}cdecl{$ENDIF};
  cef_string_multimap_size       : function(map: TCefStringMultimap): NativeUInt; {$IFDEF CPUX64}stdcall{$ELSE}cdecl{$ENDIF};
  cef_string_multimap_find_count : function(map: TCefStringMultimap; const key: PCefString): NativeUInt; {$IFDEF CPUX64}stdcall{$ELSE}cdecl{$ENDIF};

Here the function results should also be Integer rather than NativeUInt.

Current code in CEF4Delphi has other wrapper functions that call these and return Integer, but external code could call these directly and be affected in the same way as discussed above.

[salvadordf]

I just uploaded the fix for this bug. Thanks!

All those functions were correctly declared. This is one of them in /include/internal/cef_string_map.h :

Code: Select all

///
// Return the number of elements in the string map.
///
CEF_EXPORT size_t cef_string_map_size(cef_string_map_t map);

The returned value is a "size_t" which is an unsigned integer or NativeUInt;

The problem was in the "for" loops and the variable types used in those loops.

[hvassbotn]

Thanks for the quick fixing!

All those functions were correctly declared.

Well, strictly technically speaking, yes - they are unsigned integers on the C/C++ side.

Because for-loops work differently in C/C++ than i Delphi, it is not unusual to see size/count members that are unsigned rather than signed.

However, Delphi for-loops work differently in subtle ways (that allow the compiler to generate slightly smaller/faster code). That's why you'll see all implementations of Count, GetCount and similar methods always returning Integer and not some unsigned type such as Cardinal or NativeUInt.

So even though a strict translation of size_t is NativeUInt, a much more *useful* translation is Integer. The only thing you loose is the theoretical case of having more than MaxInt (2 billion) items.

That would have been a one-line change, instead of all the changes you had to do now.

Also, note that you have some functions that call the C APIs and returns the result as an integer.

So part of making an API Delphi friendly is making sure that count/size members and function results become signed - not usnigned.



I almost feel like writing another blog post here about this subtle issue

http://hallvards.blogspot.no/

Actually, the points I'm trying to make are also here:
https://stackoverflow.com/questions/797 ... ng-members

/H