How to safely UnMarshal JSON objects

Question

When i try to unmarshall a JSON object that is not well formated, I expect a object reference from UnMarshall function, but it comes nil. But so, when I close my application that object generates memory leaks.

TMyObject = class
private
  FName: String;
end;

AJSON := TJSONObject.ParseJSONValue('{ type: "MyObject.TMyObject", id: 1, fields: { FName: "David", FAge: 20 } }');

//FAge attribute don't exists in TMyObject, so it raises an exception when unmarshalling


with TJSONUnMarshal.Create() do  
begin  
  try
    Result := Unmarshal( AJSON );
    //First chance exception at $77322F71. Exception class EConversionError with message 'Internal: Field FAge cannot be found in type TMyObject'. Process MyApp.exe (3056)
  finally
    Free();
  end;
  //Here the result is nil, but internally the object was created and is alive
end

function TJSONUnMarshal.Unmarshal(Data: TJSONValue): TObject;
  var
    Root: TJSONObject;
begin
  if not (Data is TJSONObject) then
    raise EConversionError.Create(SCannotCreateObject);

  // clear previous warnings
  ClearWarnings;
  Root := TJSONObject(Data);
  try
    Result := CreateObject(Root)
  finally
    FObjectHash.Clear;
  end;
end;

If the JSON object was not in expected format, it raises an exception but don't destroy object created reference and don't return it on function.

So someone that consumes my server can call some functions and nothing garanties the JSON sended into request is well formatted.

How can I handle this kind of situation? There's a way to validate the JSON object with the respective class?

ps: I'm using Delphi XE7

Which object is not destroyed. The code in the question appears to be fine. — David Heffernan, Sep 30 '14 at 16:56
The object created inside Unmarshal. The function creates the object and so raises an exception, but no result is taken, it comes nil and the object is alive. — Lucas Belo, Sep 30 '14 at 17:00
Passing nil as parameter is well treated in function, it's checked in "CreateObject(JsonObj: TJSONObject)" by "assert(JsonObj <> nil);" — Lucas Belo, Sep 30 '14 at 17:06
So it's a bug in the Emba code? I suppose you could use a hash to try to detect a corrupted data stream? But is that why the unmarshalling fails? Is it a corruption, or an error of a different nature. — David Heffernan, Sep 30 '14 at 17:37
How have you diagnosed that there is a memory leak? Using fastmm? — David Heffernan, Sep 30 '14 at 17:55
I followed the process and confirmed by myself. Root := TJSONObject(Data); try Result := CreateObject(Root) finally FObjectHash.Clear; end; This is what happend inside UnMarshal(). FObjectHash is a map that's contains a reference seted by "StoreObject(ObjId, Obj);" inside "CreateObject()". Before that "Obj := ObjectInstance(FRTTICtx, objType);" the object is created and so is stored. But as you can see in the code above, the list is cleared and so objects it's no destroyed, just not referenced in list anymore. — Lucas Belo, Sep 30 '14 at 18:03
I see that the unmarshaled object is returned after all fields is populated and so the exit code is called as so "exit(Obj)", but when it raises exception when is populating fields the exit(Obj) is never called and the result is not setted so it returns nil. — Lucas Belo, Sep 30 '14 at 18:05
No. Nothing is returned from a function that raises an exception. — David Heffernan, Sep 30 '14 at 18:08
Right, it should not return, but it should not keep the instance alive too. I'm posting the details there... — Lucas Belo, Sep 30 '14 at 18:19
That's not valid JSON anyway, regardless of what kind of Delphi object it's supposed to represent. Object attribute names must be quoted in JSON. Consider switching to a more compliant parser — Rob Kennedy, Sep 30 '14 at 18:19
CreateObject(Root) is really big... if you check on you delphi you should be able to undestand it... — Lucas Belo, Sep 30 '14 at 18:28
The point is: the JSON is not being generated by datasnap proxy client, it could be manually created. So I need to avoid errors possibilities. — Lucas Belo, Sep 30 '14 at 18:29
@Lucas I don't have the source to this class. Must be in a different edition. It needs to have the try/except that you see in my answer. Does it? — David Heffernan, Sep 30 '14 at 18:44
Yes, it's missing the try/excpet block. Let me check if in another version it exists... — Lucas Belo, Sep 30 '14 at 18:51
In Delphi XE6 tha same thing happens. Can anyone see in previous versions(XE5, XE4, XE3...)? — Lucas Belo, Sep 30 '14 at 20:29

score 0 · Answer 1 · answered Sep 30 '14 at 17:42

0

The thing is your with (maybe) use, if you create an Unmarshal object and treat all the objects in separate:

AJSON := TJSONObject.ParseJSONValue('{ type: "MyObject.TMyObject", id: 1, fields: { FName: "David", FAge: 20 } }');

try
  if Assigned(AJSON) then
    UnmarshalThisObject(AJSON);  
finally
  FreeAndNil(AJSON);
end;

procedure UnmarshalThisObject(AJSON: TJSONObject);
var
  oUnMarshalObject: TJSONUnMarshal;
  oUnMarshalResult: TUnmarshalType {dont know};
begin
  if Assigned(AJSON) then
  begin
    oUnMarshalObject := TJSONUnMarshal.Create();
    try
      oUnMarshalResult := oUnMarshalObject.Unmarshal(AJSON);
    finally
      FreeAndNil(oUnMarshalResult);
      FreeAndNil(oUnMarshalObject);
    end;
  end;
end;

Hope it helps.

answered Sep 30 '14 at 17:42

oPsDCadarn

106
3

If `oUnMarshalObject.Unmarshal()` raises an exception, then `oUnMarshalResult` is never assigned. And so the code here is identical in meaning to the code in the question. -1 – David Heffernan Sep 30 '14 at 17:49
Don't work. The reference that's no being destroyed is not a TJSONUnMarshal, is unmarshaled object. – Lucas Belo Sep 30 '14 at 17:56
David, yes, should be an except there, but the ideia is if the OVNI object is created under TJSONUnMarshal responsibility, destroying it (TJSONUnMarshal) could free it (the unmarshaled object). – oPsDCadarn Sep 30 '14 at 18:29
Please read my comment again. The code here performs in exactly the same way as the code in the question. Actually that's not true. The code here calls free on an uninitialized variable, `oUnMarshalResult`. When `Unmarshal` raises, which is precisely what we are talking about, `oUnMarshalResult` is never assigned. Calling `Free` on it will likely result in another exception. So you'll end up leaking `oUnMarshalObject` too! – David Heffernan Sep 30 '14 at 18:38
@oPsDCararn TJSONUnMarshal don't owns the marshaled object – Lucas Belo Sep 30 '14 at 20:25

score 0 · Answer 2 · answered Sep 30 '14 at 18:19

A function like Unmarshal that returns a newly created object must be implemented like this:

function CreateObj: TObject;
begin
  Result := TObject.Create;
  try
    // do stuff with Result
  except
    Result.Free;
    raise;
  end;
end;

If there is no such try/except block in Unmarshal, and I cannot see the code to check, then it will leak whenever it raises.

There's not much that you can do from the outside. A bug like this cannot be fixed readily from the outside. Once the object is leaked, you cannot get hold of it to destroy it.

You should certainly submit a QC report. In the short term you may need to modify the Unmarshal code, and re-compile it, to add the missing try/except block

I suppose that the other way to deal with the problem is to stop feeding this function input that lead to exceptions.

If the exceptions are due to transmission failure, hash the transmitted data and check the hash on receipt to mitigate the problem.
If the exceptions are due to version incompatibilities between client and server, tighten up your version checks.
If the exceptions are due to mundane programming errors, fix the errors.

How to safely UnMarshal JSON objects

2 Answers2