{"action":"opened","issue":{"url":"https://api.github.com/repos/rathena/rathena/issues/583","labels_url":"https://api.github.com/repos/rathena/rathena/issues/583/labels{/name}","comments_url":"https://api.github.com/repos/rathena/rathena/issues/583/comments","events_url":"https://api.github.com/repos/rathena/rathena/issues/583/events","html_url":"https://github.com/rathena/rathena/issues/583","id":101767379,"number":583,"title":"faulty usage of sg->unit in land skill code","user":{"login":"theultramage","id":1738190,"avatar_url":"https://avatars.githubusercontent.com/u/1738190?v=3","gravatar_id":"","url":"https://api.github.com/users/theultramage","html_url":"https://github.com/theultramage","followers_url":"https://api.github.com/users/theultramage/followers","following_url":"https://api.github.com/users/theultramage/following{/other_user}","gists_url":"https://api.github.com/users/theultramage/gists{/gist_id}","starred_url":"https://api.github.com/users/theultramage/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/theultramage/subscriptions","organizations_url":"https://api.github.com/users/theultramage/orgs","repos_url":"https://api.github.com/users/theultramage/repos","events_url":"https://api.github.com/users/theultramage/events{/privacy}","received_events_url":"https://api.github.com/users/theultramage/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":0,"created_at":"2015-08-18T22:52:09Z","updated_at":"2015-08-18T22:52:09Z","closed_at":null,"body":"A skill_unit_group is a container for a cluster of related skill_unit game objects. It holds pointers to them in an allocated array called 'sg->unit[]' and is 'sg->unit_count' fields long. Some of these units are alive and some may have been deleted; this is indicated by an 'alive' boolean.\r\n\r\nI found multiple cases of rA's skill.c using the sequence 'sg->unit->bl' (and one 'sg->unit->range').\r\nThere is so much wrong with this:\r\n* unit->bl implicitly evaluates to unit[0]->bl; do not use this short form, it's a really bad idea.\r\n* unit[0] is probably not intended, it's likely that the author didn't realize 'unit' is an array.\r\n* there is no guarantee that unit[0] is alive, in which case 'bl' and some other fields are invalidated.\r\n* all offending code takes an 'unit' as an input variable and does 'sg = unit->group'; in this situation, 'sg->unit' effectively means 'unit->group->unit', a roundabout (and faulty) way of saying 'unit'.\r\n\r\nThe land skill code always was and still is a minefield, and a long-term source of confusing bugs. eA skill code does not touch sg->unit[], except in one specific function, and for good reason. Pay attention to what you're doing there. In particular, you guys really seem to like copy-pasting that 'battle_check_target' line and propagating the mistake further."}}