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

I checked curl with linux version of PVS-Studio #4374

Closed
ValZapod opened this issue Sep 18, 2019 · 14 comments

Comments

@ValZapod
Copy link

commented Sep 18, 2019

@bagder

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

That's like a lint output from the old days. Spewing out hundreds of mostly useless warnings and "informational texts" sprinkled with false positives.

Can you point to some of these that you think are actually valid or reason for concern?

@jay

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

I looked through some I'm not seeing anything valid. The V547 are almost always because we use macros that depending on preprocessor settings may evaluate either always true or always false, for example
#define Curl_convert_clone(a,b,c,d) ((void)a, CURLE_OK)

Some others I don't understand like this one for example
/opt/SourceCode/curl/lib/hostip.c 786 warn V1020 The function exited without calling the 'Curl_share_unlock' function. Check lines: 786, 782.

Ok but

  if(data && data->share)
    Curl_share_lock(data, CURL_LOCK_DATA_DNS, CURL_LOCK_ACCESS_SINGLE);

  freednsentry(dns);

  if(data && data->share)
    Curl_share_unlock(data, CURL_LOCK_DATA_DNS);

I have no doubt there's something valid in the log (we all make mistakes) but this has got to be 99% noise, it's too sensitive.

@ValZapod

This comment has been minimized.

Copy link
Author

commented Sep 18, 2019

@jay Someone of you have said that you need linux version of Pvs-studio log, so here you are... I can just help giving more precise (only level 1 and 2 and not 3) list...

And there are problems, just look into my other issues in other projects... I am just so tied of this and when I saw that you use if without a space I said f*** it. Really why not run clang format?? Do you really like it?
projectonly12level.txt

@bagder

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

If you want to discuss code style and format, or tools to handle that, I'd suggest you take that to the curl-library mailing list.

We scan the code very regularly with quality static code analyzers with far fewer false positives than in these lists. I thank you for them, but I've not spotted anything in there to act on.

@ValZapod

This comment has been minimized.

Copy link
Author

commented Sep 18, 2019

@bagder That is the point, I do not want to discuss it. Ookay.

GetStr(&config->login_options, nextarg);

Here see the same on line 1661

if(!result && block) {

Here result was already checked...
FILE *file = fopen(per->outfile, config->resume_from?"ab":"wb");

Here. config->resume_from is already checked on line 976

@ValZapod

This comment has been minimized.

Copy link
Author

commented Sep 18, 2019

@bagder

if((relurl[0] == '/') && (relurl[1] == '/')) {

Already checked on line 298... I am tied of this... Just look into new attached list... These were one after another. And by the way I did not check all of the code base because some parts like http3 does not configure...

@bagder

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

This is useful, thanks!

bagder added a commit that referenced this issue Sep 19, 2019
Reported-by: Valerii Zapodovnikov
Ref: #4374
@bagder

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

This second list contains 86 issues. Most of them cosmetic and harmless.

I claim 61 of them are false positives, mostly due to conditional compiles and "moving pieces".
I left two as-is, to not stir the pot.
I've addressed the remaining 23 in #4378 as 22 commits as two of the warnings were fixed by the same change.

At least one commit fixed a real bug, for CURLOPT_RTSP_SERVER_CSEQ.

@ValZapod

This comment has been minimized.

Copy link
Author

commented Sep 19, 2019

@bagder And again not all codebase was checked... You can tell what you want me to include like http3, or ldap s**** or smth. else. But please can you at least tell me what I should install (I have debian testing) I will still recheck the project after you will pull.

@pauldreik

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2019

Good that some real issues could be find and fixed, nice work!

While speaking of static analysis, I tested running the built in Visual Studio code analyzer. It also warned on

multi->multiplexing = CURLPIPE_MULTIPLEX;
which was already fixed. But none of the other warnings it gave seemed interesting at a quick glance.

@bagder

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

can you at least tell me what I should install (I have debian testing) I will still recheck the project after you will pull

Well the more things you add to the build to test, the better I guess. HTTP/3 might be a little premature to test for this now, but you could at least enable HTTP/2, alt-svc and brotli. Optionally also try a build with libssh instead of libssh2 to check that code too.

@bagder bagder added the tidy-up label Sep 19, 2019
@bagder bagder closed this in 69ea985 Sep 20, 2019
bagder added a commit that referenced this issue Sep 20, 2019
Fixes warning detected by PVS-Studio
Fixes #4374
bagder added a commit that referenced this issue Sep 20, 2019
Fixes warning detected by PVS-Studio
Fixes #4374
bagder added a commit that referenced this issue Sep 20, 2019
Fixes warning detected by PVS-Studio
Fixes #4374
bagder added a commit that referenced this issue Sep 20, 2019
... both !result and (ftp->transfer != FTPTRANSFER_BODY)!

Fixes warning detected by PVS-Studio
Fixes #4374
bagder added a commit that referenced this issue Sep 20, 2019
Fixes warning detected by PVS-Studio
Fixes #4374
bagder added a commit that referenced this issue Sep 20, 2019
Fixes warning detected by PVS-Studio
Fixes #4374
bagder added a commit that referenced this issue Sep 20, 2019
Fixes warning detected by PVS-Studio
Fixes #4374
bagder added a commit that referenced this issue Sep 20, 2019
Fixes warning detected by PVS-Studio
Fixes #4374
bagder added a commit that referenced this issue Sep 20, 2019
Fixes warning detected by PVS-Studio
Fixes #4374
bagder added a commit that referenced this issue Sep 20, 2019
Fixes warning detected by PVS-Studio
Fixes #4374
bagder added a commit that referenced this issue Sep 20, 2019
Fixes warning detected by PVS-Studio
Fixes #4374
bagder added a commit that referenced this issue Sep 20, 2019
Fixes warning detected by PVS-Studio
Fixes #4374
bagder added a commit that referenced this issue Sep 20, 2019
Fixes warning detected by PVS-Studio
Fixes #4374
bagder added a commit that referenced this issue Sep 20, 2019
Fixes bug detected by PVS-Studio
Fixes #4374
bagder added a commit that referenced this issue Sep 20, 2019
…'/')

Fixes warning detected by PVS-Studio
Fixes #4374
bagder added a commit that referenced this issue Sep 20, 2019
Fixes warning detected by PVS-Studio
Fixes #4374
bagder added a commit that referenced this issue Sep 20, 2019
Fixes warning detected by PVS-Studio
Fixes #4374
bagder added a commit that referenced this issue Sep 20, 2019
Fixes warning detected by PVS-Studio
Fixes #4374
bagder added a commit that referenced this issue Sep 20, 2019
Fixes warning detected by PVS-Studio
Fixes #4374
bagder added a commit that referenced this issue Sep 20, 2019
Fixes warning detected by PVS-Studio
Fixes #4374
bagder added a commit that referenced this issue Sep 20, 2019
Fixes warning detected by PVS-Studio
Fixes #4374
@ValZapod

This comment has been minimized.

Copy link
Author

commented Sep 20, 2019

@bagder What about 3rd comment about Curl_share_unlock?
So here are new warnings to libssh:

lib/vauth/vauth.h	46	note	V1003 The macro 'GSS_ERROR' is a dangerous expression. The parameter 'status' must be surrounded by parentheses.
lib/vauth/ntlm.c	632	warn	V614 Potentially uninitialized buffer 'ntbuffer' used. Consider checking the first actual argument of the 'Curl_ntlm_core_lm_resp' function.
lib/vauth/ntlm.c	841	warn	V547 Expression 'result' is always false.
lib/vtls/openssl.c	646	note	V575 The potential null pointer is passed into 'SSL_CTX_use_certificate_chain_file' function. Inspect the second argument.
curl/lib/vtls/openssl.c	856	note	V575 The potential null pointer is passed into 'SSL_CTX_use_PrivateKey_file' function. Inspect the second argument.
lib/vtls/openssl.c	1670	warn	V547 Expression 'rc' is always false.
lib/vtls/openssl.c	3732	warn	V560 A part of conditional expression is always true.
lib/vquic/quiche.c	53	note	V1003 The macro 'QUIC_IDLE_TIMEOUT' is a dangerous expression. The expression must be surrounded by parentheses.
lib/vssh/libssh.c	339	note	V526 The 'memcmp' function returns 0 if corresponding buffers are equal. Consider examining the condition for mistakes.
lib/vssh/libssh.c	496	err	V590 Consider inspecting the 'nprompts == - 1 || nprompts != 1' expression. The expression is excessive or contains a misprint.
lib/vssh/libssh.c	1359	warn	V560 A part of conditional expression is always true: sshc->readdir_attrs == NULL.
lib/vssh/libssh.c	2002	warn	V560 A part of conditional expression is always true: !result.

And here to http2. brotli was activated before as well as --enable-alt-svc...

lib/http2.c	2053	note	V658 A value is being subtracted from the unsigned variable. This can result in an overflow. In such a case, the '>' comparison operation can potentially behave unexpectedly.
lib/http2.c	2140	warn	V547 Expression 'stream->stream_id != - 1' is always true.
lib/setopt.c	2658	warn	V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!dep' and 'dep'.

Unfortunately debian does not have libngtcp2 or quic.

@bagder

This comment has been minimized.

Copy link
Member

commented Sep 23, 2019

His comment agrees with my view of these warnings. Since you asked me after his comment I figured you had a different view.

@ValZapod

This comment has been minimized.

Copy link
Author

commented Sep 23, 2019

@bagder Then there is no problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.