Opened 7 months ago

Closed 6 months ago

#25399 closed defect (fixed)

mmap length doesn't need to be page-aligned

Reported by: Hello71 Owned by:
Priority: Low Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Minor Keywords: review-group-34
Cc: Actual Points:
Parent ID: Points: 0.5
Reviewer: ahf Sponsor:

Description

POSIX says that mmap length doesn't need to be page-aligned. addr and off may have page alignment requirements, but length must be handled automatically. getpagesize is deprecated too.

Child Tickets

Change History (7)

comment:1 Changed 7 months ago by teor

Milestone: Tor: 0.3.4.x-final
Points: 0.5

Can you explain how this bug affects Tor?

comment:2 Changed 7 months ago by Hello71

https://cgit.alxu.ca/tor.git/commit/?h=bug25399

removes cruft, very slightly increases performance.

comment:3 Changed 7 months ago by Hello71

Status: newneeds_review

comment:4 Changed 7 months ago by nickm

Keywords: review-group-34 added

comment:5 Changed 7 months ago by dgoulet

Reviewer: ahf

Reviewer week 03/16th

comment:6 Changed 6 months ago by ahf

Status: needs_reviewmerge_ready

As far as I can tell this looks good. Even the man pages mentions that getpagesize() is deprecated and sysconf(_SC_PAGESIZE) should be used instead.

The interesting part from POSIX:

The off argument is constrained to be aligned and sized according to the value 
returned by sysconf() when passed _SC_PAGESIZE or _SC_PAGE_SIZE. When MAP_FIXED
is specified, the argument addr must also meet these constraints. The
implementation performs mapping operations over whole pages. Thus, while the
argument len need not meet a size or alignment constraint, the implementation
will include, in any mapping operation, any partial page specified by the range
[pa, pa + len).

comment:7 Changed 6 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

okay; merging this to master. I wonder if any of the mingw workarounds are still needed, but I guess we'll find out soon :)

Note: See TracTickets for help on using tickets.