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

Don't crash when writing files on 64bit machines #13

Merged
merged 1 commit into from
Apr 10, 2017

Conversation

ximion
Copy link
Contributor

@ximion ximion commented Apr 6, 2017

Hi!
If you tried to write an EDF file on an amd64 machine (and possible even any other machine) using this Python module, you ran into a segmentation fault:

Program received signal SIGSEGV, Segmentation fault.
__GI_rewind (fp=fp@entry=0x566f34a0) at rewind.c:34
34      rewind.c: Datei oder Verzeichnis nicht gefunden.
#0  __GI_rewind (fp=fp@entry=0x566f34a0) at rewind.c:34
#1  0x00007fffec7471bb in edflib_write_edf_header (hdr=hdr@entry=0x555556789bb0) at pyedflib/_extensions/c/edflib.c:4873
#2  0x00007fffec74c3ea in edf_blockwrite_physical_samples (handle=handle@entry=0, buf=<optimized out>)
    at pyedflib/_extensions/c/edflib.c:4710

This happens because the fopen64 and other functions are not defined. The compiler even warns about that and I thought about making that warning fatal while writing this patch...

The problem is that if a reference is undefined, the functions will be assumed, as per C standard, to return an int datatype, which is not large enough to hold a pointer to FILE* on 64bit devices. Therefore, the pointer gets smashed and any subsequent action on it is doomed to fail and crash the program.

The solution is to always compile with large file support, so the respective functions are defined. This will also get rid of almost all compiler warnings (the edflib.c file even contains a note about this at its top, so it looks like its original author was aware of this problem).

To reproduce the bug, just try to run the demo/writeEDFFile.py on an amd64 machine.

Cheers,
Matthias

@codecov-io
Copy link

codecov-io commented Apr 6, 2017

Codecov Report

Merging #13 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #13   +/-   ##
=======================================
  Coverage   60.21%   60.21%           
=======================================
  Files           6        6           
  Lines        3014     3014           
  Branches       58       58           
=======================================
  Hits         1815     1815           
  Misses       1179     1179           
  Partials       20       20
Impacted Files Coverage Δ
pyedflib/_extensions/c/edflib.c 59.1% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9b6e520...3ef23a7. Read the comment docs.

@holgern holgern merged commit 138f42a into holgern:master Apr 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants