#26007 closed defect (fixed)

Stop logging stack contents when reading a zero-length bandwidth file

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version: Tor: 0.2.2.1-alpha
Severity: Normal Keywords: 033-must-maybe security-low 032-backport 031-backport 029-backport fast-fix
Cc: juga Actual Points:
Parent ID: #25925 Points:
Reviewer: asn Sponsor:

Description

When reading a zero-length bandwidth file, Tor directory authorities read an uninitialised buffer and log it at warning level.

There is no guarantee that the buffer contains a NUL byte.

Child Tickets

Change History (14)

comment:1 Changed 10 months ago by teor

Status: assignedneeds_review

Please see my branch bug26007_029 on https://github.com/teor2345/tor.git

comment:2 Changed 10 months ago by teor

Oops, now it's force-pushed and based on 0.2.9.

comment:3 Changed 10 months ago by nickm

Status: needs_reviewmerge_ready

lgtm, merging to 0.2.9 and forward

Can we have a regression test for this case?

comment:4 Changed 10 months ago by teor

I think juga is working on a whole bunch of bwfile tests.
Let's make sure one of them is a regression test for this case.

comment:5 Changed 10 months ago by juga

Cc: juga added

comment:6 Changed 10 months ago by teor

Parent ID: #25925

comment:7 Changed 10 months ago by juga

Test only for this in my branch ticket26007_01.
It's using the same approach as the test for geoip, which adds a new file to the filestystem.
Nickm, would you have an idea on a different approach than adding a new file here?.
Also, it's based on master and not 0.2.9 because is using SRCDIR was introduced by nickm for the geoip test.

comment:8 Changed 10 months ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final
Status: merge_readyneeds_revision

comment:9 Changed 10 months ago by nickm

Reviewer: nickm
Status: needs_revisionneeds_review

comment:10 Changed 10 months ago by dgoulet

Reviewer: nickmasn

comment:11 Changed 10 months ago by nickm

WRT the question of how to avoid adding an extra file: you can use the "get_fname()" function to get a temporary filename from within a unit test, and then use write_str_to_file() to write a string into that file. The file will be automatically cleaned up when the unit tests exit.

For a very simple example, see test_util_listdir() in test_util.c .

comment:12 Changed 10 months ago by juga

Thanks!, that was very useful.
I changed the test following that, now in my branch ticket26007_029_02.

comment:13 Changed 10 months ago by asn

Status: needs_reviewmerge_ready

juga's branch ticket26007_029_02 looks good to me!

comment:14 Changed 10 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Also merged to 029 and forward. Thanks!

Note: See TracTickets for help on using tickets.