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: [email protected]
Website: http://www.nagios.org
This post was automatically imported from historical nagios-devel mailing list archives
Original poster: [email protected]