Opened 8 years ago
Last modified 8 years ago
#11942 closed defect
segfault on Graph().connected_component_containing_vertex('') — at Version 11
Reported by: | kini | Owned by: | jason, ncohen, rlm |
---|---|---|---|
Priority: | major | Milestone: | sage-5.0 |
Component: | graph theory | Keywords: | segfault cython |
Cc: | Merged in: | ||
Authors: | Nathann Cohen, Keshav Kini | Reviewers: | Keshav Kini, David Coudert |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
As title.
fs@zhenghe /tmp $ sage -c "Graph().connected_component_containing_vertex('')" /opt/sage/local/lib/libcsage.so(print_backtrace+0x31)[0x7f14e1a96df2] /opt/sage/local/lib/libcsage.so(sigdie+0x14)[0x7f14e1a96e24] /opt/sage/local/lib/libcsage.so(sage_signal_handler+0x20c)[0x7f14e1a96a72] /lib/x86_64-linux-gnu/libpthread.so.0(+0xf020)[0x7f14e31fa020] /opt/sage/local/lib/python2.6/site-packages/sage/graphs/base/c_graph.so(+0x11aa1)[0x7f14be4d8aa1] /opt/sage/local/lib/libpython2.6.so.1.0(+0x75876)[0x7f14e347c876] /opt/sage/local/lib/libpython2.6.so.1.0(+0x75b92)[0x7f14e347cb92] /opt/sage/local/lib/libpython2.6.so.1.0(+0xa0f08)[0x7f14e34a7f08] /opt/sage/local/lib/libpython2.6.so.1.0(PyObject_Call+0x53)[0x7f14e3451cd3] /opt/sage/local/lib/libpython2.6.so.1.0(PyEval_EvalFrameEx+0x395d)[0x7f14e34e835d] /opt/sage/local/lib/libpython2.6.so.1.0(PyEval_EvalFrameEx+0x602d)[0x7f14e34eaa2d] /opt/sage/local/lib/libpython2.6.so.1.0(PyEval_EvalCodeEx+0x879)[0x7f14e34eb549] /opt/sage/local/lib/libpython2.6.so.1.0(PyEval_EvalCode+0x32)[0x7f14e34eb642] /opt/sage/local/lib/libpython2.6.so.1.0(+0xdb07e)[0x7f14e34e207e] /opt/sage/local/lib/libpython2.6.so.1.0(PyEval_EvalFrameEx+0x4f8d)[0x7f14e34e998d] /opt/sage/local/lib/libpython2.6.so.1.0(PyEval_EvalCodeEx+0x879)[0x7f14e34eb549] /opt/sage/local/lib/libpython2.6.so.1.0(PyEval_EvalCode+0x32)[0x7f14e34eb642] /opt/sage/local/lib/libpython2.6.so.1.0(PyRun_FileExFlags+0xb0)[0x7f14e350d9b0] /opt/sage/local/lib/libpython2.6.so.1.0(PyRun_SimpleFileExFlags+0xdf)[0x7f14e350e3ef] /opt/sage/local/lib/libpython2.6.so.1.0(Py_Main+0xb23)[0x7f14e351b693] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xfd)[0x7f14e27fcead] python[0x4006a1] ------------------------------------------------------------------------ Unhandled SIGSEGV: A segmentation fault occurred in Sage. This probably occurred because a *compiled* component of Sage has a bug in it and is not properly wrapped with sig_on(), sig_off(). You might want to run Sage under gdb with 'sage -gdb' to debug this. Sage will now terminate. ------------------------------------------------------------------------ /opt/sage/local/bin/sage-sage: line 772: 19065 Segmentation fault sage-eval "$@" fs@zhenghe /tmp $
Apply:
Change History (12)
comment:1 Changed 8 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:2 follow-up: ↓ 3 Changed 8 years ago by
- Reviewers set to Keshav Kini
Wow, that was fast! :D I have a couple of questions, though. Graph().connected_component_containing_vertex(0)
already produces an exception, which includes the number 0 in the message. This is still true with your patch. Currently investigating why...
Also, your exception is a ValueError
- shouldn't it be a KeyError
? Shouldn't the current error for vertex 0
be a KeyError
too instead of a (!?) RuntimeError
? The Python docs say:
This exception is mostly a relic from a previous version of the interpreter; it is not used very much any more.
comment:3 in reply to: ↑ 2 Changed 8 years ago by
Hellooooooo !
This exception is mostly a relic from a previous version of the interpreter; it is not used very much any more.
Oh ? That's a pity, I really use it a lot :-)
I immediately update the patch to return a KeyError? :-)
Nathann
comment:4 follow-up: ↓ 5 Changed 8 years ago by
Oh, ValueError
is still used a lot but it is RuntimeError
which is not used much anymore.
comment:5 in reply to: ↑ 4 Changed 8 years ago by
Oh,
ValueError
is still used a lot but it isRuntimeError
which is not used much anymore.
Argggggg !!!
Patch reverted to the former one :-D
Nathann
Changed 8 years ago by
comment:6 follow-up: ↓ 7 Changed 8 years ago by
... but... even though ValueError
is used a lot, the correct one to use here is KeyError
... D:
Anyway don't worry about it, I'll do that in another patch later :) Sorry, didn't get much time to look at this earlier this week.
comment:7 in reply to: ↑ 6 Changed 8 years ago by
Replying to kini:
... but... even though
ValueError
is used a lot, the correct one to use here isKeyError
...D:
Arggggggggggg !!!
Anyway don't worry about it, I'll do that in another patch later :) Sorry, didn't get much time to look at this earlier this week.
Good !! THaaaaanks ! :-)
Nathann
comment:9 Changed 8 years ago by
- Milestone set to sage-4.8
- Reviewers changed from Keshav Kini to Keshav Kini, David Coudert
- Status changed from needs_review to positive_review
I don't understand why you have both stopped the discussion without concluding on the status of this patch.
I have no problem when installing the patch and building the documentation. The function tests correctly that the parameter is or is not a vertex of the graph.
For me the patch is correct, so I give a positive review.
Best, David.
comment:10 Changed 8 years ago by
- Status changed from positive_review to needs_work
Please read the comments. The ValueError? needs to be changed to KeyError?, and also I wanted to understand the following before giving it a positive review, as possibly the fix could be improved (multiple code paths for a single purpose is bad design):
Graph().connected_component_containing_vertex(0)
already produces an exception, which includes the number 0 in the message. This is still true with your patch.
Sorry for neglecting this ticket for so long.
comment:11 Changed 8 years ago by
- Description modified (diff)
- Keywords segfault cython added
- Status changed from needs_work to needs_review
Well, I'm still not sure about the multiple code paths. But here is a patch to use better exceptions throughout the file in question, though that's sort of tangential to the original purpose of the ticket. This ticket has been sitting around too long for me to delay it any further just because of my ignorance of the Cython graphs code, especially since it fixes a segfault, so I will give a positive review to Nathann's part. Maybe someone can review my exceptions patch? It passes make ptestlong
on my machine fwiw.
Right !
:-p
This comes from an unchecked operation at the beginning of BFS/DFS. Fixed in the following patch
:-)
Nathann