bcftools segfaults python with pretty much any non-zero exit

[adadiehl]

The title says it all. If it so much as exits with a usage message, it takes Python down with it. Initially discovered with a call to bcftools consensus in which the fasta reference was malformed.

[jmarshall]

This is the same problem as #958 (part two) and #965. The problem is that the external bcftools source code calls exit() on pretty much any error. This in turn exits from the Python interpreter. (The samtools code sometimes calls exit() but usually instead returns an error indication from main(), which does not abort the Python interpreter.)

When invoking bcftools commands, make very sure to get your arguments correct and avoid causing error messages 😄

[jmarshall]

I suppose we could try rewriting exit() as a longjmp() back to the main dispatcher in samtools_main()/ bcftools_main() or _pysam_dispatch. That'd cause fairly major resource leaks, but mostly only in error situations…

[jkbonfield]

I don't understand pysam so this maybe a naive question, but surely it's either A) using htslib in which case any exit is a failure and should be reported to us or B) executing bcftools command-lines in which case exit is something that can be caught and wouldn't kill python.

If it's hacking into the bcftools binary and making python calls directly from within the main thread without using a sub-command then that would come under the "courageous decision" mantra!

Edit: if this is heroically what it's attempting to do, then may I suggest a fork and wait style of approach so abnormal exits from the binary-masquerading-as-a-library idea doesn't take out the whole python interpreter?

[jmarshall]

@jkbonfield: This is the same comment as you previously made in #958 (comment) 😄

Most of pysam is nice API functions calling nice HTSlib API functions and checking the nice HTSlib return codes. (And occasionally getting annoyed by the not so nice HTSlib logging; e.g. #939.)

But one dusty annex of pysam allows you to invoke samtools/bcftools commands as Python functions with a bunch of string arguments (rather than command line arguments). That's implemented by linking in all of samtools and bcftools and invoking their main() routines as a subroutine. That is both a nice hack, a good way of avoiding a dependency on an external executable, and a somewhat “courageous decision” — all simultaneously.

You already did suggest fork-and-wait, and I replied that patches are welcome. No PRs have appeared from any quarter, and I have just thought of setjmp/longjmp as a slightly less nasty alternative (in some ways) that I would be up for implementing. TL;DR there is no suggest, only do 🙃

[jkbonfield]

lol so it is, almost exactly! I'd obviously managed to scrub this lunacy from my memory. Peril sensitive sunglasses perhaps - there's hope my sanity will recover in that case after dabbling into mpileup. :)

However I have enough on my plate bringing our existing code up to scratch. I'm not a python developer, so I'm not going to learn a new language just to fix someone elses broken bindings for them. I can only advise on what is sensible for people trying to integrate our (samtools org) code. Until then, I think I can only but whole-heartedly agree with the author of #965.

[jmarshall]

The author of #965 was remarkably rude, and his issue caused maintainer demotivation and increased burnout. It was also a ridiculous false claim: the bulk of pysam propagates HTSlib and other error statuses just fine. Only the pysam.samtools.* and pysam.bcftools.* functions invoke samtools/bcftools application code directly like this, and if you don't want to use them you have the simple solution of not using them.

@adadiehl's approach OTOH is probably going to result in this being ameliorated. 😄

[jkbonfield]

Agreed it's obviously only the samtools / bcftools ones as htslib is (I hope!) safe, but I've no idea on the usage/prevalence of each part of API.

If those bits are unsafe though and pysam developers don't have time to guard against this, then maybe the documentation just needs to state the callers should wrap them up some way to protect against application exits. Not pleasant, but documented nasties beat undocumented ones! Ie "Told you so! :P".

A FAQ entry perhaps?

[jmarshall]

Now fixed by rewriting exit() as a longjmp() back to the main dispatcher.