OK, bug submitted to JIRA with patch :-) https://mariadb.atlassian.net/browse/MDEV-4212
On 25/02/13 12:38, Arjen Lentz wrote: > Hi Andrew > >> If the bug is in the new code in passing in a bogus data structure, I >> guess I was thinking, that table.cc seems to be in the core of MariaDB, >> and I was a little surprised something central could be crashed by a >> broken plugin. >> >> Anyway, would a patch to table.cc should go into the main code base in >> which case, how would I go about that? Or should I just fix in my tree >> now and it would get merged in the future with oqgraph? > > There are a few ways to go about this, but it shouldn't be mixed with oqgraph > patches/merges. > So, see this as a separate bug, as it could be triggered by other plugins > also (like you said). > > You could file a bug on MariaDB's Jira bug tracker, and include a patch > segment there or refer to a pushed branch that you have on Launchpad that > purely fixes that particular issue - you can even set a "merge request" on > that branch to its trunk in mainline MariaDB. > > I think that's the most proactive approach: "I identified a problem, and here > is my fix". Even if the fix is not liked, you've done your homework and made > an effort. In that case another approach is likely to get committed. > Otherwise, possibly your fix. So all good. > > >>> Crashing a multi-threaded/multi-user daemon is not nice. So >>> generally when something fails in MySQL, you have a mechanism to >>> return a fail from that function. >>> At the very least you'd have an assert in there (do add it), but >>> obviously you'd want to code to not get into that situation in the >>> first place >> >> Agreed on both :-) >> >> init_tmp_table_share() is void however, so I am not sure what the >> usual pattern is for MySQL/mariadb core code in these cases... > > Could be blunt and put an assert() in there, as obviously continuing with a > NULL ptr will result in a segfault anyway. Better to know and see it > identified clearly even if it's not neatly handled. > > InnoDB has some places like that in its code. > > >> This function would appear to be part of the mysql API and perhaps >> should be >> changed to return an error code if something unexpected happens - but >> who/how >> decides these things? For example, should it also check for key==NULL, >> although this condition wont crash that function? > > Practicality should decide, and sometimes time. > > Changing from void to int is relatively harmless, although a compiler will > chuck warnings to calls that don't void or use the return then, right? Well > that's ok too. You can suggest it. > > I recommend you get connected to the mariadb dev list - in your comment > field, say you're "working on oqgraph with Antony and Arjen and through there > have also become involved with some of the MariaDB code code and wish to > further contribute" or something like that. I think that's excellent! > > >> Another "odd" thing is this is C++ code with a bunch of classes, >> coexisting up with a bunch of plain root namespace functions; and >> table.cc is 6900 lines long... would it make sense to split this >> cc/h into multiple files one major class per each, etc? > > We jokingly refer to the MySQL/MariaDB source code as written in C+. > Code cleanups are awesome, but also very delicate issues even just from a > technical perspective - initially you'd have to make absolutely sure that > there are no functional changes, even stuff that seems harmless. > > Also there's still the consideration with merges from Oracle upstream, > although that might become less of an issue. > > For this, I'd say awesome you're interested and definitely something to look > in to further, but right now focus on the oqgraph codebase as a) we have full > control over it, b) there's a time factor there (we have to hurry up to get > it into MariaDB 10.0) and c) your activity will show up, so it provides > practical credit when working on core MariaDB later. All beneficial. > > Cheers, > Arjen. -- Mailing list: https://launchpad.net/~oqgraph-dev Post to : oqgraph-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~oqgraph-dev More help : https://help.launchpad.net/ListHelp
