#21134 closed defect (fixed)

Fail if file is too large to mmap.

Reported by: junglefowl Owned by:
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version: Tor: 0.2.9.8
Severity: Normal Keywords: tor_mmap_file, review-group-15
Cc: Actual Points:
Parent ID: Points:
Reviewer: nickm Sponsor:

Description

If tor_mmap_file is called with a file which is larger than SIZE_MAX,
only a small part of the file will be memory-mapped due to integer
truncation.

This can only realistically happen on 32 bit architectures with large
file support.

Child Tickets

Attachments (2)

tor_mmap_file.patch (1.6 KB) - added by junglefowl 10 months ago.
patch
tor_mmap_file-ceiling.patch (1.6 KB) - added by junglefowl 10 months ago.
check against SSIZE_T_CEILING

Download all attachments as: .zip

Change History (19)

comment:1 Changed 11 months ago by nickm

Milestone: Tor: 0.3.0.x-final
Status: newneeds_review

comment:2 Changed 10 months ago by dgoulet

Status: needs_reviewneeds_information

Hrm, both on 32 and 64 bit, don't size_t and off_t (st.st_size) are the same size? If so, that SIZE_MAX check is only useful with "==". I do think the < check is useful which would detect the overflow if the pagesize adjustment triggers it.

if (st.st_size >= SIZE_MAX || size < st.st_size) {

This snippet gives me the same size (on 64 bit and -m32):

  size_t max = SIZE_MAX;
  off_t max2 = (~(off_t)0);

comment:3 Changed 10 months ago by nickm

off_t is usually 64-bit even when size_t is 32-bit.

comment:4 Changed 10 months ago by junglefowl

---
#include <stdint.h>
#include <stdio.h>

int
main(void)
{

printf("size_t: %d\n", sizeof(size_t));
printf("off_t: %d\n", sizeof(off_t));

return 0;

}
---

If you compile this code with -D_FILE_OFFSET_BITS=64 (large file support), 32 bit systems will use 4 bytes for size_t and 8 bytes for off_t:

$ uname -m
i686
$ gcc -o sizes sizes.c
$ ./sizes
size_t: 4
off_t: 4
$ gcc -D_FILE_OFFSET_BITS=64 -o sizes sizes.c
$ ./sizes
size_t: 4
off_t: 8

comment:5 Changed 10 months ago by nickm

Status: needs_informationneeds_review

comment:6 Changed 10 months ago by nickm

Keywords: review-group-15 added

comment:7 Changed 10 months ago by nickm

Reviewer: nickm

comment:8 Changed 10 months ago by dgoulet

Status: needs_reviewneeds_information

Wait, so normal behavior of gcc on 32 bit is that off_t and size_t are the same size so this still wouldn't work then?

if (st.st_size >= SIZE_MAX || size < st.st_size) {

comment:9 Changed 10 months ago by nickm

Status: needs_informationneeds_review

We build with large-file support by default, since we have "AC_SYS_LARGEFILE" in our configure.ac.

Looking at the code, I think this needs these changes.

  • It needs a changes file (see doc/HACKING/GettingStarted.md)
  • We should really allow the case where st.st_size == SIZE_MAX.
  • This should be at least a warning, not an INFO message, since we can't really recover from this problem.

comment:10 Changed 10 months ago by nickm

Oh, one more: off_t is a signed type, and size_t isn't, so the comparison of the two can be trouble.

comment:11 Changed 10 months ago by nickm

Status: needs_reviewneeds_revision

comment:12 Changed 10 months ago by junglefowl

I have modified the patch as requested:

  • added a changes file
  • st_size is checked to be non-negative before comparing with SIZE_MAX
  • increased log level to warn

Keep in mind that an empty file is logged with info though. In fact I copied that block because it seemed to be the best matching error handling one.

I did not adjust the code to accept SIZE_MAX. It will be impossible to load SIZE_MAX bytes into the address space, because even the used bytes in the stack make it impossible to succeed.

comment:13 Changed 10 months ago by junglefowl

Status: needs_revisionneeds_review

comment:14 in reply to:  12 ; Changed 10 months ago by teor

Replying to junglefowl:

...

I did not adjust the code to accept SIZE_MAX. It will be impossible to load SIZE_MAX bytes into the address space, because even the used bytes in the stack make it impossible to succeed.

There are some architectures where the size of individual objects is limited to SIZE_MAX, but the address space is larger than SIZE_MAX. Segmented architectures (where the stack and heap are in separate address spaces) are one example. So I don't think we can rely on the stack to save us from mapping SIZE_MAX.

(Also, sometimes we use SIZE_T_CEILING to catch underflows - that might be what you want to do here.)

comment:15 in reply to:  14 Changed 10 months ago by junglefowl

Replying to teor:

Thank you for your explanation of SIZE_MAX, I always considered it as the maximum address space available. I also like your SIZE_T_CEILING recommendation a lot, although I adjusted it to SSIZE_T_CEILING so the signedness is in sync with off_t.

This does not support SIZE_MAX though. That is why I uploaded it as a second patch file. But I prefer this solution now, because it behaves just like the alternative implementation of tor_mmap_file if the memory has to be allocated by using malloc (if a system lacks mmap support).

A 32 bit system could be down to almost 2 GB files, but this still sounds very acceptable. It is a common limitation of such systems, even due to a signed off_t.

Changed 10 months ago by junglefowl

Attachment: tor_mmap_file.patch added

patch

Changed 10 months ago by junglefowl

Attachment: tor_mmap_file-ceiling.patch added

check against SSIZE_T_CEILING

comment:16 Changed 10 months ago by nickm

Merged! I added a cast to the comparison to avoid a signed/unsigned comparison warning.

comment:17 Changed 10 months ago by nickm

Resolution: fixed
Status: needs_reviewclosed
Note: See TracTickets for help on using tickets.