- Package:
- cdparanoia
- Source:
- cdparanoia
- Description:
- audio extraction tool for sampling CDs
- Submitter:
- Billy
- Date:
- 2010-11-10 00:03:03 UTC
- Severity:
- minor
There's a leak in cdda_find_a_cdrom().
A buffer is allocated via copystring() for each attempt
at filling the '?' wildcard in the cdrom_devices list.
This buffer is never freed. Also, the buffer is
allocated once per pattern attempt, which isn't really
necessary, so I changed that while I was in there.
I also changed the magic numbers 48 and 97 to
'0' and 'a' for clarity's sake.
diff -ru cdparanoia-3a9.8/interface/scan_devices.c cdparanoia/interface/scan_devices.c
--- cdparanoia-3a9.8/interface/scan_devices.c Wed Apr 21 11:41:25 2004
+++ cdparanoia/interface/scan_devices.c Wed Apr 21 11:51:49 2004
@@ -65,21 +65,16 @@
char *pos;
if((pos=strchr(cdrom_devices[i],'?'))){
int j;
- /* try first eight of each device */
- for(j=0;j<4;j++){
- char *buffer=copystring(cdrom_devices[i]);
-
+ /* try 0->4 and a->d for each device */
+ char *buffer=copystring(cdrom_devices[i]);
+ for(j=0;j<8;j++){
/* number, then letter */
-
- buffer[pos-(cdrom_devices[i])]=j+48;
- if((d=cdda_identify(buffer,messagedest,messages)))
- return(d);
- idmessage(messagedest,messages,"",NULL);
- buffer[pos-(cdrom_devices[i])]=j+97;
+ buffer[pos-(cdrom_devices[i])]=(j>>1)+(j&1)?'a':'0';
if((d=cdda_identify(buffer,messagedest,messages)))
return(d);
idmessage(messagedest,messages,"",NULL);
}
+ free(buffer);
}else{
/* Name. Go for it. */
if((d=cdda_identify(cdrom_devices[i],messagedest,messages)))
The patch had a precedence bug in the '?:'. Revised.
diff -ru cdparanoia-3a9.8/interface/scan_devices.c cdparanoia/interface/scan_devices.c
--- cdparanoia-3a9.8/interface/scan_devices.c Wed Apr 21 11:41:25 2004
+++ cdparanoia/interface/scan_devices.c Wed Apr 21 15:30:41 2004
@@ -65,21 +65,16 @@
char *pos;
if((pos=strchr(cdrom_devices[i],'?'))){
int j;
- /* try first eight of each device */
- for(j=0;j<4;j++){
- char *buffer=copystring(cdrom_devices[i]);
-
+ /* try 0->4 and a->d for each device */
+ char *buffer=copystring(cdrom_devices[i]);
+ for(j=0;j<8;j++){
/* number, then letter */
-
- buffer[pos-(cdrom_devices[i])]=j+48;
- if((d=cdda_identify(buffer,messagedest,messages)))
- return(d);
- idmessage(messagedest,messages,"",NULL);
- buffer[pos-(cdrom_devices[i])]=j+97;
+ buffer[pos-(cdrom_devices[i])]=(j>>1)+((j&1)?'a':'0');
if((d=cdda_identify(buffer,messagedest,messages)))
return(d);
idmessage(messagedest,messages,"",NULL);
}
+ free(buffer);
}else{
/* Name. Go for it. */
if((d=cdda_identify(cdrom_devices[i],messagedest,messages)))
Dear Billy, I am now reviewing the patches that we have pending in our bug tracking system and I am considering your patch right now. Unfortunately, it does not apply anymore against the tree that we are using, since the code has changed slightly. You can see our current tree at: http://git.debian.org/?p=pkg-opt-media/cdparanoia.git I still see that we have the use of the magic constants 48 and 97 instead of the characters 'a' and '0'. It would be nice to get rid of that and submit that to Chris Montgomery (upstream for cdparanoia---I am including him as a CC to this message). Thanks for your contribution, Rogério Brito.
Dear Billy, I am now reviewing the patches that we have pending in our bug tracking system and I am considering your patch right now. Unfortunately, it does not apply anymore against the tree that we are using, since the code has changed slightly. You can see our current tree at: http://git.debian.org/?p=pkg-opt-media/cdparanoia.git I still see that we have the use of the magic constants 48 and 97 instead of the characters 'a' and '0'. It would be nice to get rid of that and submit that to Chris Montgomery (upstream for cdparanoia---I am including him as a CC to this message). Thanks for your contribution, Rogério Brito.
Wow. This was a long time ago. 6 years! I'll look into updating it. -- Typed on a tiny virtual keyboard
updated patch
diff --git a/interface/scan_devices.c b/interface/scan_devices.c
index fc58110..07f928b 100644
--- a/interface/scan_devices.c
+++ b/interface/scan_devices.c
@@ -67,21 +67,20 @@ cdrom_drive *cdda_find_a_cdrom(int
messagedest,char **messages){
/* is it a name or a pattern? */
char *pos;
if((pos=strchr(cdrom_devices[i],'?'))){
+ const char suffixes[] = "0a1b2c3d";
int j;
- /* try first eight of each device */
- for(j=0;j<4;j++){
+ /* try each suffix for each device pattern */
+ for(j=0;suffixes[j];j++){
char *buffer=copystring(cdrom_devices[i]);
/* number, then letter */
- buffer[pos-(cdrom_devices[i])]=j+48;
- if((d=cdda_identify(buffer,messagedest,messages)))
- return(d);
- idmessage(messagedest,messages,"",NULL);
- buffer[pos-(cdrom_devices[i])]=j+97;
+ buffer[pos-(cdrom_devices[i])]=suffixes[j];
if((d=cdda_identify(buffer,messagedest,messages)))
return(d);
idmessage(messagedest,messages,"",NULL);
+
+ free(buffer);
}
}else{
/* Name. Go for it. */
It looks like gmail mangled that patch. I'll try attaching.
Hi, Billy. Well, the package had a "change in management" :) and I am trying to address some of the older bugs. Thank you very much for your updated patch. Due to some changes regarding the BSD's your patch didn't apply cleanly. Then, I took the liberty of breaking it in two parts: 1 - the plug for the memory leak; 2 - the optimized loop. Unfortunately, it seems that for the BSDs we can't use letters as suffixes and I have made the index that jumps from one suffix into the other be incremented by 2, instead of 1. As I have no BSD here at the moment, I am CC'ing the people at the debian-bsd list, so that they can test it. To -bsd: can you please test the cdparanoia package built including the commit ca62a7f in http://git.debian.org/?p=pkg-opt-media/cdparanoia.git ? BTW, Billy, leak well spotted. Regards,