Opened 5 months ago

Closed 8 weeks ago

#31147 closed defect (fixed)

Check tor_vasprintf for error return values.

Reported by: paldium Owned by:
Priority: Low Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version: Tor: 0.4.0.5
Severity: Normal Keywords: 042-can
Cc: Actual Points:
Parent ID: Points:
Reviewer: ahf Sponsor:

Description

In case of error, a negative value will be returned or NULL written into
first supplied argument.

This patch uses both cases to comply with style in the specific files.

A tor_vasprintf error in process_vprintf would lead to a NULL dereference
later on in buf_add, because the return value -1 casted to size_t would
pass an assertion check inside of buf_add.

On the other hand, common systems will fail on such an operation, so it
is not a huge difference to a simple assertion. Yet it is better to
properly fail instead of relying on such behaviour on all systems.

Child Tickets

Attachments (1)

0001-Check-tor_vasprintf-for-error-return-values.patch (2.5 KB) - added by paldium 5 months ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 5 months ago by nickm

Keywords: 035-backport 040-backport 041-backport added
Milestone: Tor: 0.4.2.x-final
Status: newneeds_review

comment:2 Changed 5 months ago by dgoulet

Reviewer: mikeperry

comment:3 Changed 3 months ago by nickm

Type: enhancementdefect

Calling this "defect" rather than "enhancement" since the problem is not checking the return value.

comment:4 Changed 3 months ago by teor

Reviewer: mikeperry

Mike is not doing reviews this month, please re-assign.

comment:5 Changed 3 months ago by nickm

Keywords: 042-can added

comment:6 Changed 3 months ago by asn

Reviewer: ahf

comment:7 Changed 2 months ago by ahf

I added a changes file and opened a PR on Github:

https://github.com/torproject/tor/pull/1417

Let us see what our CI says to this.

In the future it is generally a good idea to open the PR on Github directly, since this allows the CI to run upfront such that we can check it once the CI passes :-)

comment:8 Changed 2 months ago by ahf

Status: needs_reviewmerge_ready

Okay, I think the patch looks good.

To the merger: it seems like something went fishy with AppVeyor and Travis at first where it tried to pull a branch from torproject/tor instead of ahf/tor, which failed, but the second round seems to have passed. I have no idea why that is the case.

comment:9 Changed 2 months ago by nickm

Status: merge_readyneeds_revision

This LGTM but the ticket says it is for backporting, and the branch is based against master. Can I have a branch based on 035 if this is really for backport?

Alternatively we could say that this is not for backport, and I'd be okay with that too.

comment:10 Changed 2 months ago by ahf

Keywords: 035-backport 040-backport 041-backport removed
Status: needs_revisionneeds_review

I don't think this needs to be backported. I think the NULL dereference bugs would cause a crash at run-time if this had ever happened in the wild.

comment:11 Changed 8 weeks ago by ahf

Status: needs_reviewmerge_ready

comment:12 Changed 8 weeks ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged to master. Thanks, Paldium!

Note: See TracTickets for help on using tickets.