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

curl_sasl.c PVS Studio warnings #745

Closed
bagder opened this Issue Mar 31, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@bagder
Member

bagder commented Mar 31, 2016

@alagoutte run PVS Studio on the curl code and we got two warnings on the curl_sasl.c code that I would like you to have a look at @captain-caveman2k:

V537 Consider reviewing the correctness of 'SASL_OAUTH2' item's usage. curl_sasl.c 332

That's puzzling to me, but I gave the code a gance and I couldn't immediately see what the tool actually refers to here! 😮

V595 The 'mech' pointer was utilized before it was verified against nullptr. Check lines: 376, 381. curl_sasl.c 376

To me, this looks like it could potentially be correct and possibly warrants an extra check in the code or so.

@alagoutte

This comment has been minimized.

Show comment
Hide comment
@alagoutte

alagoutte Mar 31, 2016

Contributor

It is possible it is a false postive... (specialy for the first case)

Contributor

alagoutte commented Mar 31, 2016

It is possible it is a false postive... (specialy for the first case)

@captain-caveman2k

This comment has been minimized.

Show comment
Hide comment
@captain-caveman2k

captain-caveman2k Mar 31, 2016

Member

@bagder - I believe the check at line 375/376 is relatively new whilst the code at 381 is a little older. Off the top of my head I think this check is designed to not send a response that may break the line length of the protocol that is using this code and may stem from line length limits in SMTP (??). This newer code is a result of when the authentication code was consolidated and moved out of IMAP, POP3 and SMTP. I would have to look at the log to be absolutely sure though.

That aside, I think PVS Studio is partly getting confused when no mechanism is chosen / matched against and "mech" will be NULL. Technically speaking "resp" will be NULL as well so this code cannot be executed.

When I have this type of thing with Visual Studio and uninitialized variables, for example, I tend to keep the compiler happy and initialise those variables so I would be tempted to try and fix the code and check that "mech" is a valid pointer before using it:

a) We can stick a check for "mech" in the if at line 375. However, we would end up with two checks for "mech" - see line 381.

b) We could move the block at line 375 into the "if(mech)" check but would end up with extra nesting.

c) Modify "if(!result)" to become "if(!result && mech) then the extra if(!mech) on line 381 can come out.

Member

captain-caveman2k commented Mar 31, 2016

@bagder - I believe the check at line 375/376 is relatively new whilst the code at 381 is a little older. Off the top of my head I think this check is designed to not send a response that may break the line length of the protocol that is using this code and may stem from line length limits in SMTP (??). This newer code is a result of when the authentication code was consolidated and moved out of IMAP, POP3 and SMTP. I would have to look at the log to be absolutely sure though.

That aside, I think PVS Studio is partly getting confused when no mechanism is chosen / matched against and "mech" will be NULL. Technically speaking "resp" will be NULL as well so this code cannot be executed.

When I have this type of thing with Visual Studio and uninitialized variables, for example, I tend to keep the compiler happy and initialise those variables so I would be tempted to try and fix the code and check that "mech" is a valid pointer before using it:

a) We can stick a check for "mech" in the if at line 375. However, we would end up with two checks for "mech" - see line 381.

b) We could move the block at line 375 into the "if(mech)" check but would end up with extra nesting.

c) Modify "if(!result)" to become "if(!result && mech) then the extra if(!mech) on line 381 can come out.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Mar 31, 2016

Member

The first warning seems to be based on the names according to docs so it is propably warning because the variables are named state1 and state2 but both assign OAUTH2 values (emphasis on the number 2 there). So let's disregard that one.

To me it seems like your suggestion (C) is the smallest fix that is still quite nice.

Member

bagder commented Mar 31, 2016

The first warning seems to be based on the names according to docs so it is propably warning because the variables are named state1 and state2 but both assign OAUTH2 values (emphasis on the number 2 there). So let's disregard that one.

To me it seems like your suggestion (C) is the smallest fix that is still quite nice.

captain-caveman2k added a commit that referenced this issue Apr 3, 2016

curl_sasl: Fixed potential null pointer utilisation
Although this should never happen due to the relationship between the
'mech' and 'resp' variables, and the way they are allocated together,
it does cause problems for code analysis tools:

V595 The 'mech' pointer was utilized before it was verified against
     nullptr. Check lines: 376, 381. curl_sasl.c 376

Bug: #745
Reported-by: Alexis La Goutte
@captain-caveman2k

This comment has been minimized.

Show comment
Hide comment
@captain-caveman2k

captain-caveman2k Apr 3, 2016

Member

Implemented option c) in commit e655ae0.

Member

captain-caveman2k commented Apr 3, 2016

Implemented option c) in commit e655ae0.

@lock lock bot locked as resolved and limited conversation to collaborators May 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.