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

https://github.com/WasmEdge/WasmEdge/pull/2425 #4

Open
juntao opened this issue Apr 22, 2023 · 2 comments
Open

https://github.com/WasmEdge/WasmEdge/pull/2425 #4

juntao opened this issue Apr 22, 2023 · 2 comments

Comments

@juntao
Copy link
Contributor

juntao commented Apr 22, 2023

No description provided.

Copy link
Contributor Author

juntao commented Apr 22, 2023

Hello, I am a serverless review bot on flows.network. Here are my reviews of changed source code files in this PR.


plugins/wasmedge_httpsreq/httpsreqfunc.cpp

The code appears functional but has some potential issues that can be improved.

  1. Error handling
    While error handling is done in some cases, such as checking for null pointers and handling failed SSL functions, there are other parts where error handling can be improved. For instance, when calling socket(), connect(), SSL_write(), and SSL_read(), the return values should be checked for errors and appropriate messages should be logged or propagated.

  2. Resource leaks
    In cases of unhandled errors, resources allocated to 'Addr' may not be freed, leading to memory leaks. The code should ensure freeaddrinfo(Addrs) is called in all cases, including on error conditions.

  3. Missing include files
    Some functions such as spdlog::error(), getaddrinfo(), and ERR_print_errors_fp() may not have been declared or included in the provided code snippet. Ensure you have the required include files for these functions.

  4. Buffer size
    Currently, the buffer size for SSL_read() is hardcoded to 1024. It would be better to declare this as a constant or allow the user to provide a buffer size.

  5. Namespace
    It would be useful to add comments about the purpose of the WasmEdge and Host namespaces.

  6. Function naming
    Function naming can be inconsistent. For example, WasmEdgeHttpsReqGetRcvLen is using camel case, while other functions like WasmEdgeHttpsReqSendData are not. Consider adhering to a consistent naming convention.

Overall, the code appears functional but can benefit from improved error handling, cleanup, and consistency.

The patch introduces the following key changes:

  1. Addition of an include statement for the openssl/tls1.h header file.

    #include <openssl/tls1.h>
    
  2. Addition of a call to SSL_set_tlsext_host_name() function before the SSL_connect() call.

    SSL_set_tlsext_host_name(Ssl, Host);
    
  3. Improved error reporting when SSL_connect() fails by introducing a variable 'Code' to store the returned error code from SSL_get_error() function, and changing the logging message to log the error code.

    const int Code = SSL_get_error(Ssl, Status);
    ERR_print_errors_fp(stderr);
    spdlog::error("[WasmEdge Httpsreq] SSL_get_error code {}", Code);
    

cc WasmEdge/WasmEdge#2425

@juntao
Copy link
Contributor Author

juntao commented Apr 22, 2023

Flows summarize

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

1 participant