[Fixed] TDocVariantData.GetValueIndex, TDocVariantData.GetVarData

Eugene Ilyin · 2021-11-11 15:49:29

Hi, the recent 1.18.6318:

Check

program Test;
{$APPTYPE CONSOLE}
uses
  SynCommons;
var
  V: Variant;
  VD: TVarData;
begin
  with TDocVariantData(V) do
  begin
    InitJSON('{"":1}');
    Writeln(GetValueIndex('') = 0);
    Writeln(GetVarData('', VD, @StrComp));
    Writeln(AddValueFromText('','test') = 0);
  end;
  Readln;
end.

Expected

TRUE
TRUE
TRUE

Actual

Exception class $C0000005 with message 'access violation at 0x0052573a: read of address 0xfffffffc'. Process Test.exe (11556)

Issue

TDocVariantData.GetValueIndex problem place:

...
      result := FindNonVoidRawUTF8(pointer(VName),aName,aNameLen,VCount) else
      result := FindNonVoidRawUTF8I(pointer(VName),aName,aNameLen,VCount) else

[Actual]
  FindNonVoidRawUTF8:
    for result := 0 to count-1 do // all VName[]<>'' so n^<>0
      if (PStrLen(n^-_STRLEN)^=len) and CompareMemFixed(pointer(n^),name,len) then

  FindNonVoidRawUTF8I:
    if (PStrLen(n^-_STRLEN)^=len) and IdemPropNameUSameLen(pointer(n^),name,len) then

[Expected]
  FindNonVoidRawUTF8:
    for result := 0 to count-1 do
      if ((n^=0) and (len=0)) or
        ((PStrLen(n^-_STRLEN)^=len) and CompareMemFixed(pointer(n^),name,len)) then

  FindNonVoidRawUTF8I:
    if ((n^=0) and (len=0)) or
      ((n^<> 0) and (PStrLen(n^-_STRLEN)^=len) and IdemPropNameUSameLen(pointer(n^),name,len)) then

TDocVariantData.GetVarData problem place:

[Actual]
  if (integer(VType)<>DocVariantVType) or not(dvoIsObject in VOptions) or
     (VCount=0) or (aName='') then

[Expected]
  if (integer(VType)<>DocVariantVType) or not(dvoIsObject in VOptions) or
     (VCount=0) then

TDocVariantData.AddValueFromText problem place:

[Actual]
  function TDocVariantData.AddValueFromText(const aName,aValue: RawUTF8;
    Update, AllowVarDouble: boolean): integer;
  begin
    if aName='' then begin
      result := -1;
      exit;
    end;
  result := GetValueIndex(aName);

[Expected]
//  if aName='' then begin
//    result := -1;
//    exit;
//  end;
  result := GetValueIndex(aName);

Impact

Any request contained the empty name in JSON object will crush the thread request processing
Any TDocVariantData access: Exists(aName), NameIndex(aName), AddValue, AddValueFromText, SearchItemByProp, ReduceAsArray, Rename, Delete(aName), GetValueOrDefault, GetValueOrNull, GetValueOrEmpty, GetValueEnumerate, GetAsPVariant, RetrieveValueOrRaiseException, SetValueOrItem, AddOrUpdateValue, GetOrAddIndexByName, GetOrAddPVariantByName, GetPVariantByName, IntSet, DoFunction, Exists, ExistsOrLock, AddExistingPropOrLock, ...
Any JSON iterating, search, etc.

Context

Is empty name allowed for JSON object members?
Yes. Following the spec:

json
  element
elements
  element
  element ',' elements
element
  ws value ws
value
  object
object
  '{' members '}'
members
  member
  member ',' members
member
  ws string ws ':' element <-- String HERE for member name
string
  '"' characters '"' <-- Chars HERE for string value
characters
  "" <-- Empty content HERE for chars
  character characters

Coverage
No empty name tests provided in sanity checks.

P. S. Please check the other places where the Name prop assumed not to be empty (no sure that I found all the cases).

Last edited by Eugene Ilyin (2021-11-11 20:25:24)

ab · 2021-11-11 17:08:34

By design, we don't support void field names, even if the JSON specs allow it.
TDocVariantData.InternalAdd expects to be called with aName='' for arrays, so it would not allow to create void field names anyway.

So indeed there is a missing check for void name in TDocVariantData.GetValueIndex.
I have just fixed it by https://github.com/synopse/mORMot2/comm … 1df265067a and https://synopse.info/fossil/info/b914adc67d
It is the only place I have seen missing such a check - TDocVariantData.GetVarData has a proper check.

Eugene Ilyin · 2021-11-11 20:28:19

Hm, ok partial support of the JSON spec for this rare case is good enough.
Anything better than unexpected exception for incoming JSONs

Thanks, fix confirmed, now it's

FALSE
FALSE
FALSE

ab · 2021-11-11 20:46:16

Sorry for the issue.

It was affecting mORMot 1 and 2 BTW.

mORMot Open Source

#1 2021-11-11 15:49:29