Catch block causes TypeError in strict mode because it tries to add properties to a string

[alexvy86]

We're running into an error that we can't reproduce consistently but wanted to bring it up while we keep investigating how to get it to occur. It seems the code here (and it seems to be a pattern in many APIs) is sometimes receiving a plain string instead of an Error object and trying to add the caller property to it, which results in a TypeError when things are running in strict mode. This is all in a Node process, using Node 18.17.1.

TypeError: Cannot create property 'caller' on string 'buffer error'
    at Object.readTree (/workspaces/FluidFramework/server/tinylicious/node_modules/.pnpm/isomorphic-git@1.19.2/node_modules/isomorphic-git/index.cjs:12651:16)

I haven't identified where the "buffer error" is coming from, but it feels like independent of that, the error handling code should check if it's dealing with an object or a string.

[jcubic]

I don't see any 'buffer error' in the source code. Maybe the error came from Buffer itself or from other native library that is used by isomorphic-git.

It would be nice to see the error. What you can do is to modify the source code and add console.trace() before err.caller = 'git.readTree'.

I would add:

if (typeof err === 'string') {
  console.trace()
} else {
  err.caller = 'git.readTree'
}

And see when it will log the stack trace. You can also log different things. So find when this function is called.

[alexvy86]

Yeah, I looked for "buffer error" as well to see if it was coming from the library itself but couldn't find it. I'll make the suggested edit to the code to get more information next time this happens.

[alexvy86]

Didn't take long to see it again, but printing the trace did not provide any additional information:

Trace
    at Object.readTree (/workspaces/FluidFramework/server/tinylicious/node_modules/.pnpm/isomorphic-git@1.19.2/node_modules/isomorphic-git/index.cjs:12653:15)
    at async getTree (/workspaces/FluidFramework/server/tinylicious/dist/routes/storage/git/trees.js:77:28)

I now added a JSON.stringify(err) call just in case, but I still suspect err is a plain string with no extra properties.

[jcubic]

I was searching the source code of NodeJS if there is such an error but I was not able to find using the GitHub interface. I'm not cloning the repo to check locally.

Another idea that came into my mind is that you use the Buffer library somehow and that Buffer is throwing an error.

I will start a Discussion on NodeJS repo after I check the source code if there is such string.

[alexvy86]

We might be using Buffer somewhere in the project, yeah, but this error must be from a call made by isomorphic-git, right? When our code calls readTree() we're passing Node's fs module and two plain strings (dir and oid).

Can this catch be triggered by isomorphic-git calling our code in some way? Felt like from that function "down" it was all library code.

[jcubic]

I've looked at the NodeJS source code and the only place where there is 'buffer error' came from zlib/zutil.c. But I'm not sure when it's thrown. I've started the discussion: nodejs#50146

[jcubic]

What is the oid you're asking for?

[jcubic]

This is just a guess but this is the place where it use compression:

https://github.com/isomorphic-git/isomorphic-git/blob/main/src/utils/inflate.js

You can try to add try..catch to call to packo:

import { InternalError } from '../errors/InternalError.js'

export async function inflate(buffer) {
  if (supportsDecompressionStream === null) {
    supportsDecompressionStream = testDecompressionStream()
  }
  try {
  return supportsDecompressionStream
    ? browserInflate(buffer)
    : pako.inflate(buffer)
  } catch(e) {
    throw new InternalError(`Invalid compressed buffer`);
  }
}

and see if you will get this error instead. Maybe the real problem is that the git index is corrupted.

And do the same to:

https://github.com/isomorphic-git/isomorphic-git/blob/main/src/utils/deflate.js

[alexvy86]

Added the try-catches around inflate and deflate, I'll update with results once it happens again.

I still think as a matter of robustness it'd be good to have a check on the error object before trying to add properties (caller) to it. Other plain-string errors from Node or low-level libraries could sneak in at some point.

[alexvy86]

Yeah, with the extra try-catch around pako calls now that's the error I get, so the "buffer error" seems to be coming from there. Specifically pako.inflate.

InternalError: An internal error caused this command to fail. Please file a bug report at https://github.com/isomorphic-git/isomorphic-git/issues with this error message: Invalid compressed buffer
    at inflate (/workspaces/FluidFramework/server/tinylicious/node_modules/.pnpm/isomorphic-git@1.19.2/node_modules/isomorphic-git/index.cjs:2541:11)
    at _readObject (/workspaces/FluidFramework/server/tinylicious/node_modules/.pnpm/isomorphic-git@1.19.2/node_modules/isomorphic-git/index.cjs:3025:39)
    at async resolveTree (/workspaces/FluidFramework/server/tinylicious/node_modules/.pnpm/isomorphic-git@1.19.2/node_modules/isomorphic-git/index.cjs:3788:28)
    at async _readTree (/workspaces/FluidFramework/server/tinylicious/node_modules/.pnpm/isomorphic-git@1.19.2/node_modules/isomorphic-git/index.cjs:4868:34)
    at async Object.readTree (/workspaces/FluidFramework/server/tinylicious/node_modules/.pnpm/isomorphic-git@1.19.2/node_modules/isomorphic-git/index.cjs:12673:12)
    at async getTree (/workspaces/FluidFramework/server/tinylicious/dist/routes/storage/git/trees.js:77:28) {
  caller: 'git.readTree',
  code: 'InternalError',
  data: { message: 'Invalid compressed buffer' }
}

[jcubic]

That's progress. I think it will be a good idea to add this try catch and retrow internal error. Maybe the next step is to ask in at pako repo what may be the cause of this error. Maybe they need to do similar things as we did here.

[jcubic]

Also, do you have this project public? Or maybe you can create a reproduction. It would be easier to test this.

[alexvy86]

The project is public but reproducing the problem is very tricky. My ad-hoc setup right now is running a set of heavy tests in a loop until eventually they fail, because they don't do it consistently; and it's not particularly quick to set up. If I get some time I'll try to come up with a minimal repro, but to be honest I don't think I'm close to having one.

[jcubic]

If you can reproduce that would be great. Even if the code doesn't fail all the time it's better than nothing, but the code should be minimal if possible.

I will add the try..catch maybe it happens to other users as well. If the error they get will say to report the issue here they will gladly do this. Recently we had a lot of issues from Salesforce CLI because they didn't catch the error from isomorphic-git the error message said to report the issue and they did. I think that some of the users even created the account just to report the issue.

[jcubic]

Got a reply from NodeJs discussion. The error is probably thrown when the buffer is empty.