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

Segfault after obspy.read #1658

Merged
merged 9 commits into from Mar 24, 2017

Conversation

Projects
None yet
4 participants
@krischer
Member

krischer commented Mar 23, 2017

Hi devs, running the QC we are sometimes running in to a segfault reading the record headers of some files. It's been quite a challenge but I traced it back to the obspy.read subroutine... it really blows my mind.

from obspy import read
from obspy.io.mseed.util import get_flags

# Reading the file before getting the records causes a segfault
# when getting header flags.
read("broken_records.mseed")
get_flags("broken_records.mseed")

>>> Warning: Number of blockettes in fixed header (2) does not match the number parsed (1)
>>> Segmentation fault (core dumped)

When we do not read the file first the segmentation fault does not occur.:

from obspy import read
from obspy.io.mseed.util import get_flags

get_flags("broken_records.mseed")

>>> Warning: Number of blockettes in fixed header (2) does not match the number parsed (1)
>>> Warning: Number of blockettes in fixed header (2) does not match the number parsed (1)
>>> ... identical for all 16 records

Here is the file: ftp://www.orfeus-eu.org/pub/outgoing/broken_records.mseed (8 kb).

Thanks for the help and hope you understand this behaviour better than me..

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Feb 8, 2017

Member

I can confirm the problem.. no idea what's going on though..

Member

megies commented Feb 8, 2017

I can confirm the problem.. no idea what's going on though..

@megies megies added this to the 1.1.0 milestone Feb 8, 2017

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Mar 22, 2017

Member

Any new insights for this one @Jollyfant?

Member

megies commented Mar 22, 2017

Any new insights for this one @Jollyfant?

@Jollyfant

This comment has been minimized.

Show comment
Hide comment
@Jollyfant

Jollyfant Mar 22, 2017

Contributor

Nope.. I'm putting my money on @krischer for this one.

Contributor

Jollyfant commented Mar 22, 2017

Nope.. I'm putting my money on @krischer for this one.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Mar 22, 2017

Member

Might be connected to #1728..?

Member

megies commented Mar 22, 2017

Might be connected to #1728..?

krischer added some commits Mar 23, 2017

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Mar 23, 2017

Member

This is now A PR fixing the issue.

The problem was that the call to read_mseed() hooked up libmseed's logging to some Python callbacks. Leaving the read function the Python functions were garbage collected and a call the next time naturally resulted in a segfault.

There is now a wrapper around the ctypes lib to always setup the logging so this can no longer happen and it is completely transparent to anyone directly using libmseed from Python.

Does not fix #1728 which is another issue.

Member

krischer commented Mar 23, 2017

This is now A PR fixing the issue.

The problem was that the call to read_mseed() hooked up libmseed's logging to some Python callbacks. Leaving the read function the Python functions were garbage collected and a call the next time naturally resulted in a segfault.

There is now a wrapper around the ctypes lib to always setup the logging so this can no longer happen and it is completely transparent to anyone directly using libmseed from Python.

Does not fix #1728 which is another issue.

krischer added some commits Mar 23, 2017

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Mar 23, 2017

Member

IMHO ready to go.

Member

krischer commented Mar 23, 2017

IMHO ready to go.

krischer added some commits Mar 24, 2017

Renamed internal mseed exception + warning classes.
Now called InternalMSEEDError instead of InternalMSEEDReadingError and
InternalMSEEDWarning instead of InternalMSEEDReadingWarnings as both can
now also be raised in non-reading contexts.
@Jollyfant

Wrapper appears to be good and is covered by tests.

@krischer krischer merged commit c147e47 into master Mar 24, 2017

7 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 97.52% of diff hit (target 90%)
Details
codecov/project 87.58% (+1.54%) compared to a2f4a69
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
docker-deb-buildbot Deb packaging and testing succeeded
Details
docker-testbot Docker tests succeeded
Details
@QuLogic

There were still a few issues...

Show outdated Hide outdated CHANGELOG.txt
@@ -581,11 +497,11 @@ class MSTraceSeg(C.Structure):
('samprate', C.c_double), # Nominal sample rate (Hz)
('samplecnt', C.c_int64), # Number of samples in trace coverage
('datasamples', C.c_void_p), # Data samples, 'numsamples' of type
# 'sampletype'
# 'sampletype'

This comment has been minimized.

@QuLogic

QuLogic Mar 24, 2017

Member

This is wrapped from the previous line; looks pretty weird unindented like this.

@QuLogic

QuLogic Mar 24, 2017

Member

This is wrapped from the previous line; looks pretty weird unindented like this.

def _wrapper(*args):
# Collect exceptions. They cannot be raised in the callback as
# they could never be caught then. They are collected an raised

This comment has been minimized.

@QuLogic

QuLogic Mar 24, 2017

Member

s/an/and/

@QuLogic

QuLogic Mar 24, 2017

Member

s/an/and/

_warns.append(msg)
diag_print = \
C.CFUNCTYPE(C.c_void_p, C.c_char_p)(log_error_or_warning)

This comment has been minimized.

@QuLogic

QuLogic Mar 24, 2017

Member

Return type is None, not c_void_p.

@QuLogic

QuLogic Mar 24, 2017

Member

Return type is None, not c_void_p.

def log_message(msg):
if self.verbose:
print(msg[6:].strip())
log_print = C.CFUNCTYPE(C.c_void_p, C.c_char_p)(log_message)

This comment has been minimized.

@QuLogic

QuLogic Mar 24, 2017

Member

Return type is None, not c_void_p.

@QuLogic

QuLogic Mar 24, 2017

Member

Return type is None, not c_void_p.

# this is completely different to C.c_char_p which is a string
__clibmseed.mst_packgroup.argtypes = [
C.POINTER(MSTraceGroup), C.CFUNCTYPE(
C.c_void_p, C.POINTER(C.c_char), C.c_int, C.c_void_p),

This comment has been minimized.

@QuLogic

QuLogic Mar 24, 2017

Member

None, not c_void_p (the first time).

@QuLogic

QuLogic Mar 24, 2017

Member

None, not c_void_p (the first time).

__clibmseed.mst_packgroup.argtypes = [
C.POINTER(MSTraceGroup), C.CFUNCTYPE(
C.c_void_p, C.POINTER(C.c_char), C.c_int, C.c_void_p),
C.c_void_p, C.c_int, C.c_short, C.c_short, C.POINTER(C.c_int), C.c_short,

This comment has been minimized.

@QuLogic

QuLogic Mar 24, 2017

Member

C.POINTER(C.c_int64), actually

@QuLogic

QuLogic Mar 24, 2017

Member

C.POINTER(C.c_int64), actually

C.c_int8,
C.CFUNCTYPE(C.c_void_p, C.c_char_p),
C.CFUNCTYPE(C.c_void_p, C.c_char_p)]
__clibmseed.setupLogging.restype = C.c_void_p

This comment has been minimized.

@QuLogic

QuLogic Mar 24, 2017

Member

None

@QuLogic
__clibmseed.msr_free.argtypes = [C.POINTER(C.POINTER(MSRecord))]
__clibmseed.msr_free.restype = C.c_void_p

This comment has been minimized.

@QuLogic

QuLogic Mar 24, 2017

Member

None

@QuLogic
__clibmseed.lil_free.argtypes = [C.POINTER(LinkedIDList)]
__clibmseed.lil_free.restype = C.c_void_p

This comment has been minimized.

@QuLogic

QuLogic Mar 24, 2017

Member

None

@QuLogic
@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Mar 25, 2017

Member

Thanks for the review @QuLogic, I've opened a follow up issue to make sure your comments don't go unnoticed..

Member

megies commented Mar 25, 2017

Thanks for the review @QuLogic, I've opened a follow up issue to make sure your comments don't go unnoticed..

@megies megies deleted the segfault-get-flags branch Mar 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment