Opened 6 weeks ago

Closed 43 hours ago

#24099 closed defect (fixed)

tor should remove empty/invalid consensus cache files (Unable to map file from consensus cache: Numerical result out of range)

Reported by: Hello71 Owned by: nickm
Priority: Low Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Minor Keywords: 031-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: ahf Sponsor:

Description

I use XFS, which tends to leave newly-written files empty on crash rather than non-existent. tor should handle this better.

Child Tickets

Change History (11)

comment:1 Changed 6 weeks ago by nickm

Milestone: Tor: 0.3.2.x-final

comment:2 Changed 4 weeks ago by nickm

Owner: set to nickm
Status: newaccepted

comment:3 Changed 3 weeks ago by nickm

Keywords: 031-backport added
Status: acceptedneeds_review

Branch bug24099_031 tries to fix this. Please review?

comment:4 Changed 3 weeks ago by dgoulet

I'm a bit confused here, ERANGE is not documented as an errno value from mmap(2) both on Linux and FreeBSD. It seems that the range issue is handled with:

  EINVAL We don't like addr, length, or offset (e.g., they are too large, or not aligned on a page boundary).

@Hello71, what is your kernel version here? I've checked (on kernel 4.14) and XFS doesn't seem to be returning ERANGE in the case of a mmap...

The fix lgtm; but this ERANGE thing is just weird and confusing to me from which syscall this can come. The open(), fstat() and mmap() don't seem to never return that :S ...

comment:5 Changed 3 weeks ago by ahf

Reviewer: ahf

comment:6 Changed 3 weeks ago by Hello71

/** Try to create a memory mapping for <b>filename</b> and return it.  On
 * failure, return NULL.  Sets errno properly, using ERANGE to mean
 * "empty file". */
tor_mmap_t *
tor_mmap_file(const char *filename)

comment:7 Changed 3 weeks ago by dgoulet

Oh true, it is "artificial" there. Fair enough, patch lgtm;

comment:8 Changed 3 weeks ago by ahf

Status: needs_reviewmerge_ready

cabcb752d7ecc2d16e6cb630b3de0684b4f97ec5 looks good.

As per discussion on IRC, in c8ee12b2e8108658d647aedb92885311291b6f71, it would be useful to have a comment in consensus_cache_rescan() about the meaning of errno == ERANGE after calling tor_mmap_file().

I think we are generally a bit inconsistent about this, but there is an empty line at the end of changes/bug24099, but it looks like that shouldn't really matter.

Otherwise the patches look good.

comment:9 Changed 3 weeks ago by nickm

I've added an explanatory comment to the branch, and am now about to merge to 0.3.2 and forward.

It's okay to be inconsistent about blanks at the end of changes files: our tools clean them up when the changelog is built.

comment:10 Changed 3 weeks ago by nickm

Milestone: Tor: 0.3.2.x-finalTor: 0.3.1.x-final

Merged to 0.3.2 and forward; marking for possible backport.

comment:11 Changed 43 hours ago by nickm

Resolution: fixed
Status: merge_readyclosed

merged to 0.3.1!

Note: See TracTickets for help on using tickets.