532108 - Null pointer dereference or redundant check in mailnews/mime/src/mimei.cpp.

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Adam Buchbinder]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.5) Gecko/20091109 Ubuntu/9.10 (karmic) Firefox/3.5.5
Build Identifier: thunderbird-3.0rc1

cppcheck, a static code analysis tool, reported:

[comm-1.9.1/mailnews/mime/src/mimei.cpp:1502]: (error) Possible null pointer dereference: options - otherwise it is redundant to check if options is null at line 1505

Reproducible: Always




In mime_parse_url_options(), the 'options' argument is a pointer. It's used by calling "options->headers" before being checked with "if (!options)". If it's expected that options could be null, the assignment to default_headers should be moved below the check; if it's can never be null, the check is redundant and can be removed.

Either way, the test as it stands does nothing; if 'options' is null, it will simply lead to a null pointer reference and subsequent crash.

I'm marking this as minor, because I haven't been able to generate an actual crash. Please advise if I should mark this sort of issue with a different severity.

Comment 1

[Adam Buchbinder]

Attachment: Patch to move the assignment below the check.

I've used the simplest change; the assignment now occurs after the check, so it's safe against 'options' being null.

Comment 2

[David :Bienvenu]

Comment on attachment 419718 [details] [diff] [review]
Patch to move the assignment below the check.

thx for the patch, Adam - mozilla style would be to move the declaration to be with the assignment, instead of splitting them up. r=me with that change.

Comment 3

[Adam Buchbinder]

Attachment: Move the assignment below the check [Checkin: Comment 5 & 7]

Edited the patch so that the assignment is together with the declaration, per Mozilla style.

Comment 4

[David :Bienvenu]

Comment on attachment 419902 [details] [diff] [review]
Move the assignment below the check
[Checkin: Comment 5 & 7]

thx, Adam.

Comment 5

[Serge Gautherie (:sgautherie)]

Comment on attachment 419902 [details] [diff] [review]
Move the assignment below the check
[Checkin: Comment 5 & 7]


http://hg.mozilla.org/comm-central/rev/3ed8bb7b219b

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 419902 [details] [diff] [review]
Move the assignment below the check
[Checkin: Comment 5 & 7]

We're not seeing any crashers in this area currently. We've already got a significant amount for 3.0.1 so I don't want to add this as well even thought it may be small.

Comment 7

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/releases/comm-1.9.1/rev/b3812420fcab