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

[Ben Woodard]

On Wed, 2003-12-10 at 08:55, Albert Chu wrote:
> Ben says he prefers having the marshalling functions malloc() a buffer
> instead of us passing a buffer in.  I personally prefer passing in a
> buffer and declaring a global macro IPMI_CMD_MAX_LEN.  We can debate
> about this.
> 

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.

-- 
Ben Woodard <address@hidden>
Red Hat Inc.