Azure / azure-functions-host

The host/runtime that powers Azure Functions
https://functions.azure.com
MIT License
1.93k stars 441 forks source link

JavaScript function response objects containing functions can crash the runtime #1755

Open vjrantal opened 7 years ago

vjrantal commented 7 years ago

Repro steps

Provide the steps required to reproduce the problem:

  1. Create a Function App
  2. Set deployment from Git
  3. Deploy from https://github.com/vjrantal/azure-functions-mutual-tls/tree/crash (notice to use the crash branch)
  4. Issue an HTTP request to the function to trigger it

Expected behavior

The JSON representation of the provided response object gets returned to the HTTP request.

Actual behavior

The response is an internal server error and logs show the following error:

Newtonsoft.Json.JsonSerializationException : Error getting value from 'Func' on 'NodejsFunc'. ---> System.InvalidCastException : Unable to cast object of type '' to type 'Nan.Persistent<v8::Function,v8::NonCopyablePersistentTraits<v8::Function> >*'.
   at GetFunc(Object )
   at Newtonsoft.Json.Serialization.DynamicValueProvider.GetValue(Object target) 
   End of inner exception

Known workarounds

One workaround is to stringify the object already in the JavaScript side before handing it over to the runtime. This can be performed with the following diff to the code pointed in the reproduce steps:

   if (certHeader) {
     var parsed = sshpk.parseCertificate(Buffer.from(certHeader, 'base64'), 'x509');
-    context.res = parsed;
+    context.res = JSON.stringify(parsed);
mamaso commented 7 years ago

Investigated - the result is due to functions returned on the binding, which get exposed from edge and newtonsoft barfs on. parseCertificate returns an object with functions like toBuffer, etc.

related to: https://github.com/Azure/azure-webjobs-sdk-script/issues/1292 https://github.com/Azure/azure-webjobs-sdk-script/pull/1293/files

The reason is this code follows the CreateNegotiatedResponse codepath instead of CreateResultContent, and it appears that the previous work missed that.

One workaround would be the following, which will take the CreateResultContent branch and appropriately strip functions:

var parsed = sshpk.parseCert(...)
context.res = {
  header: { content-type: 'application/json' },
  body: parsed
}
tony-gutierrez commented 7 years ago

Getting a similar error when doing something like this:

/*
errors is an array of classes. 
errors: 
   [ ValidationError {
       property: 'instance.senderRole',
       message: 'is not one of enum values: CA,FA,FO,GA,OD,PL,TO',
       schema: [Object],
       instance: 'FAT',
       name: 'enum',
       argument: [Object],
       stack: 'instance.senderRole is not one of enum values: CA,FA,FO,GA,OD,PL,TO' } ],
*/

context.res.body = errors;

results in:

2017-09-05T19:19:32.838 Function completed (Success, Id=c1119611-7b4a-43ce-9c69-2dc6859b4b3b, Duration=4933ms)
2017-09-05T19:19:32.838 Executed 'Functions.az_acknowledge_message' (Succeeded, Id=c1119611-7b4a-43ce-9c69-2dc6859b4b3b)
2017-09-05T19:19:32.884 {"id":"0cf31dff-5f2d-4b55-adba-8e6518323515","requestId":"02e7629b-8d28-4dda-89b4-23f3430f5dde","statusCode":500,"errorCode":0,"message":"An error has occurred. For more information, please check the logs for error ID 0cf31dff-5f2d-4b55-adba-8e6518323515"}
2017-09-05T19:19:32.884 Newtonsoft.Json.JsonSerializationException : Error getting value from 'Func' on 'NodejsFunc'. ---> System.InvalidCastException : Unable to cast object of type '' to type 'Nan.Persistent<v8::Function,v8::NonCopyablePersistentTraits<v8::Function> >*'.

Of course it works fine in the express wrapper we use for local dev of azure funcs.

mamaso commented 7 years ago

@tony-gutierrez, as a workaround can you strip the functions from the returned classes / convert to data only objects?

tony-gutierrez commented 7 years ago

Yeah, we have. Just chiming in here as it wasted a lot of time. Also it is very concerning that it crashes the entire app service, and not just one function.

tony-gutierrez commented 7 years ago

@mamaso Has anyone reported this to whoever maintains Azure functions?

ahmelsayed commented 7 years ago

@mamaso is on the Azure Functions team 😄

christopheranderson commented 6 years ago

Burke and Simona (CDAs) got hit with this recently. It's probably not a breaking change to strip functions from objects on return since it would normally crash the process.

christopheranderson commented 6 years ago

So this doesn't exist in v2 because we try to JSON.stringify everything before it goes over the wire and then try to parse it to JSON again on the other end.

In v1, we only attempt our automagic JSON if you return an expando object. One thing to do would be to catch exceptions for that automatic expando and at least return a helpful error.

From there we can either leave it up to the user to fix it, or we could potentially attempt to strip the bad items ourselves, but that's maybe risking breaking changes or just too much work.

burkeholland commented 6 years ago

In our case, the choke was because the object contained a non-legit type thanks to Mongo. See "_id" field here...

{
    _id: 59ef719092e0a714ef8c28ac,
    id: 3,
    name: 'The Tick',
    saying: 'Spoooooooon!',
    __v: 0
}

A helpful error would great. Also, the time that we have to wait for this error also seems a tad on the long side - almost like a timeout for the 502. So we end up waiting a long time for the function to fail.

Additionally, the CLI does NOT choke on the _id field so "works on my machine".

luigimannoni commented 6 years ago

Seems after almost 2 days I found the culprit of my random 500s/502s.

Apparently on v1 even using JSON.stringify() on objects with no conventional type (like mongodb responses or docs) crashes badly everything, context.log is not much of a help and neither the runtime provide a bare explanation to it. Would be really helpful to have something along Unknown Object TypeError printed on console instead of crashing with a segfault

tony-gutierrez commented 6 years ago

This happens on any class based object passed to the response.

burkeholland commented 6 years ago

@luigimannoni My issue looks resolved on the v2 runtime. Have you tested V2 by chance?

luigimannoni commented 6 years ago

@tony-gutierrez yep pretty much, even if I generate one by myself and not necessarily using the class object from mongodb the Azure cli still kills itself with a segfault error. Is this something related to node? 🤔 What is even odd is that sometimes the function still keeps running and returns data as expected and sometimes it crashes in a quite random fashion.

@burkeholland unfortunately can't change to v2 at the moment as we are sticking to v1 for production.

At the end after reading this issue I just resolved by passing to the function another object with the bare minimum data I needed instead of passing the entire object and cleaning it on the function itself (I am using an external require to execute queries on mongo and pass the results to the azure function)