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

No credit to Markus Kuhn #104

Closed
khanaffan opened this issue Mar 4, 2022 · 9 comments · Fixed by #105
Closed

No credit to Markus Kuhn #104

khanaffan opened this issue Mar 4, 2022 · 9 comments · Fixed by #105

Comments

@khanaffan
Copy link

khanaffan commented Mar 4, 2022

Hi,
I am trying to use this library and some one point out that part of this software is written by Dr Markus Kuhn. But he is not mention in the LICENSE.md. Can any one add him there as second contributor to software. I have to put two license notices in my software and it would be great if I have to just add one for this library.

Source: https://github.com/websockets/utf-8-validate/blob/master/src/validation.c
Author: Markus Kuhn http://www.cl.cam.ac.uk/~mgk25/

Thanks

@lpinca
Copy link
Member

lpinca commented Mar 6, 2022

@lpinca
Copy link
Member

lpinca commented Mar 6, 2022

IANAL but by reading https://www.cl.cam.ac.uk/~mgk25/short-license.html, it seems to be sufficient to license the code under any of the licenses listed there? It does not say anything about the copyright notice.

@lpinca
Copy link
Member

lpinca commented Mar 6, 2022

See #105. I do not think it is needed but if anyone can chime in, then that would be nice.

@khanaffan
Copy link
Author

khanaffan commented Mar 7, 2022

So I guess I have a special case, I have case where i have to statically link the .c file in here to node.js so it can run on mobile. So in my case no one can read the license from .c file and so i have to add license notices separately . Even npm package get webpacked so i copy their licenses as well. In this case i notice that technically there are two contributor but since .c get compiled no one can see the other contributor at least not directly unless they open this package git hub repo and lookup source code.

I locally add the other license notice. You can close this issue if you think it require no fix on your side.

By the way thanks for quick response.

@lpinca
Copy link
Member

lpinca commented Mar 7, 2022

So I guess I have a special case, I have case where i have to statically link the .c file in here to node.js so it can run on mobile.

Aren't Node.js addons dinamically loaded 🤔 ?

@khanaffan
Copy link
Author

khanaffan commented Mar 7, 2022

NodeJs is not supported on iOS/Android. We just made it compile and work but there is no concept of dynamic loading module as such on these OS. We have to statically link modules and the use e.g. process._linkbindings("validation") to get them. In this case I modified node.js to make this module accessible via require("utf-8-validation") . Without this and another bufferutil library we see 30% drop in performance of websockets on mobile devices. By the way the whole app including nodejs run in same process.

@lpinca
Copy link
Member

lpinca commented Mar 7, 2022

Ok, so the issue is that the module is used in a non standard way. In that case I think it is not a utf-8-validate issue. Anyway I have no problem merging #105 and cutting a new patch release if that makes your life easier.

@khanaffan
Copy link
Author

khanaffan commented Mar 8, 2022

Yes that change look good.

One unrelated note bug in .c files is that it does not check args type or number and so if i pass random argument or no argument it cause a crash. I would say it might be a security issue. You can fix it but if you like i can either send you code or create PR here. Also asserts are ignore in release and not useful in preventing crash.

For example following later crash if argv[0] is not supplied. The code before this point does not check if right number and type of argument was sent. You need to either return "undefined" or use napi to throw js exception.
buf and len are not initialize to null or zero and following napi function does not initialize it if argv[0] is not supplied or undefined.

status = napi_get_buffer_info(env, argv[0], (void **)&buf, &len);

which mean later code if some garbage value for len and buf and cause illegal memory access voilation.

Thanks

@lpinca
Copy link
Member

lpinca commented Mar 8, 2022

One unrelated note bug in .c files is that it does not check args type or number and so if i pass random argument or no argument it cause a crash. I would say it might be a security issue.

This is by design. Argument validation should be done in JavaScript, if needed, before calling the function.

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

Successfully merging a pull request may close this issue.

2 participants