X-Authentication-Warning: delorie.com: mail set sender to geda-user-bounces using -f X-Recipient: geda-user AT delorie DOT com Date: Tue, 15 Mar 2016 07:12:37 +0100 (CET) X-X-Sender: igor2 AT igor2priv To: geda-user AT delorie DOT com X-Debug: to=geda-user AT delorie DOT com from="gedau AT igor2 DOT repo DOT hu" From: gedau AT igor2 DOT repo DOT hu Subject: Re: [geda-user] pcb: more memory leaks and potential buffer overrun report In-Reply-To: Message-ID: References: User-Agent: Alpine 2.00 (DEB 1167 2008-08-23) MIME-Version: 1.0 Content-Type: MULTIPART/MIXED; BOUNDARY="0-1094945186-1458022357=:7885" Reply-To: geda-user AT delorie DOT com Errors-To: nobody AT delorie DOT com X-Mailing-List: geda-user AT delorie DOT com X-Unsubscribes-To: listserv AT delorie DOT com Precedence: bulk This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --0-1094945186-1458022357=:7885 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Another batch: 0. Buffer overrun - report.c:ReportFoundPins(): there's a temp[64] in which element and pin names are written. Long names can make this buffer overrun and pcb segfault. If we are using dynamic strings already, we should string-append the names and use temp[] only for sprintf()'ing the integer. (attached segf1.pcb; r1297, but I used the new append-printf API; in mainline pcb it'd be a series of string/character append calls with DS) - report.c:ReportDialog(): report[2048] is a similar case: we hope the report text being printed won't be longer than 2k, but we don't guarantee our input is truncated. Using pcb_g_strdup_printf() would be more reasonable. (r1300; I use different pcb printf naming coneventions, but other than the function name the fix is the same) 1. leak: easy - report.c:ReportFoundPins() has a static dynamic string that tries to cache allocation but never frees the last one. I don't think this function is called often enough that this optimization worths. I'd remove static and modify it to free the string at the end of the call. (r1297, but I used gds instead of DS) - misc.c:EvaluateFilename() keeps a cache of the last file name buffer but in return strdup()'s the name on return. This means one at least allocation per invocation, while noone free()'s the last file name in static command. A simpler mechanism would be not to try to cache the buffer, but return it. This would not increase the number of allocations per invocation but would get rid of the "who free()'s the last" problem. (r1303). Callers will get simpler too, as they attempt to free the previous allocation all the time; in the new setup they free their current one (r1305) - misc.c:ExpandFilename(): not a real leak just misleading code: answer shouldn't be static, as its only allocated field is returned. When NULL is returned it should be free()'d, tho. The caller should also free the value returned. This is an example of the mechanism EvaluateFilename() could use. 2. leak: hard n/a 3. misc/off-topic - report.c:report_dialog_syntax should be "ReportObject()" as the action got renamed later. (r1299) - I couldn't find any code that uses ExpandFilename(). Are there 3rd party plugins depending on this function? Or is it superseded by EvaluateFilename()? --0-1094945186-1458022357=:7885 Content-Type: TEXT/PLAIN; charset=US-ASCII; name=segf1.pcb Content-Transfer-Encoding: BASE64 Content-ID: Content-Description: Content-Disposition: attachment; filename=segf1.pcb IyByZWxlYXNlOiBwY2Itcm5kIDEuMC44DQoNCiMgVG8gcmVhZCBwY2IgZmls ZXMsIHRoZSBwY2IgdmVyc2lvbiAob3IgdGhlIGdpdCBzb3VyY2UgZGF0ZSkg bXVzdCBiZSA+PSB0aGUgZmlsZSB2ZXJzaW9uDQpGaWxlVmVyc2lvblsyMDA3 MDQwN10NCg0KUENCWyIiIDYwMDAwMCA1MDAwMDBdDQoNCkdyaWRbMjUwMC4w IDAgMCAxXQ0KQ3Vyc29yWzE2NzUwMCAxMzc1MDAgMC4wMDAwMDBdDQpQb2x5 QXJlYVszMTAwLjAwNjIwMF0NClRoZXJtYWxbMC41MDAwMDBdDQpEUkNbMTIw MCA5MDAgMTAwMCA3MDAgMTUwMCAxMDAwXQ0KRmxhZ3MoIm5hbWVvbnBjYixj bGVhcm5ldyxzbmFwcGluIikNCkdyb3VwcygiMSwzLDQsYzoyLDUsNixzOjc6 OCIpDQpTdHlsZXNbIlNpZ25hbCwxMDAwLDc4NzQsMzE1MCwyMDAwOlBvd2Vy LDIwMDAsODY2MSwzOTM3LDIwMDA6RmF0LDgwMDAsMTM3ODAsNDcyNCwyNTAw OlNpZy10aWdodCwxMDAwLDY0MDAsMzE1MCwxMjAwIl0NCkF0dHJpYnV0ZSgi UENCOjpncmlkOjp1bml0IiAibWlsIikNCg0KRWxlbWVudFsiIiAiZGlwKDgp IiAiVGhpc19pc19hX3ZlcnlfbG9uZ19uYW1lX2Zvcl9teV9mYXZvcml0ZV9s aXR0bGVfZWxlbWVudF9qdXN0X3RvX2RlbW9uc3RyYXRlX2hvd19vdXJfYnVm ZmVyX2lzX3Rvb19zbWFsbF9UaGlzX2Nhbl9wb3RlbnRpYWxseV9jYXVzZV9h X25pY2Vfc2VnZmF1bHQiICI4KjMwMCIgMTk1MDAwIDE3NzUwMCAwIC0xMDAw MCAwIDEwMCAiIl0NCigNCglQaW5bMCAwIDgwMDAgNTAwMCA4NjAwIDM5Mzcg IiIgIjEiICJmb3VuZCxzcXVhcmUiXQ0KCVBpblszMDAwMCAwIDgwMDAgNTAw MCA4NjAwIDM5MzcgIiIgIjgiICIiXQ0KCVBpblswIDEwMDAwIDgwMDAgNTAw MCA4NjAwIDM5MzcgIiIgIjIiICIiXQ0KCVBpblszMDAwMCAxMDAwMCA4MDAw IDUwMDAgODYwMCAzOTM3ICIiICI3IiAiIl0NCglQaW5bMCAyMDAwMCA4MDAw IDUwMDAgODYwMCAzOTM3ICIiICIzIiAiIl0NCglQaW5bMzAwMDAgMjAwMDAg ODAwMCA1MDAwIDg2MDAgMzkzNyAiIiAiNiIgIiJdDQoJUGluWzAgMzAwMDAg ODAwMCA1MDAwIDg2MDAgMzkzNyAiIiAiNCIgIiJdDQoJUGluWzMwMDAwIDMw MDAwIDgwMDAgNTAwMCA4NjAwIDM5MzcgIiIgIjUiICIiXQ0KCUVsZW1lbnRM aW5lIFstNTAwMCAtNTAwMCAtNTAwMCAzNTAwMCAxMDAwXQ0KCUVsZW1lbnRM aW5lIFszNTAwMCAzNTAwMCAtNTAwMCAzNTAwMCAxMDAwXQ0KCUVsZW1lbnRM aW5lIFszNTAwMCAzNTAwMCAzNTAwMCAtNTAwMCAxMDAwXQ0KCUVsZW1lbnRM aW5lIFstNTAwMCAtNTAwMCAxMDAwMCAtNTAwMCAxMDAwXQ0KCUVsZW1lbnRM aW5lIFsyMDAwMCAtNTAwMCAzNTAwMCAtNTAwMCAxMDAwXQ0KCUVsZW1lbnRB cmMgWzE1MDAwIC01MDAwIDUwMDAgNTAwMCAwIDE4MCAxMDAwXQ0KDQoJKQ0K TGF5ZXIoMSAiY29tcG9uZW50IikNCigNCikNCkxheWVyKDIgInNvbGRlciIp DQooDQopDQpMYXllcigzICJjb21wLUdORCIpDQooDQopDQpMYXllcig0ICJj b21wLXBvd2VyIikNCigNCikNCkxheWVyKDUgInNvbGQtR05EIikNCigNCikN CkxheWVyKDYgInNvbGQtcG93ZXIiKQ0KKA0KKQ0KTGF5ZXIoNyAic2lnbmFs MyIpDQooDQopDQpMYXllcig4ICJvdXRsaW5lIikNCigNCikNCkxheWVyKDkg InNpbGsiKQ0KKA0KKQ0KTGF5ZXIoMTAgInNpbGsiKQ0KKA0KKQ0K --0-1094945186-1458022357=:7885--