A bit of C debugging

I have a customer who has a hodge-podge of machines running various software programs. It's all a mess, low on disk space, not every machine that should be backed up is backed up, and so on. I'm slowly working toward a better world on some of those things, but we still have to deal with the day to day problems.

One of those problems is their scheduling program. They schedule orders onto presses, track production, typical stuff. It's driven by an html front end and a bunch of shell, Perl, awk and C programs behind it. Kind of a mess. My intention is to rewrite the whole darn thing, but that is sometime in the future. For now we have to live with what we have.

I'd never looked at the code until yesterday when all orders disappeared from the presses. The screen would come up, but no orders were assigned to anything. Not very useful, so I dug in and found the section of code that read the orders in. I added some debugging, and that showed me that it was succesfully reading the orders. It then read a "press" file, which assigned orders to presses, and that was all fine too, or at least the various presses had orders assigned. The html wasn't showing them, but I could see that they were there.

The section of code responsible looks like this:

                while(fgets(buff,BUFF,fp)) {
                        buff[strlen(buff)-1] = '\0';
                        /*
                        printf("<!-- looking press %d  order %s -->\n",p,buff);
                        */
                        for(i=0; orders[i].name[0]; i++) {
                                printf("debug %d\n",i); /* added */
                                if(strcmp(orders[i].name,buff) == 0) {
                                        display(buff);
                                        orders[i].displayed = 1;
                                        count++;
                                }
                        }


It was the "for(i=0; orders[i].name[0]; i++) " loop that wasn't being executed, as though there were no orders. My "debug" printf never saw the light of day. But I knew from the read_order function that 488 orders had been read. So what gives?

The immediately suspicious part is that "orders[i].name[0];" in the middle. That's going to stop the loop if there's a null string. As I could see it never got into the loop at all, that would mean the very first member of the array was at fault. But it wasn't. I could see that it contained a valid string when read.

Ahh, yes, "when read". But years of hacking at my own and other people's code leads to the next question: what happens to the array after that? So I put this in at various places:

That produced

until just after a "load_flag_delete()" was called. After that, it showed:

There's my null.

I looked at "load_flag_delete" and it had absolutely no reference to the orders[] array. Since it obviously WAS writing to that array, that meant that it was overstepping bounds somewhere - it thinks it's still writing to a buffer of deleted items, but that's allocated too small, so it's writing elsewhere and munging the orders array.

OK. This should be easy to fix. The code is littered with arrays like "char buff[BUFF];"; obviously BUFF is too small. Let's check what it is; yep, it's in a ".h" file and it's defined as 256. What's the current count of deleted items being read? Why it's 261. There we are. Fix the .h, change BUFF to 1024 for now, recompile and..

Same problem. Huh? It has to be that. OK, yes it is 4:30 AM and I haven't had my coffee yet, but I'm not that groggy. The "load_flag_delete" is overwriting the orders array. What the heck?

Back to the code for a closer look. Aaargh! The furshluginner person who wrote this defined the buffer in "load_flag_delete" as "char buff[256]", not "char buff[BUFF]", and I hadn't noticed. Changed that, compiled, and the html now shows orders assigned to presses.

There's more to be done, of course. This is a horrid mix of junk. It must have been worked on by several people, because why would someone who knew Perl and C write an awk script that takes the absolute worst path through the orders to find matches for completed orders? They wouldn't, which means someone worked on that later, and all they knew was awk.

But, we play the cards we're dealt. I'll neaten this all up eventually, or try to convince the owners to replace it with one of several commercial apps that could do a far better job. I'd rather the latter, honestly. They should have a totally integrated system rather than this conglomeration.

6 comments

More Articles by Tony Lawrence - Find me on Google+

Click here to add your comments




Mon Oct 24 11:52:51 2005:   bruceg2004


"But, we play the cards we're dealt. I'll neaten this all up eventually, or try to convince the owners to replace it with one of several commercial apps that could do a far better job. I'd rather the latter, honestly. They should have a totally integrated system rather than this conglomeration."


That may be tougher than debugging the code. I wish you good luck on that venture. What commercial program would you recommend?

- Bruce



Mon Oct 24 12:35:27 2005:   TonyLawrence


I have another label making company that found software they like.. I don't recall the name, but I'll ask them.



Mon Oct 24 12:39:56 2005:   TonyLawrence


I'm pretty sure http://www.tailored.com/label_traxxmain.html is what my other client bought and is happy with. They did need a little customization if I remember correctly.



Mon Oct 31 14:32:09 2005:   Dean


Tony, I know this isn't your code, but why do people misuse strlen so much? Correct me if I'm wrong, but doesn't fgets read in AT MOST one less than "size". Which in this case is BUFF. Instead of calling strlen over and over, why not just:

 

memset(buf, 0, BUFF); 

while(fgets(buff,BUFF,fp)) { 

    // Remove that bloody strlen() 

Sorry, it just gets me sometimes... Maybe I'm guilty of it too.




Mon Oct 31 15:22:32 2005:   TonyLawrence


I dunno. Probably because their brain is moving in one direction and it just falls into it.





Mon Oct 31 18:50:16 2005:   Dean


Hey, much more logical than me getting all annoyed over it, probably... :)

Don't miss responses! Subscribe to Comments by RSS or by Email

Click here to add your comments

If you want a picture to show with your comment, go get a Gravatar

Jump to Comments

Many of the products and books I review are things I purchased for my own use. Some were given to me specifically for the purpose of reviewing them. I resell or can earn commissions from the sale of some of these items. Links within these pages may be affiliate links that pay me for referring you to them. That's mostly insignificant amounts of money; whenever it is not I have made my relationship plain. I also may own stock in companies mentioned here. If you have any question, please do feel free to contact me.

Specific links that take you to pages that allow you to purchase the item I reviewed are very likely to pay me a commission. Many of the books I review were given to me by the publishers specifically for the purpose of writing a review. These gifts and referral fees do not affect my opinions; I often give bad reviews anyway.

We use Google third-party advertising companies to serve ads when you visit our website. These companies may use information (not including your name, address, email address, or telephone number) about your visits to this and other websites in order to provide advertisements about goods and services of interest to you. If you would like more information about this practice and to know your choices about not having this information used by these companies, click here.