I see that MIDAS installs several set-uid-root programs into /usr/local/bin.
In this age and time of evil computer hackers, this is not a good idea and
we should Do Something (TM) about it. Here is my risk assessment:
[olchansk@midtis06 midas]$ ls -l /usr/local/bin | grep wsr
-rwsr-sr-x 1 root root 25811 Nov 20 09:27 dio
-rwsr-sr-x 1 root root 344553 Nov 20 09:27 mhttpd
-rwsr-sr-x 1 root root 70736 Nov 20 09:27 webpaw
dio- is required to be setuid-root to gain I/O permissions. I looked at it a
few times, and it is probably safe, but I would like to get a second
opinion. Stephan, can you should it to your local security geeks?
mhttpd- definitely unsafe. It has more buffer overflows than I can shake a
stick at. Why is it suid-root anyway?
webpaw- what is it?!?
> While reviving midas on midtig01 after it was not used for a while ...
> [local:tigress:S]/>scl -w
> Name Host Timeout Last called
> mhttpd midtig01.triumf.ca 10000 -2037131082
These clients cannot be deleted. I tried:
1) shutdown from mhttpd "programs" page -> "cannot shutdown client"
2) "sh mhttpd" from odbedit ->
[midas.c:5298:cm_shutdown] cannot connect to client mhttpd on host
midtig01.triumf.ca, port 32853
Client mhttpd not active
3) in odbedit: "cd /system/clients; rm xxxx"
refuses to delete the key
Lacking any better ideas, I deleted them via brain surgery on the odb file:
1) stop everything
2) ipcrm the SYSV shared memory segment
3) odbedit -> save xxx.odb
4) xemacs xxx.odb, delete offending odb entries
5) rm .ODB.SHM
6) odbedit -> load xxx.odb
7) voila, bad clients gone, gone, gone.
While reviving midas on midtig01 after it was not used for a while, we see
this. Notice negative "last called" numbers. Looks like a time_t wraparound
Name Host Timeout Last called
mhttpd midtig01.triumf.ca 10000 -2037131082
Logger midtig01.triumf.ca 10000 -2037131166
Analyzer midtig01.triumf.ca 10000 -2037131048
JACQ midtig01.triumf.ca 10000 -2037131667
mhttpd1 midtig01.triumf.ca 10000 325
ODBEdit midtig01.triumf.ca 10000 829
I tried to reproduce the problem, but without success. So in case this happens
again, one should debug the code im cm_watchdog() next to the line
/* decrement notify_count for open records and clear exclusive mode */
So if a killed client is removed from the ODB via the watchdog (or a "cleanup"
is done in ODBEdit), the notify_count should be decreased and thus the "open
records" should be closed.
As Konstantin pointed out correctly, the db_create_record() call is pretty
heavy since it copies whole structures around the ODB. Therefore, it
should not used frequently. It might be that several problems are caused
by that, for example the "phantom" records reported in elog:40 .
I have therefore implemented the function
db_check_record(HNDLE hDB, HNDLE hKey, char *keyname, char *rec_str,
which takes an ASCII structure in the same way as db_create_record(), but
only checks this ASCII structure against the ODB contents without writing
anything to the ODB.
If the record does not exist at all, it is created via db_create_record().
This is useful for example with the /Runinfo structure on a virgin ODB.
If the parameter "correct" is FALSE, the function returns
DB_STRUCT_MISMATCH if the ODB contents is wrong (wrong order of variables,
wrong name of variables, wrong type or array size). The calling function
should then abort, since a subsequent db_open_record() would fail. Note
that although abort() is useful, one should add cm_disconnect_experiment()
just before the abort() in order to have the application "log out" from
the ODB gracefully. If the parameter "correct" is TRUE, the function
db_create_record() is called internally to correct a mismatching record.
I have changed most calls of db_create_record() in mhttpd.c, mfe.c, mana.c
and mlogger.c. Pierre, could you do the same for lazylogger.c?
I also started to put assert()'s everywhere and encourage everyone to
follow. Under Windows, the asserts() are removed automatically if
compiling in "Release" mode.
So I committed many changes, did some quick tests, but am not 100%
convinced that all the changes are good. So please use the new code
cautiously, and let me know if there is any new problem. I also would like
to get some feedback if the whole thing becomes more stable now.
- Remove temporary "/Programs/Lazy" creation.
- Fix Rate calculation for Web display.
- Change FTP channel description (see help).
Let me propose a revised scheme for midas standard VME calls (mvmestd.h).
Pierre mentioned some limitations before, and I find now also some fields
to improve. Right now, the vme_open() call retrieves a handle. For some
interfaces (like SBS/Bit3), one has to obtain separate handles for
different addressing modes A24D32/A32D32 and so on, which I find a bit
troublesome. I would rather keep the handle internally, invisible to the
user, and use ioctl() statments to change the address/data mode.
So the API could look like:
vme_open() Deprecated, will be removed
vme_init(void) Standard initialization, open device(s), stores handles
internally in a table
vme_exit(void) Deallocates any memory, close handles
vme_read(void *dst, DWORD vme_addr, DWORD size)
vme_write(void *src, DWORD vme_addr, DWORD size)
vme_ioctl(int request, int *param)
Request is one of
Sets VME crate (in case several interfaces are
plugged into singlePC, meaningless for embedded CPUs)
VME_BUS/VME_RAM/VME_LM for VME bus, RAM in VME
interface, or LM for local memory (used in Bit3
Sets/Retrieves VME AMOD (= VME_AMOD_xxx as currently
defined in mvmestd.h)
Sets/Retrieves VME data size (D8/D16/D32/D64)
Enable/Disable DMA, should be independent of AMOD
Set VME interrupts
Set autoincremet of source pointer, can be disabled
for FIFO readout
vme_mmap(void **ptr, DWORD vme_addr, DWORD size)
vme_unmap(void *ptr, DWORD size)
Map/Unmap VME to local memory
vme_read2(void *dst, DWORD vme_addr, DWORD size, DWORD flags)
vme_write2(void *src, DWORD vme_addr, DWORD size, DWORD flags)
With these functions one can directly specify the flags
usually managed by vme_ioctl(). Usefule for applications
where the address modifier for example has to be
different in each read/write operation.
Note that the vme_read/write functions do not have a VME handle any more,
nor an address modifier. This is all accomplished with vme_ioctl() calls.
Please have a look at this proposal, compare it with what you do currently
in VME, and let me know if we should add/modify something. I volunteer to
implement the API for the SBS/Bit3 617 and the Struck SIS1100/3100
interfaces, for VxWorks somebody at TRIUMF should take care.
I have seen the same behaviour and it annoys me, too. What I did in the past
is a "cleanup" in ODBEdit which removes these open records. I have soem code
in cm_watchdog(), which should take care of that. If a client is dead, it
gets removed from the ODB, and its open records should get its notify_count
decremented. So obviously this code has some bug. I plan to do in the
following week (now I got some spare time) the following:
- exchange most db_create_record() by something better. Maybe
db_check_record(..., correct_flag), which creates the record only if it does
not exist at all, otherwise checks the structure. If correct_flag is TRUE, it
corrects the strucure (by calling db_create_record()), if it's false it just
returns an error code. This way one can decide from case to case which option
is better. Like for the /Runinfo, the flag would be FALSE, maybe with a
notification that the /Runinfo is different from the compiled-in structure,
and one hast to recompile the application.
- revisit the open record issue from dying frontends. I remember vaguely that
I tried to kill a frontend (kill -9), wait until the watchdog cleans up its
entries, and it worked fine. So it's more the problem to reproduce the issue
described in the previous elog entry.
Sometimes (maybe after a client uncleanly exits?), I see phantom "open
records", for example:
/Equipment/Gas/Common open 2 times by fe1hp
/Equipment/Gas/Variables open 1 times by Logger
/Equipment/Gas/Variables/Flow1 open 2 times by uBeamTcl1 uBeamTcl
/Equipment/Gas/Settings/Command open 2 times by fe1hp
/Equipment/Gas/Statistics open 1 times by
Note the blank client name in the "/Equipment/Gas/Statistics" line.
This causes these warnings from mfe.c:
Cannot init equipment record, probably other FE is using it
Cannot delete statistics record, error 320
Cannot create statistics record, error 320
Cannot open statistics record, error 318. Probably other FE is using it
Then the number of generated events for this front end is never incremented.
Also attempts to delete this "open" record fail:
Are you sure to delete the key
and all its subkeys? (y/[n]) y
key is open by other client
How do I go about writing the db_validate_xxx() code to cleanup this
bogosity? I am not too familiar with the implementation of "open record"...
Ok, I apologize. It's all ok. Thanks for clearifying. Concerning the assert's, it
would be nice to be able to disable them in release code. Under Windows, the
assert() is actually a macro which expands to zero if NDEBUG is defined. I
believe it's the same under linux, but I don't know about VxWorks. So we have
1) Keep asserts always. This might possible slow down a DAQ system, but I'm not
sure how much. Might be negligible.
2) Disable asserts by default (standard make). Only the "experts" can enable it
in the make file (by removing NDEBUG), since only they know what to do with the
3) Let the user decide on the standard installation. Maybe have two libraries,
one debug, one no-debug. The no-debug can even have the compiler optimization
disabled, which makes debugging easier.
So what is your opinion (comments from others are welcome as well) of which way
> > I found where we tickle the race condition in db_create_record().
> The reason for the db_create_record() is the following: Assume that we change
> the /runinfo structure...
I think there is a deep fundamental problem with changing data structures "on the
fly". Calling db_create_record("/runinfo") at every show_status_page() does not
If I change the runinfo structure, rebuild, relink and restart "mhttpd", the
db_create_record("/runinfo") from cm_connect_experiment() will update the runinfo
structure in ODB. In this case, the call from show_status_page() is redundant. As
a side effect, when we do this, we break every running ODB client- they still
have the old runinfo layout. Not good...
If I change the runinfo structure, rebuild, relink and restart all applications,
*except* for mhttpd, "/runinfo" in ODB will be updated when the first updated
client connects to ODB via the db_create_record("/runinfo") from
cm_connect_experiment(). Then, the old mhttpd will restore the old layout via the
db_create_record("/runinfo") in show_status_page(), breaking everything. Not good...
If I change the runinfo structure, rebuild, relink and restart everything,
"/runinfo" in ODB will be updated when the first client connects to ODB via the
db_create_record("/runinfo") from cm_connect_experiment(). In this case, the call
from show_status_page() is redundant. This is the only corruption-free scenario.
This lack of integrity enforcement vs version skew in binary data structures is,
I think, an ODB design error. Perhaps, ODB applications should be prohibited from
direct access to ODB "C" data structures: we cannot ensure that the data layout
in the application and in ODB are the same.
> One could think of checking the record size, and re-creating the runinfo if
> the ODB record size does not match the C record size. But this does not
> prevent the potential error that some variable are reversed in order. They
> are then mapped wrongly to the C runinfo structure.
> I see that you work very hard now on all possible checks for the run number.
> But I would not commit that and make it part of the distribution...
This is a philosophical issue.
My checks are in line with the "design by contract" school of programming. In a
nutshell, this ideology requires that before I do anything, I should enforce the
validity of my inputs and after I am done, I should enforce the validity of my
outputs. In practice, this translates into liberal use of assert()'s *in
To ensure that old bugs stay fixed, and that new bugs are promptly discovered, it
is essential that the "contract checks" stay in the production code forever.
But let better writers argue programming philosophy in the literature.
Personally, when hunting down bugs in unstable code, I find this technique to be
vastly superior to the more common appoach of "This program has no bugs. Error
checking and assert()s are wasteful. Let's close our eyes and hope no bad things
happen to us (again)".
> But if you start now, please put [asserts] in all other 100000 places (;-)
I know that no good deed goes unpunished, but pewleeze!!!
> If you cannot resolve your zero run number problem, do the following: ...
> [lock ODB, freeze the experiment, look at log files]
This technique is obsolete. Today, we instrument the code with sanity checks
and validity tests. Then all the bugs find themselves with minimal manual
> > Stephan, why did you prohibit building mana.c without ROOT and HBOOK
> > support? I think such a configuration is valid and should be allowed.
> Oops, sorry, my fault. I forgto that people use mana.c without ROOT and
> HBOOK. The reason I made the change was that people forgot the -DHVAE_HBOOK
> in their makefile. In that case, no HBOOK init is done in mana.c and the
> first histogram booking in the user code crashes HBOOK.
Ahem. There is only so much rope we can give out to prevent people from shooting
themselves in the foot...
> So please take the #error statement out of mana.c
> One possibility is that we put an additional layer on top of the histogram
> boooking/filling. These macros are converted to their HBOOK or ROOT
> equivalents depending on the HAVE_HBOOK/HAVE_ROOT. If none of both is
> present, the histogram booking macro can produce a runtime error. This has
> the additional advantage that users can switch from HBOOK to ROOT without
> change of their user code.
I can't think of anything other than wrapping every HBOOK call with "if
(!hbook_is_initialized) initialize_hbook();". But then, where is PAWC
coming from anyway?!?
We could also print a warning message "This mana.c has no HBOOK support. If you
see HBOOK crashes, please relink with hmana,c". Ugly, but informative, plus it
points anybody who knows how to read towards a solution.
> > I added error checking to the places where we read "/runinfo/run number".
> Now YOU broke the system by editing all these files with something I consider
> temporary debugging code. A run number of zero is *VALILD*.
I think I broke nothing. I do know that run number 0 is a valid odb value. Here
is an audit of all places where I abort on invalid run numbers:
mana.c: line 3676: assert(current_run_number > 0);
we take the run number from an event and write it into ODB. Events cannot have
run number negative or zero.
mana.c:analyze_run(): line 4632: assert(run_number > 0);
we are asked to analyze run "run_number". zero or negative is not valid.
midas.c:assert(run_number > old_run_number);
midas.c:assert(run_number > 1);
this code is not in CVS.
odbedit.c: line 2563: assert(old_run_number >= 0);
run number zero is valid
odbedit.c: line 2641: assert(new_run_number > 0);
starting a new run number zero is not valid
mfe.c: line 1786: if (run_number<=0) cm_msg(MERROR, "main", "aborting on attempt
to use invalid run number %d", run_number);
auto restart from run 0 to 1 is not valid
midas.c: line 3917: if (run_number<=0) cm_msg(MERROR, "cm_transition", "aborting
on attempt to use invalid run number %d",run_number);
transition to run zero or negative is not valid
midas.c: line 16101: if (run_number<0) cm_msg(MERROR, "el_submit", "aborting on
attempt to use invalid run number %d", run_number);
negative run numbers are not valid
mlogger.c: line 3301: if (run_number<=0) cm_msg(MERROR, "main", "aborting on
attempt to use invalid run number %d", run_number);
auto restart from run 0 to run 1 is not valid
> I added error checking to the places where we read "/runinfo/run number". In
> general, I do this:
> Affected files:
Now YOU broke the system by editing all these files with something I consider
temporary debugging code. A run number of zero is *VALILD*. If I want to make
sure a new experiment starts with run number #1, I put a run number of 0 into
the ODB. So on the first start the number is incremented by one which results
in run number from one. So please remove those checks which prevents me of
doing that. Again, your "run number zero" problem is soemhow specific to your
environment, and I would not put all these tests into the distribution,
because this can have side effects, like that one I described above.
> I found where we tickle the race condition in db_create_record().
> 1) in mhttpd.c, every time we show the status page, we call
> db_create_record(hDB, 0, "/Runinfo", strcomb(runinfo_str));
> 2) internally db_create_record() deletes /RunInfo
> 3) other programs read "/runinfo/run number" while it is deleted do not
> check for the db_get_value() error code and happily get a zero run number.
> Stephan fixed the race condition, and now I commited an mhttpd.c change that
> only calls db_create_record(hDB, 0, "/Runinfo", strcomb(runinfo_str)); if
> /runinfo does not exist. This seems to be redundant with a similar call in
> cm_connect_experiment1(), called each time a new client starts up.
The reason for the db_create_record() is the following: Assume that we change
the /runinfo structure, by adding an additional variable in the future. If we
run a "new" mhttpd on an "old" experiment, the "runinfo" C structure does not
match the ODB contents. The db_create_record() ensures that the ODB structure
exactly matches the C structure. I agree with you that this can cause
potential problems. But most of them should be fixed by the additional lock()
I added recently. So other programs cannot read the run number while it is
One could think of checking the record size, and re-creating the runinfo if
the ODB record size does not match the C record size. But this does not
prevent the potential error that some variable are reversed in order. They
are then mapped wrongly to the C runinfo structure.
I see that you work very hard now on all possible checks for the run number.
But I would not commit that and make it part of the distribution, since all
experiments at PSI for example do not have this run number problem. Run it
locally, determine the cause of your problem (the discovery of the race
condition was already very good, I'm glad that your found it, should make the
system much more stable), and we'll fix it. Puttin ASSERT's is a good idea, I
should have done it from the very beginning. But if you start now, please put
it in all other 100000 places (;-)
I would not add a db_get_value_cannot_possibly_fail() into the standard
distribution, because it probably cannot correct the initial problem and then
just will go into an infinite loop. We should tackle problems always at their
If you cannot resolve your zero run number problem, do the following: There
is a cm_msg(MDEBUG, ...) which only puts a message into the shared memory,
but not in midas.log. This can be used for real time debugging. Add those
message temporarily in db_get_value() etc. to see what is going on. As soon
as the run number goes to zero, stop all processes immediately (for example
by locking the database with db_lock_database), and the look backwards in the
sysmsg buffer to see what happened *before* the run number went to zero.
> Stephan, why did you prohibit building mana.c without ROOT and HBOOK
> support? I think such a configuration is valid and should be allowed.
Oops, sorry, my fault. I forgto that people use mana.c without ROOT and
HBOOK. The reason I made the change was that people forgot the -DHVAE_HBOOK
in their makefile. In that case, no HBOOK init is done in mana.c and the
first histogram booking in the user code crashes HBOOK.
So please take the #error statement out of mana.c (I'm away in two hours for
one week), but think about preventing the above mentionend problem. I don't
know any way for the makefile or mana.c to figure out if there is any HF1
call in the user code. Actually HF1 should return a "proper" error message
than just crashing.
One possibility is that we put an additional layer on top of the histogram
boooking/filling. These macros are converted to their HBOOK or ROOT
equivalents depending on the HAVE_HBOOK/HAVE_ROOT. If none of both is
present, the histogram booking macro can produce a runtime error. This has
the additional advantage that users can switch from HBOOK to ROOT without
change of their user code.
I found where we tickle the race condition in db_create_record().
1) in mhttpd.c, every time we show the status page, we call
db_create_record(hDB, 0, "/Runinfo", strcomb(runinfo_str));
2) internally db_create_record() deletes /RunInfo
3) other programs read "/runinfo/run number" while it is deleted do not
check for the db_get_value() error code and happily get a zero run number.
Stephan fixed the race condition, and now I commited an mhttpd.c change that
only calls db_create_record(hDB, 0, "/Runinfo", strcomb(runinfo_str)); if
/runinfo does not exist. This seems to be redundant with a similar call in
cm_connect_experiment1(), called each time a new client starts up.
I added error checking to the places where we read "/runinfo/run number". In
general, I do this:
status = db_get_value("/runinfo/run number",&run_number);
assert(run_number >= 0); (and run_number>0, where appropriate)
Here is the rationale: if we cannot read the run number, something must be
very terribly wrong. I cannot think of any recovery action other than
abort() and make a core dump for our debugging enjoyment.
I considered and rejected adding a "retry" loop: if we allow db_get_value()
to intermittently fail, then it's every use has to be wrapped in a retry
loop, which then should be inside db_get_value(), making it pointless to
have external "retry" loops.
I am now pondering on proposing a "db_get_value_cannot_possibly_fail()"
function (it would abort(), exit() with an error or commit harakiri if it
can't get the value). They way most db_xxx() functions are used in midas,
maybe they should be made "void" and "unfailible", with "STATUS
db_xxx_yes_I_can_fail_and_return_an_error_code()" evil twins. I guess this
is why "they" invented C/C++ exceptions. Anyway, something to think about.
The default C indentation style in xemacs uses "tab" characters, violating
the MIDAS coding convention. To disable this misfeature in xemacs (emacs
too?), put this incantation in your .xemacs/custom.el file:
Stephan, why did you prohibit building mana.c without ROOT and HBOOK
support? I think such a configuration is valid and should be allowed.
Also, this prohibition broke the Midas Makefile, it now bombs building
mana.c. The Makefile is setup for building hmana.c with HBOOK support,
rmana.c with ROOT support (if ROOTSYS is set) and mana.c without HBOOK and
ROOT support (currently bombs on #error in mana.c).