problem with cgiutils.c on solaris 10?

[Ethan Galstad]

Bob -

Thanks for the notes!  Comments are inline...

Bob Ingraham wrote:
> The pointer arithmetic looks OK, but three things come to mind after
> looking at the mmap_fgets function:
> 
> 1. Potential memory-alignment-access issue
> 
> When I used to write for Solaris on SPARC platforms, I had to be careful
> about non-word-aligned memory accesses, since they would cause SIGBUS
> violations.
> 
> However, Brian's box looks like it's Solaris on x86 (seeing the cl and esi
> register names...), so I'm not so sure that it applies, since x86 allows
> non-word-aligned memory access.

I've never developed on a platform that had word-aligned memory 
restrictions, so I can't say for sure whether or not this is a problem. 
  Seems like a total pain to have to work around it, but that's just my 
opinion. :-)

> 
> 
> 2. Potential bug if last "line" in the file isn't new-line terminated.
> 
> I suppose that this is unlikely, but if it occurred, then the for-loop
> would increment the x variable one byte past the file_size.  Then the
> length calculation would be off and the memcpy would try to access one
> byte beyond the mapped file space.

Yep, definite bug.  The version of mmap_fgets() in utils.c (Nagios 
daemon) had a fix for this a while back, but I must have forgotten to 
apply it to cgiutils.c as well.

> 
> 
> 3. The parenthesis grouping for type-casting isn't quite
> right/paranoid-enough (for me, anyway)
> 
> For example, the following line:
> 
>     if(*(char *)(temp_mmapfile->mmap_buf+x)=='\n')
> 
> should really be:
> 
>     if(*((char *)(temp_mmapfile->mmap_buf)+x)=='\n')
> 
> 
> And,
> 
>     len=(int)x-temp_mmapfile->current_position+1;
> 
> should be:
> 
>     len=(int)(x-temp_mmapfile->current_position)+1;
> 
> 
> And,
> 
>     memcpy(buf,(char
> *)(temp_mmapfile->mmap_buf+temp_mmapfile->current_position),len);
> 
> should be:
> 
>     memcpy(buf,((char
> *)(temp_mmapfile->mmap_buf)+temp_mmapfile->current_position),len);


Sounds good.  I've applied patches to correct this and item #2 in CVS.

> 
> 
> Also, Brian, if you have the time, it would be helpful to see a couple of
> other things in your stack trace:
> 
> - A dump of the values of the temp_mmapfile member variables.
> 
> - A dump of the values of the local variables: buf, len and x
> 
> Just my $0.02.
> 
> Bob
> 


Ethan Galstad,
Nagios Developer
---
Email: nagios at nagios.org
Website: http://www.nagios.org


-------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642