Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unnecessary register of uncaughtException handlers #4

Open
krystianolech opened this issue Sep 28, 2020 · 7 comments
Open

Unnecessary register of uncaughtException handlers #4

krystianolech opened this issue Sep 28, 2020 · 7 comments

Comments

@krystianolech
Copy link

At the beginning, I will mention that I'm aware that the main source of the issue is node-xmllint however it looks that node-xmllint is not maintained anymore, so any fix there would require creating a fork or merging its code directly here.

I noticed in my application that handle SAML that I got node warnings:
(node:1) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 uncaughtException listeners added to [process]. Use emitter.setMaxListeners() to increase limit

After deeper investigations, I found that uncaughtException listeners are registered by node-xmllint called by samlify-node-xmllint.
node-xmllint has been made as a conversion of libxml2 using Emscripten. Emscripten contains a feature that it auto-register these handlers for each compilation. In the newer version, it is possible to disable it:
emscripten-core/emscripten#7855

Why is this problematic?

  • Library produce undocumented side effect
  • As a registered handler is terminating the application, it might block other custom handlers from running (for example user might want to connect the logger to this event before closing the app, which might not be called due to handlers registered by node-xmllint)
  • Handler is registered with each reference to this library which might lead to memory leaks.
@netmiller
Copy link

netmiller commented Oct 5, 2021

I'm trying to figure why my app memory usage is growing slowly but quite linear . This is Node-Express app transpiled via TypeScript. I made a shell-script for recording mem usage (via pm2) :

2021-10-05 09:20:01 ==>  117.5mb
2021-10-05 09:30:01 ==>  117.7mb
2021-10-05 09:40:01 ==>  117.8mb
2021-10-05 09:50:01 ==>  116.7mb
2021-10-05 10:00:01 ==>  130.0mb
2021-10-05 10:10:01 ==>  148.1mb
2021-10-05 10:20:01 ==>  147.7mb
2021-10-05 10:25:01 ==>  158.9mb
2021-10-05 10:30:01 ==>  156.1mb
2021-10-05 10:35:01 ==>  164.2mb

There was several authentication tasks while this test, and one to suspect specially is :

node-xmllint : Handler is registered with each reference to this library which might lead to memory leaks.

Does anyone know how can I ensure this happening, because there are also other possibilities for growing mem usage.

@netmiller
Copy link

Think it's fixed now.

I had set schemaValidator to :
const validator = require('@authenio/samlify-validate-with-xmllint');

and now removed it totally, and skipped whole validation by :

samlify.setSchemaValidator({
    validate: (response: string) => {
        return Promise.resolve('skipped');
    }
});

Mem usage seems to be ok now, and linear growing has disappered.

Could you add some warning to docs, maybe Deprecated, to @authenio/samlify-validate-with-xmllint.
I bet it can harm similar Express apps like this, if used.

@fatton139
Copy link

@netmiller Does this mean you're not doing any XML validation at all?

@netmiller
Copy link

True. My only IdP is national security authority, and I can suppose the XML they provide is valid and checked properly.

@mariuszMWMW
Copy link

I also encountered the issue very randomly - running HAPI web server on Node. Sometimes when logging has a big payload, on drain of stdout event node-xmllint quits the process like nothing happened, no exception!!!
Workaround with turning off validation works, but would appreciate a long term fix.

@ivarconr
Copy link

I took the job of forking this library and in-lined "node-xmllint" (orginal repo is removed). I manually patched the memory leak in node-xmllint.

This is the result, currently validating in a real application:
https://www.npmjs.com/package/samlify-validator-js

@ivarconr
Copy link

ivarconr commented Feb 2, 2023

Update; we have been running the above version in production across all our customers integrating with various SAML 2.0 providers (Google Workspace, Okta, Azure AD, and more). We have not observed any memory leak after switching to https://www.npmjs.com/package/samlify-validator-js (github)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants