MESSAGE
| DATE | 2004-04-20 |
| FROM | Billy
|
| SUBJECT | Re: [hangout] Debugging cdparanoia in C
|
On Tue, Apr 20, 2004 at 04:31:57PM -0400, Ruben I Safir wrote:
> It's funny, but even as rusty as I am in C and clueless in Unix
> functions in C, I'm still finding amature like mistakes in the
> code for cdparanioa
>
> Look at this snippet of code for example. This works, but it
> should be programed more correctly (unless I'm missing something).
>
> while(cdrom_devices[i]!=NULL){
>
> /* is it a name or a pattern? */
> 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]);
>
> /* 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;
> if((d=cdda_identify(buffer,messagedest,messages)))
> return(d);
> idmessage(messagedest,messages,"",NULL);
> }
>
>
> Breaking this function down -
>
> cdrom[] is an array of char pointers to possible cdrom devices
>
> if the array string has a '?' in it, then search for various devices by numbers and letter (such as /dev/hda1, /dev/hda2, ect)
>
> pos is returned the position for the '?' in the string
>
> from 0-4 then COPY the contents of the string in cdrom_device[i] into *buffer <<<=========== FOR EVERY TIME THROUGH THE LOOP.
> ***BOING****
>
> They SHOULD copy the string into the buffer FIRST. I'd imagine that
> the optimizer fixes this,
I don't think the optimizer will 'fix' it. There are too many side
effects to the copystring() function. (It's a malloc()) I just don't
think the optimizer can do stuff like that.
> but this is the kind of mistakes in coding which used to drive me
> NUTS. First, it adds a line into the loop. Second, your acking the
> system to do something over and over again which needs to be done only
> ONCE.
You're right.
> In a different situation, this kind of carelessness creates difficult
> to track bugs.
Actually, it has already masked a bug in this situation... :)
The bug is that buffer is malloc()'ed by copystring(). It isn't ever
free()'ed. If you run a daemon (let's say) that looks for
cdda_find_a_cdrom(), you might run out of memory. Real nice.
Even better, copystring() doesn't check the return of malloc(),
so the program wouldn't even know what hit it when memory
ran dry.
Also bothersome is the use of magic numbers 48 and 97
instead of '0' and 'a'. Come on!!
____________________________
NYLXS: New Yorker Free Software Users Scene
Fair Use -
because it's either fair use or useless....
NYLXS is a trademark of NYLXS, Inc
|
|
