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:	<alpine.DEB.2.00.1603130700180.7885@igor2priv>
Message-ID:	<alpine.DEB.2.00.1603150446310.7885@igor2priv>
References:	<alpine DOT DEB DOT 2 DOT 00 DOT 1603130700180 DOT 7885 AT igor2priv>
User-Agent:	Alpine 2.00 (DEB 1167 2008-08-23)
MIME-Version:	1.0
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

  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: <alpine DOT DEB DOT 2 DOT 00 DOT 1603150712370 DOT 7885 AT igor2priv>
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--

- Raw text -