Back Midas Rome Roody Rootana
  Midas DAQ System  Not logged in ELOG logo
Entry  07 Mar 2008, Randolf Pohl, Bug Report, array overflows and other bugs out.0.make
    Reply  07 Mar 2008, Stefan Ritt, Bug Report, array overflows and other bugs 
    Reply  10 Mar 2008, Stefan Ritt, Bug Report, array overflows and other bugs 
Message ID: 462     Entry time: 10 Mar 2008     In reply to: 452
Author: Stefan Ritt 
Topic: Bug Report 
Subject: array overflows and other bugs 
There were some trivial and some non-trivial issues. Glad the compiled picked up on
this!

> I see loads of warnings during compile, most of which I know from earlier 
> compiles:
> * warning: dereferencing type-punned pointer will break strict-aliasing rules
> * warning: pointer targets in passing argument 3 of 'getsockname' differ in
>            signedness

I ignore these for the moment until I have a gcc 4.2 myself (we use Scientific
Linux 5 which has gcc 4.1 for the moment). As Randolph pointed out correctly you
can make gcc shut up by a proper flag there. The warnings have no influence on the
stability of midas.

> (1)=========================
> src/midas.c:7398: warning: array subscript is above array bounds
> Inspection of midas.c:
> 
>    if (i == MAX_DEFRAG_EVENTS) {
>       /* no buffer available -> no first fragment received */
> 7398: free(defrag_buffer[i].pevent);
>       memset(&defrag_buffer[i].event_id, 0, sizeof(EVENT_DEFRAG_BUFFER));
>       cm_msg(MERROR, "bm_defragement_event",
>              "Received fragment without first fragment (ID %d) Ser#:%d",
>              pevent->event_id & 0x0FFF, pevent->serial_number);
>       return;
>    }

The free() was just wrong at that place, I removed it.

> (2)==========================
> src/midas.c:2958: warning: array subscript is above array bounds
> 
>    for (i = 0; i < 13; i++)
> 2958  if (trans_name[i].transition == transition)
>          break;

Fixed that by
     
     for (i=0 ;; i++)
        if (trans_name[i].name[0] == 0 || trans_name[i].transition == transition)
           break;

Since trans_name[i].name = "" indicates the end of the list.

> (3)=============================
> mfe.c:
> src/mfe.c:412: warning: array subscript is above array bounds
> src/mfe.c:311: warning: array subscript is above array bounds
> src/mfe.c:340: warning: array subscript is above array bounds
> 
> 412: device_drv->dd(CMD_GET_DEMAND, device_drv->dd_info, i, 
>           &device_drv->mt_buffer->channel[i].array[CMD_GET_DEMAND]);

The code at 412 was wrong there, the demand value is queried later by the device
driver directly. For the other two occurences (311 and 340) I had to really
increase the array size by one. This issue can cause segfaults if you have a slow
control front-end which uses multithreading (not many people use it except me).

> (4)=========================
> src/lazylogger.c:1957: warning: array subscript is below array bounds
> 
> if ((channel < 0) && (lazyinfo[channel].hKey != 0))
> 
> That is lazyinfo[something below zero].

This has to be fixed by Pierre. I guess an or instead of an and would do it, but
I'm not 100% sure.

> (5)=============================
> More warnings an expert might want to have a look at:
> 
> * warning: deprecated conversion from string constant to 'char*'
> 
> * src/fal.c:106: warning: non-local variable '<anonymous struct> out_info'
>                  uses anonymous type
> * src/fal.c:3064: warning: non-local variable '<anonymous struct> eb' uses
>                   anonymous type
> 
> I attach the full output of make.
> Could someone knowledgeable please have a look at these warnings and fix them?

Uahhh. Especially the "const char*" vs. "char*" is in principle right, but will
cause a major rework. Probably hundreds of occations have to be fixed. Many strings
must be declared const, others not. It will help the programmer to find some errors
during compile which would later show up only during runtime (like writing into a
fixed string), but I only will go through that when I have gcc 4.2 installed
myself, and have two free days to work on this ;-)

> They make me a bit nervous when thinking about data integrity, and
> there are now so many that they actually start to hide serious stuff
> like the ones I presented.

Except the slow control stuff (which only is an issue for multithreaded frontends)
none of the above things will have an influence on the data integrity. But I agree
that they should be fixed.

- Stefan
ELOG V3.1.4-2e1708b5