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

resolve a possible null pointer derefence found by cppcheck #28

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chipitsine
Copy link

[src/otp.c:366] -> [src/otp.c:357]: (warning) Either the condition 'if(vpn_username&&!strcmp(vpn_username,user_entry.name)&&vpn_secret&&password&&!strcmp(vpn_secret,password))' is redundant or there is possible null pointer dereference: vpn_username.

[src/otp.c:366] -> [src/otp.c:357]: (warning) Either the condition 'if(vpn_username&&!strcmp(vpn_username,user_entry.name)&&vpn_secret&&password&&!strcmp(vpn_secret,password))' is redundant or there is possible null pointer dereference: vpn_username.
@evgeny-gridasov
Copy link
Owner

vpn_username comes from openvpn_plugin_func_v1(...):

 const char *username = get_env ("username", envp);
 const char *password = get_env ("password", envp);

I think when openvpn plugin intercepts OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY, the username and password will be set in the environment *envp[].

That check is indeed redundant, and I'd rather fail early in the openvpn_plugin_func_v1(...) function instead and return OPENVPN_PLUGIN_FUNC_ERROR if username or password ==NULL.

@chipitsine
Copy link
Author

@evgeny-gridasov
Copy link
Owner

Yes, I did mean that. Will add extra check soon to be safe. But I'm pretty sure there is a promise from openvpn to set those variables in environment.

@chipitsine
Copy link
Author

ok, so you'll remove reduntant check by yourself, right ?

@evgeny-gridasov
Copy link
Owner

Yes.

@k0ste
Copy link
Contributor

k0ste commented Mar 19, 2019

@chipitsine, resolved via 5fa5582?

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 this pull request may close these issues.

3 participants