Re: Patch: fix various issues found by static analysis [LONG_MAX]

[Vitezslav Crhonek]

On 10/15/24 1:06 PM, Gavin Smith wrote:
> On Tue, Oct 15, 2024 at 12:36:59PM +0200, Vitezslav Crhonek wrote:
> > Hi,
> >
> > Please review proposed fixes of issues found by static analysis
> > of texinfo-7.1 in the attached patch and consider applying them.
>
> Thanks for sending them, I would like to take some time to look
> at and understand them.

Sure, no problem.

> >  From e807d95e3422b1b45b6ec9d3b6b0f559c136fa5f Mon Sep 17 00:00:00 2001
> > From: Vitezslav Crhonek <vcrhonek@redhat.com>
> > Date: Tue, 15 Oct 2024 10:51:07 +0200
> > Subject: [PATCH 1/7] * info/makedoc.c: fix possible integer overflow
> >
> > ---
> >   info/makedoc.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/info/makedoc.c b/info/makedoc.c
> > index 6810d4d228..9f525df110 100644
> > --- a/info/makedoc.c
> > +++ b/info/makedoc.c
> > @@ -296,7 +296,7 @@ process_one_file (char *filename, FILE *doc_stream, FILE 
> > *funs_stream)
> >         char *func, *doc;
> >         char *func_name;
> >   
> > -      for (; offset < (file_size - decl_len); offset++)
> > +      for (; offset < (file_size - decl_len) && offset < (LONG_MAX - 
> > decl_len); offset++)
> >           {
> >             if (buffer[offset] == '\n')
> >               {
>
> Do we really need to check for integer overflow?  Surely every C program
> in existence has possible integer overflows if its input gets big enough
> and I don't want comparisons to LONG_MAX etc. everywhere.
>
> The check
>
>     offset < (file_size - decl_len) && offset < (LONG_MAX - decl_len)
>
> is strange.  One of the two values that offset is compared to is as least
> as large as the other (and files_size and decl_len don't change values
> throughout the loop).
>
> Suppose
>
>     (file_size - decl_len) > (LONG_MAX - decl_len)
>
> Then
>
>     file_size > LONG_MAX
>
> This is impossible as file_size is a long.
>
> So (file_size - decl_len) <= (LONG_MAX - decl_len) and the part you
> added is unnecessary.

I would agree that this shouldn't be real problem and if you want to
omit this hunk, I'm fine with that.

Vita