Opened 9 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 kini)

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:

  1. trac_11942.patch
  2. trac_11942-exceptions.patch

Change History (12)

comment:1 Changed 9 years ago by ncohen

  • Authors set to Nathann Cohen
  • Description modified (diff)
  • Status changed from new to needs_review

Right ! :-p

This comes from an unchecked operation at the beginning of BFS/DFS. Fixed in the following patch :-)

Nathann

comment:2 follow-up: Changed 9 years ago by kini

  • 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 9 years ago by ncohen

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: Changed 9 years ago by kini

Oh, ValueError is still used a lot but it is RuntimeError which is not used much anymore.

comment:5 in reply to: ↑ 4 Changed 9 years ago by ncohen

Oh, ValueError is still used a lot but it is RuntimeError which is not used much anymore.

Argggggg !!!

Patch reverted to the former one :-D

Nathann

Changed 9 years ago by ncohen

comment:6 follow-up: Changed 9 years ago by kini

... 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 9 years ago by ncohen

Replying to kini:

... but... even though ValueError is used a lot, the correct one to use here is KeyError... 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:8 Changed 9 years ago by jdemeyer

  • Milestone sage-4.7.3 deleted

Milestone sage-4.7.3 deleted

comment:9 Changed 8 years ago by dcoudert

  • 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 kini

  • 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 kini

  • Authors changed from Nathann Cohen to Nathann Cohen, Keshav Kini
  • 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.

Note: See TracTickets for help on using tickets.