From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 96C0C385BAED for ; Fri, 24 Jun 2022 18:05:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 96C0C385BAED Received: from mail-qv1-f72.google.com (mail-qv1-f72.google.com [209.85.219.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-65-jHfezWLPPFeUekSWqJ1iOg-1; Fri, 24 Jun 2022 14:05:10 -0400 X-MC-Unique: jHfezWLPPFeUekSWqJ1iOg-1 Received: by mail-qv1-f72.google.com with SMTP id b18-20020a0ccd12000000b004703d1b04e8so3237617qvm.13 for ; Fri, 24 Jun 2022 11:05:10 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:subject:from:to:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=5OK2vwMJBmN3oiKqR19zVhQsVL2lVrCP85yj46+p9bQ=; b=tA5ARzqbYxp/UlRd7bOdbSwpLaBM4nS7ehPm2DUGRu3qvKMPWZgVfPNy38tUpxLMmV /PVJE4DlHdAWBlCnFMxXGk9vJ596Ft6M++Em0mR3wVAZA3cOyJWvG3SVqcOpUNH70OYN iDdUQoYIl4vgy3KCfJZYu/5WcDx8hxjNihSqTEX5MfRIoqKbCa6WfkZFYQdDM3XH5C6E 3MRUswTSdFkX9cJvsfOkovgccoBmiAkkbWt5NN+aonEWRTCF+f2zk5wYxVGMj2K4+8i5 DbVvfD6HiC2LlWiwc0bAQtYfn7NE5huAWA+ZeTaI+IjzxuNg6QcqP789bxOw0/rOuSou Ragg== X-Gm-Message-State: AJIora+NpVdqDhsK+j0UcnXsX1iSc5AoTGagu5gmEAEjOSNkkbGd2VRN wAWg9kHz7ATs4dtDMilOBLdm2FrT+QoD963f+71OehiVXmt0qfwY3zav3j4daSzGmKspoN1fMWs hQvPvuI4= X-Received: by 2002:a05:620a:284b:b0:6a9:3787:ecaa with SMTP id h11-20020a05620a284b00b006a93787ecaamr340576qkp.317.1656093908254; Fri, 24 Jun 2022 11:05:08 -0700 (PDT) X-Google-Smtp-Source: AGRyM1sS2UNtcMRCv3evsaWcU3+35uAIiA5gUMIWYfuEhZUGK8izDVJRXW/+Bod3ClmGQyoQVDi3gg== X-Received: by 2002:a05:620a:284b:b0:6a9:3787:ecaa with SMTP id h11-20020a05620a284b00b006a93787ecaamr340546qkp.317.1656093907864; Fri, 24 Jun 2022 11:05:07 -0700 (PDT) Received: from t14s.localdomain (c-73-69-212-193.hsd1.ma.comcast.net. [73.69.212.193]) by smtp.gmail.com with ESMTPSA id 93-20020aed3166000000b00304df6f73f0sm1861042qtg.0.2022.06.24.11.05.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 24 Jun 2022 11:05:07 -0700 (PDT) Message-ID: <8d8eca80dbe287a79f633b410fbba3b775de3054.camel@redhat.com> Subject: Re: [PATCH] static analysis support for posix file desccriptor APIs From: David Malcolm To: Mir Immad , gcc@gcc.gnu.org Date: Fri, 24 Jun 2022 14:05:06 -0400 In-Reply-To: References: User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Jun 2022 18:05:13 -0000 On Thu, 2022-06-23 at 23:58 +0530, Mir Immad wrote: >  Hi Dave, > Thanks for the suggestions, > > I changed most of the things that you suggested, however reporting > for > warnings like close of known invalid fd was problematic: > > consider the following code: > > if (fd >= 0) > { write (fd,...); } > close(fd); > > As I was checking the exploded graph for this; the "close(fd)" stmt > when > visited by the FALSE edge of if stmt (fd < 0) finds fd to be in > m_invalid > state; hence warns about "close on known invalid fd" which I believe > is not > true as fd at that point is not *known* to be invalid. I spent quite > some > time on this and decided not to add this diagnosis for now. That choice seems reasonable to me. > > Also, when close transitions m_invalid to m_close; this leads to > double > close even when the second close is outside the ( < 0 ) condition > which > again does not seem right. > if (fd < 0) > close(fd): > close(fd); // double close here. "close" on an invalid FD doesn't give you a closed FD, you still have an invalid FD. By analogy with sm-malloc.cc: free (NULL) doesn't make NULL a freed pointer, it's still NULL, and thus just a no-op. (although in this case: close(invalid) will also set errno to EBADF, so it's not strictly a no-op). > > > Maybe consolidate on_read and on_write by adding a subroutine that > > checks for m_closed, and for checking access mode (maybe a > > "check_for_open_fd" that takes an access mode enum value amongst > > other > > params.  If you pass around caller_fndecl, I think much of this > > code > > can be shared that way between on_read and on_write (which will > > help > > simplify things if we want to support further functions) > > I hope I got this right. > > > > > +        } > > > +    } > > > +  else > > > +    { > >  >+      /* FIXME: add leak reporting */ > > > Do you have a testcase that exhibits this behavior? > > I was thinking of the following case: > void test() > { >  open(..); > } > Here the resources are leaked because there is no way to free them. Please add a test case for this, marking it with xfail if need be. But hopefully it will be easy to implement the FIXME here, with an: sm->on_warn (new fd_leak ()); or somesuch (not sure if there's a "tree var" you can give it for this case though, might have to use NULL). > > In "read" and "write" funcs, I'm warning for unchecked_use_of_fd and > access_mode_mismatch only when we know fd is not in closed state. > Otherwise, such code leads to lot of irrelevant warnings, example: > > void test1(const char *path, void *buf) { >   int fd = open(path, O_RDONLY); >   if (fd >= 0) >   { >   close(fd); >   read(fd, buf, 1); // read on closed fd + read on possibly invalid > fd >   write(fd, buf, 1); // write on closed fd + write on possibly invlid > fd >   } > } Should we transition the FD to the "stop" state after the first warning? That way we ought to avoid a bunch of followup warnings for that FD. > > > Adding docs for new options still remains pending. I added some new > tests; > and all tests are passing. The stuff about O_* macros is left as-is. > >  I'm sending a patch in another email. > > Thanks a lot. Thanks Dave