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: 452     Entry time: 07 Mar 2008     Reply to this: 453   462
Author: Randolf Pohl 
Topic: Bug Report 
Subject: array overflows and other bugs 

I have just compiled MIDAS svn 4132 on a fresh SuSE 10.3 x86_64 system and gcc 
found a bunch of bugs, I guess.

> uname -a
Linux pc #1 SMP 2008/02/10 20:01:04 UTC x86_64 x86_64 
x86_64 GNU/Linux

> gcc --version
gcc (GCC) 4.2.1 (SUSE Linux)

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

But then there is a new one (in fact lots of this one), and brief
inspection suggests this is a true bug with the possibility of truly
nasty consequences.

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);

And midas.c line 7297:

So, if(i==MAX_DEFRAG_EVENTS) free(defrag_buffer[i]).
I guess this is an off-by-one bug.

src/midas.c:2958: warning: array subscript is above array bounds

   for (i = 0; i < 13; i++)
2958  if (trans_name[i].transition == transition)

Holy cow, hard-coded "13" in the code! Should be a #define, shouldn't it?

Now look at midas.c lines 94ff:
struct {
   int transition;
   char name[32];
} trans_name[] = {
   TR_START, "START",}, {
   TR_STOP, "STOP",}, {
   TR_PAUSE, "PAUSE",}, {
0, "",},};

There is no trans_name[12].

The trans_name[12] shows up in line 2894 and 2790, too.

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, 

   for (cmd = CMD_GET_FIRST; cmd <= CMD_GET_LAST; cmd++) {
311:  device_drv->mt_buffer->channel[current_channel].array[cmd] = value;

   for (cmd = CMD_GET_FIRST; cmd <= CMD_GET_LAST; cmd++) {
340:  device_drv->mt_buffer->channel[i].array[cmd] = value;

CMD_GET_DEMAND is in include/midas.h:
#define CMD_GET_DEMAND               CMD_GET_DIRECT  // = 20

I haven't even tried to understand mfe.c, nor did I read it. 
But I suspect the thing should always be something like
so array[] is indexed from [0], not from an arbitrary number that
depends on the number of commands you insert before line 698 in
midas.h. But please could the author of this check this very carefully?

src/lazylogger.c:1957: warning: array subscript is below array bounds

if ((channel < 0) && (lazyinfo[channel].hKey != 0))

That is lazyinfo[something below zero].

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?

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.

Oh, I got rid of the "dereferencing type-punned pointer" thing by adding
"-fno-strict-aliasing" as a compiler flag. Was suggested on the Web. Seemed to 
have worked during data taking (the data look reasonable :-). Is that a 
possible fix/workaround?


Attachment 1: out.0.make  40 kB  | Show | Show all
ELOG V3.1.4-2e1708b5