Opened 7 years ago

Closed 7 years ago

#11942 closed defect (fixed)

segfault on Graph().connected_component_containing_vertex('')

Reported by: kini Owned by: jason, ncohen, rlm
Priority: major Milestone: sage-5.0
Component: graph theory Keywords: segfault cython
Cc: Merged in: sage-5.0.beta0
Authors: Nathann Cohen, Keshav Kini, Jeroen Demeyer Reviewers: Keshav Kini, David Coudert, Nathann Cohen
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

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.2.patch

Attachments (3)

trac_11942.patch (1.8 KB) - added by ncohen 7 years ago.
trac_11942-exceptions.patch (9.5 KB) - added by kini 7 years ago.
apply to $SAGE_ROOT/devel/sage
trac_11942-exceptions.2.patch (9.7 KB) - added by jdemeyer 7 years ago.
Use LookupError? instead of ValueError? or KeyError?

Download all attachments as: .zip

Change History (34)

comment:1 Changed 7 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 7 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 7 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 7 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 7 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 7 years ago by ncohen

comment:6 follow-up: Changed 7 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 7 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 7 years ago by jdemeyer

  • Milestone sage-4.7.3 deleted

Milestone sage-4.7.3 deleted

comment:9 Changed 7 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 7 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 7 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.

comment:12 Changed 7 years ago by ncohen

  • Status changed from needs_review to needs_work

Well, what about setting the message to what it was before, and just change the exception's name if yo feels it should be ? The former message was way clearer, especially when the exception is thrown by a submethod of the method run by the user.

Nathann

comment:13 Changed 7 years ago by kini

ValueError's constructor accepts a string representing a message, but KeyError's constructor accepts an object representing the key which was not found in some collection. So this is the "correct" way to raise the exception here. The traceback call stack should theoretically give you all the info that was in the error message before. I noticed this because raise ValueError('foo') gives you a message "ValueError: foo" whereas raise KeyError('foo') gives you a message "KeyError: 'foo'" (note the quotation marks). I asked about it on the Python IRC channel and the above is the answer I got.

comment:14 Changed 7 years ago by ncohen

Well, for me this modification just makes the error message harder to understand, and I do not see the point of losing that if the only gain is to respect some standard we did not know even existed two weeks ago.

comment:15 Changed 7 years ago by kini

Hmm. OK, I'll just put back the messages and still use KeyError?. The quotation marks are a bit weird, but not such a big deal, I guess - we are not actually trying to catch these exceptions and do anything with them anyway.

comment:16 Changed 7 years ago by dcoudert

Hello, It would be nice if the KeyError? method could take an extra argument to describe the error. For instance KeyError?(key, 'You should add some tests to your algorithm!'). Unfortunately, I'm not able to find the documentation of the KeyError? function.

However, according to the documentation of Python, the raise method can handle multiple arguments: raise_stmt ::= "raise" [expression ["," expression ["," expression]]] See: http://docs.python.org/reference/simple_stmts.html#raise It may help satisfying both of you ;-)

Best, David.

comment:17 Changed 7 years ago by kini

Thank you, David! I agree with you, that would be great. Unfortunately it is not supported by Python. The documentation you linked about the raise statement is unfortunately not so helpful - there must be between 1 and 3 arguments, and they have special meanings. Actually, the multiple-argument syntax for raise is there in order to provide backwards compatibility with old versions of Python in which Exceptions were not objects with constructors. Now we would do something like

raise HALError("I can't let you do that, Dave")

but before you would write

raise HALError, "I can't let you do that, Dave"

because the HALError(foo) syntax didn't exist. So the multiple arguments basically allows you to still use the old syntax. Unfortunately this doesn't mean that we can just raise multiple exceptions...

Changed 7 years ago by kini

apply to $SAGE_ROOT/devel/sage

comment:18 Changed 7 years ago by kini

  • Status changed from needs_work to needs_review

New patch up. Sorry about all this noise, I should really have just made a different ticket for this exception stuff.

comment:19 Changed 7 years ago by ncohen

  • Reviewers changed from Keshav Kini, David Coudert to Keshav Kini, David Coudert, Nathann Cohen
  • Status changed from needs_review to positive_review

Hellooooo !!!

Well, thank you for this new version :-)

It passes all tests, so this ticket can go !

Nathann

comment:20 follow-up: Changed 7 years ago by kini

Hurrah! Thanks for the review (and the original patch)!

BTW, I botched that 2001 quote above... it should be "I'm sorry, Dave, I'm afraid I can't do that", of course :P

comment:21 in reply to: ↑ 20 Changed 7 years ago by ncohen

BTW, I botched that 2001 quote above... it should be "I'm sorry, Dave, I'm afraid I can't do that", of course :P

No harm done, we both watched the french version of it :-D

comment:22 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-4.8 to sage-5.0

Changed 7 years ago by jdemeyer

Use LookupError? instead of ValueError? or KeyError?

comment:23 Changed 7 years ago by jdemeyer

  • Authors changed from Nathann Cohen, Keshav Kini to Nathann Cohen, Keshav Kini, Jeroen Demeyer
  • Description modified (diff)
  • Status changed from positive_review to needs_work

Since KeyError is specifically about dicts, I think the more general LookupError is better. Needs review.

comment:24 Changed 7 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:25 Changed 7 years ago by kini

  • Milestone changed from sage-5.0 to sage-4.8

Oh, I was under the impression that those base classes shouldn't be instantiated, but that seems to be unfounded after reading the docs. Looks good to me! Lines 855-856 of $SAGE_ROOT/devel/sage/sage/graphs/base/graph_backends.py contains another exception which seems like it should be a LookupError but I'm not sure I understand why the current exception is imported from !NetworkX. Maybe there is a good reason for it?

comment:26 Changed 7 years ago by kini

  • Milestone changed from sage-4.8 to sage-5.0

Whoops, didn't mean to change the milestone.

comment:27 Changed 7 years ago by ncohen

Well, these lines are contained in the NetworkX backend, so it may mean that they define their own Exceptions in the NetworkX project. I do not think this class is still used in Sage anyway.

Nathann

comment:28 Changed 7 years ago by kini

  • Status changed from needs_review to positive_review

But didn't Sage developers write that NetworkX backend wrapper? Well, never mind, if it's not used in Sage anymore.

comment:29 follow-up: Changed 7 years ago by ncohen

Well yes, and that's most probably this class. But if you run some code made for NetworkX on a Sage graph with our own exception it may break the NetworkX code. I guess this was the point of importing exceptions. Thank you for the patch Jeroen !

Nathann

comment:30 in reply to: ↑ 29 Changed 7 years ago by kini

Replying to ncohen:

if you run some code made for NetworkX on a Sage graph

Aha, now I get it. And yes, thanks Jeroen :)

comment:31 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.0.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.