Opened 5 months ago

Closed 4 months ago

#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 4 months ago.
patch
tor_mmap_file-ceiling.patch (1.6 KB) - added by junglefowl 4 months ago.
check against SSIZE_T_CEILING

Download all attachments as: .zip

Change History (19)

comment:1 Changed 5 months ago by nickm

  • Milestone set to Tor: 0.3.0.x-final
  • Status changed from new to needs_review

comment:2 Changed 4 months ago by dgoulet

  • Status changed from needs_review to needs_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 4 months ago by nickm

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

comment:4 Changed 4 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 4 months ago by nickm

  • Status changed from needs_information to needs_review

comment:6 Changed 4 months ago by nickm

  • Keywords review-group-15 added

comment:7 Changed 4 months ago by nickm

  • Reviewer set to nickm

comment:8 Changed 4 months ago by dgoulet

  • Status changed from needs_review to needs_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 4 months ago by nickm

  • Status changed from needs_information to needs_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 4 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 4 months ago by nickm

  • Status changed from needs_review to needs_revision

comment:12 follow-up: Changed 4 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 4 months ago by junglefowl

  • Status changed from needs_revision to needs_review

comment:14 in reply to: ↑ 12 ; follow-up: Changed 4 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 4 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 4 months ago by junglefowl

patch

Changed 4 months ago by junglefowl

check against SSIZE_T_CEILING

comment:16 Changed 4 months ago by nickm

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

comment:17 Changed 4 months ago by nickm

  • Resolution set to fixed
  • Status changed from needs_review to closed
Note: See TracTickets for help on using tickets.