Re: [PATCH] lib/autoconf/c.m4: fix NULL pointer dereference in _AC_LANG_IO_PROGRAM

[Nicolas Iooss]

On Tue, Jun 29, 2021 at 2:47 PM Eric Blake <eblake@redhat.com> wrote:
> On Tue, Jun 29, 2021 at 09:56:44AM +0200, Nicolas IOOSS wrote:
> > > >  [FILE *f = fopen ("conftest.out", "w");
> > > > - return ferror (f) || fclose (f) != 0;
> > > > + return !f || ferror (f) || fclose (f) != 0;
> > > >  ])])
> > >
> > > Thank you for the bug report and the patch. The change looks correct
> > > and is simple enough not to require a copyright assignment. I will
> > > apply it to Autoconf trunk in the next few days.
> > >
> > > Our test suite does not catch this bug, which probably means it never
> > > tests AC_LANG_IO_PROGRAM inside AC_RUN_IFELSE. Would you be willing to
> > > write a test case, perhaps based on the configure.ac that caused you
> > > to discover the bug? If you don't have time to write a test yourself,
> > > could you at least tell us about how you found the bug?
> >
> > Hello,
> > Thanks for your quick reply! Actually I found this bug running clang's
> > static code analyzer on a project named secp256k1. When using
> > "scan-build -enable-checker alpha.unix.Stream", this tool spot an
> > issue while running ./configure and I found out it was a real bug. For
> > more details, I described on
> > https://github.com/bitcoin-core/secp256k1/pull/959#issuecomment-869733706
> > the exact command line I used. So the autoconf.ac file that I used is
> > https://github.com/bitcoin-core/secp256k1/blob/75ce488c2a6d47bfd6ed333e3e919a98ea86139a/configure.ac
>
> You do realize, of course, that configure code snippets often take
> shortcuts or otherwise perform the bare minimum necessary to come up
> with an answer that works, rather than worrying about satisfying the
> demands of every compiler warning and static code analyzer out there.
> Yes, making sure the fopen() succeeded is good coding practice, but in
> the scope of this particular configure test, you've got MUCH bigger
> problems if a 3-line program can't even perform fopen() without
> succeeding.
>
> While I'm not opposed to your patch, I don't think that chasing the
> windmill of making configure snippets warning-free is going to be
> worth it, because in the end, it won't change the results of those
> configure tests.

I do not understand what you mean by "chasing the windmill", and the
top result on Google Search
(https://iit.adelaide.edu.au/news/list/2021/04/29/chasing-the-windmill-what-is-wrong-with-the-us-approach-on-developing-country)
seems to be about developing countries. Is this some cultural
reference that non-English-speaking people are not supposed to
understand? Could you please use expressions for which the meaning can
be easily searched on the Internet?

On the topic, I agree that "making configure snippets warning-free" is
not a great objective, and is quite difficult because static code
analyzer software tends to report many false positive issues. But here
it is a real fix and I find it inappropriate that you reply with a
message which translates to "you are doing something idiot, please
stop this stupid thing, static analysis is worthless". In my opinion,
I can carry patches on my own which help reduce the number of false
positive issues reported by static analyzer tools (without involving
any upstream discussion), but when I find a bug I prefer to report it
and help upstream developers to fix it rather than keeping it for
myself. This is why I submitted a patch and I believe such an approach
is what makes free software possible, instead of calling the work of
someone else "not worth it".

> But lest I sound too negative, thank you for reporting what you found,
> and for providing a patch!
>
> > The information contained in this email and in any
> > document enclosed is strictly confidential and is intended solely for the
>
> Legalese blurbs like these are unenforceable on a publically-archived
> mailing list; you may want to consider using an alternative account
> that does not append your employer's text on your mails to open source
> lists.

Sorry about this. I posted the patch from my professional email and
missed the fact that my employer added a few weeks ago a configuration
to GMail which automatically adds this blurb, even to full-text emails
sent to mailing lists.

Best regards,
Nicolas