Re: [llnl-devel] Re: [Freeipmi-devel] kcs byte array model - validation

[Anand Babu]

,----[ Ben Woodard <address@hidden> ]
| The reason that I prefer this is the function who really knows what
| size the thing is going to be on the wire is the marshal function. No
| one else knows exactly how big it has to be. So having it malloc the
| right sized buffer is clean and efficient.
|
| In the case where you pass in a buffer, there is no guarantee that the
| caller actually passed in a good buffer, and a buffer that is the
| right size. Even though you may have defined IPMI_CMD_MAX_LEN there is
| no way to enforce that people use it. In practice people often times
| do things like, "how big should this be. Well it can't be over 1KB
| so. char buf[1000];" They never even look for something like
| IPMI_CMD_MAX_LEN even if you put it in the documentation. The problem
| doesn't arise if the user wisely assumes something like 1000 (and
| waste gobs of space) but say for whatever reason they decide that they
| know that an ipmi packet is 64 bytes and so they make a 64 byte buffer
| but in some cases the buffer really needs to be 68 bytes.
|
| Plus if they do pass in too small of a buffer for the marshal function
| to work. How do they recover from the error. Most of the time
| programmers will not take the time to write something that recovers
| gracefully. They will write something like:
|
| char buf[60]; int len=marshall(&data,buf,buflen);
| write(fd,buf,buflen);
|
| So now the write is failing because it is getting a size of -1 and a
| programmer really has to look to find the reason why.
|
| Also, say they pass in a bogus pointer. When they try to debug the
| program it will initially look like it failed in your library and they
| will spend quite a while tracing through your code to figure out that
| the problem is actually in their program.
|
| Also I have seen at least one case where someone wrote some code where
| they made the packet buffer a file static variable in their kind of
| middle level library and forgot about it. Then they later on redid
| their program so that it was multithreaded. Every so often the program
| would break in weird ways and eventually I tracked it down to this
| global buffer being reused in a weird race condition. Sure it was a
| bug on their part and using a file static variable for something like
| a buffer for a packet isn't the wisest thing but many people do
| it. However, the fact that the lower level library didn't allocate its
| own memory provoked the problem.
|
| So those are my reasons for suggesting that the marshal function
| mallocs the buffer.
`----

I definitely agree with all of your points.


But my concern is,
+----------------------------------------------------------------------+
|  On most systems, `malloc' and `free' are not reentrant, because     |
|  they use a static data structure which records what memory blocks   |
|  are free.  As a result, no library functions that allocate or free  |
|  memory are reentrant.  This includes functions that allocate space  |
|  to store a result.                                                  |
+----------------------------------------------------------------------+

This may be true, with respect to signal handlers and call-back
procedures. But I'm skeptic about malloc usage inside library. Because
malloc, free uses static references inside, there is no way to
guarantee the buffer returned really belongs to the last call of the
current thread in a multi-threaded environment. I have seen some
references mentioning malloc is thread safe!. On all platforms and
implementations ?

Even for libc functions that allocate memory from inside are
alternated with functions prefixed with _r and take pointer 
from the user. Examples: strerror_r, readdir_r.

Now that we are going to hide the marshall function from user, we are
OK for now. But really curious to find a good solution for this
problem.

--
Anand Babu
CaliforniaDigital.com
Office# +1-510-687-7045
Cell# +1-510-396-0717
Home# +1-510-894-0586

Free as in Freedom <www.gnu.org>