Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using lseek inside map_file is unfortunate, better use fstat #5543

Closed
vicuna opened this issue Mar 16, 2012 · 7 comments

Comments

Projects
None yet
1 participant
@vicuna
Copy link

commented Mar 16, 2012

Original bug ID: 5543
Reporter: gerd
Status: closed (set by @xavierleroy on 2013-08-31T10:48:57Z)
Resolution: fixed
Priority: normal
Severity: tweak
Version: 3.12.1
Category: otherlibs
Monitored by: mehdi

Bug description

The problem is that there are file descriptors that do not support seeking, but that are mappable. For example, this is the case for the descriptors returned by shm_create(). lseek is not listed in the allowed operations, but fstat is. Of course, a shared memory object has a size, and it is normally perfectly usable with map_file.

The problem occurs on BSD systems, just saw it on OS X (but I remember I saw it already on FreeBSD), which returns ESPIPE.

Additional information

Suggested fix: In otherlibs/bigarray/mmap_unix.c, replace the lines

currpos = lseek(fd, 0, SEEK_CUR);
if (currpos == -1) {
...
}

by

if (fstat(fd, &st) == -1) {
...
}
currpos = st.st_size;

Also declare:

struct stat st;

and

#include <sys/stat.h>

File attachments

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 16, 2012

Comment author: @xavierleroy

Yes, that's a good suggestion. lseek() hackery would still be needed in the case where the file needs to be grown before mmap()-ing, but not in the other case. I would gladly consider a patch, esp. if it's been tested.

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 16, 2012

Comment author: gerd

Ok, I'll also replace the other lseek/write with an ftruncate, at least if lseek is not possible. Working on a patch.

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 25, 2012

Comment author: gerd

So, produced a patch which hopefully catches all cases. Comments are inside the patch. On a modern OS lseek is never used, but the lseek code is still available for older systems.

Testing so far only on 64 bit Linux. Will try to do some more testing the next days.

@vicuna

This comment has been minimized.

Copy link
Author

commented Mar 26, 2012

Comment author: @xavierleroy

Thanks for the highly nontrivial patch :-) A cosmetic suggestion: for clarity, maybe the machinery to grow the file should be moved to a separate static "grow_file" function. Let us know how the testing goes.

@vicuna

This comment has been minimized.

Copy link
Author

commented Apr 2, 2012

Comment author: gerd

The results so far are good:

  • FreeBSD 7.2 (32 bit)
  • Opensolaris 8.11 (32 bit)
  • Mac OSX 10.5 (32 bit)
  • Linux 2.6.26/glibc 2.11.3 (64 bit)

On the latter system I especially tested mmapping a large file (8GB).

I did not find any system lacking pwrite support. The code using lseek as fallback is a bit questionable because of this - but maybe we should keep it for supporting exotic systems.

I'll see whether I can also do a test on Cygwin.

@vicuna

This comment has been minimized.

Copy link
Author

commented Apr 2, 2012

Comment author: gerd

Uploaded new patch mmap_growfile implementing the suggested changes.

@vicuna

This comment has been minimized.

Copy link
Author

commented Apr 4, 2012

Comment author: @xavierleroy

Second patch merged in 4.00 branch (r12316) and in trunk (r12317).

@vicuna vicuna closed this Aug 31, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.