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 443BD3813E4C for ; Wed, 5 Jun 2024 13:49:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 443BD3813E4C Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 443BD3813E4C Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1717595387; cv=none; b=Gy+GRua2022uD8kWBAVZVa5r4zpyFBe5y/Nqbg76trRntu+zQ9NlbzEqMAM95eZC1HdFowJp+filKujM6kW7I0SJsa1oy2UIh+HHamig5kgPqjjlXvY0eRaZH8Gq4EtCFEmpnCWLWTl5sg9jZ51QgRKQisKvESKU756xnfwM/HU= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1717595387; c=relaxed/simple; bh=95SUBlls6cfxCs2QeC4kJvO1zz6jdvyrYQoDNfiPK7o=; h=DKIM-Signature:Message-ID:Subject:From:To:Date:MIME-Version; b=Fif3U8IrT6sjduRcnH502ruP+cK56D+d5VAEWBhiDezAHmy/0esYAA8FngTZ2/hoQkrFZyRQ3HggNnqe9FCfYAgMMYStvsEb/FEV0PhFVNWFJC2v0uOfFtoSr3nz84mH3VrYy4X4nz6DrgVNo4ATnXLhVvUZ/YqOC21udl+WMmY= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1717595384; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=maocDBHrdK4PAGhTwCQD6AEjH+EWMASYL2BmnX07Y64=; b=AtT6Ji2vz5QUjpYzVoLIHndONR6d4Yl7WLbbohuX5bWmiIqtOe7U3EyKAz1Vexl/Ny+48B kEUxQ8FoBQXGqdf3XT9PDZWy/ZHtLoxN52bMItWsf6BgeRBAjh7nuxcfqjPYLJel+fTlK3 CEYh1c0EnfmqI6Hddj0hpH7NheouQw0= Received: from mail-oi1-f197.google.com (mail-oi1-f197.google.com [209.85.167.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-327-FSBG0Qd_M0yF_pO0sUYsLA-1; Wed, 05 Jun 2024 09:49:43 -0400 X-MC-Unique: FSBG0Qd_M0yF_pO0sUYsLA-1 Received: by mail-oi1-f197.google.com with SMTP id 5614622812f47-3d1ca4ed5a3so6819365b6e.3 for ; Wed, 05 Jun 2024 06:49:43 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1717595383; x=1718200183; h=mime-version:user-agent:content-transfer-encoding:references :in-reply-to:date:cc:to:from:subject:message-id:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=Y7RWF+mt33jQBZH5qnvVCaJgHAhsm4DZGcUlTYtY4GI=; b=FqkYn0HmOr4cVsVYF62Ld4Y649eH8Jd9BHrqSkyrxJq+1F8x+x4wTiLbrv0pM/GBsA taoloF+m9wba99oRDNRvF4/ve7TW0s+dp6U+AJld3iMglxbiAznASdB3pjucedPDQGqx /11AcEeXSuuWH/wKgzImHwwz7IEyqVMWkulxC5k7qj4p+/HcUHotVlb/a2tYEEK/920T dvaBut8vJH/SPAK2H+oudhvf7HW9cNzktSNj5hHbf4taWgWNhXqLoaUZ4a8bP1/XT0q+ baI5BMAjgyje5bsdlfIAcFUfjzxxo5ebi9B6HYzLJD759IzLQX0c9qGa56HwC4VpgmGG OhYQ== X-Forwarded-Encrypted: i=1; AJvYcCWpQjH4V4//yVB+uepaEemFMN/BQBcUvMBFGVaQwj7CLEi7Xn0++YWpcFiO5rSa8Fa3xxFncdRT8vBwDn0trFQFYM5606xK4Q== X-Gm-Message-State: AOJu0YxqAyUoOcmtKtAwp7tX/G2x3lo2BoHmlx5ZwA1nG1X1cLd/kudH kfynZj2vc6SxeIVy2pFad1vlE4htd8rDe4eomeodUp7DRYpxSJND+Io+AqqsVxLPCBc4oGJJIj4 waJNI3pAJ7sPX6qo6fJEpj8psBBzF+xh9xyo/ZDZNkgvOsBMbJ1dH8S0= X-Received: by 2002:aca:2b19:0:b0:3d1:fd86:5d84 with SMTP id 5614622812f47-3d20417ce5dmr2479605b6e.0.1717595382331; Wed, 05 Jun 2024 06:49:42 -0700 (PDT) X-Google-Smtp-Source: AGHT+IH6nxaVue15Acr7+ajgg4kX8x8VoNsQJdDPF7IpwBCFOIKUaMjxYJmnhvR7O2EFK1aYr6sYBg== X-Received: by 2002:aca:2b19:0:b0:3d1:fd86:5d84 with SMTP id 5614622812f47-3d20417ce5dmr2479586b6e.0.1717595381868; Wed, 05 Jun 2024 06:49:41 -0700 (PDT) Received: from t14s.localdomain (c-76-28-97-5.hsd1.ma.comcast.net. [76.28.97.5]) by smtp.gmail.com with ESMTPSA id af79cd13be357-79523945b89sm69026885a.64.2024.06.05.06.49.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 05 Jun 2024 06:49:41 -0700 (PDT) Message-ID: <8a42e01a6bb4c86704c473f709c9ac0ffb8c571d.camel@redhat.com> Subject: Re: "counted_by" and -fanalyzer From: David Malcolm To: Qing Zhao Cc: Richard Biener , Joseph Myers , Siddhesh Poyarekar , "uecker@tugraz.at" , kees Cook , "isanbard@gmail.com" , GCC Patches Date: Wed, 05 Jun 2024 09:49:40 -0400 In-Reply-To: <8794CE99-50F5-418B-8A1D-AE0C30BCAC88@oracle.com> References: <20240530122700.1516243-1-qing.zhao@oracle.com> <20240530122700.1516243-3-qing.zhao@oracle.com> <26305p25-o014-9p09-n96q-q56285n4r66p@fhfr.qr> <446051bb29a896fb18b3c7e29c53290988885e2a.camel@redhat.com> <8794CE99-50F5-418B-8A1D-AE0C30BCAC88@oracle.com> User-Agent: Evolution 3.44.4 (3.44.4-2.fc36) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,RCVD_IN_SORBS_WEB,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Tue, 2024-06-04 at 22:09 +0000, Qing Zhao wrote: >=20 >=20 > > On Jun 4, 2024, at 17:55, David Malcolm > > wrote: > >=20 > > On Fri, 2024-05-31 at 13:11 +0000, Qing Zhao wrote: > > >=20 > > >=20 [...] > > >=20 > > >=20 > > > Thanks a lot for the review. > > > Will commit the patch set soon. > >=20 > > [...snip...] > >=20 > > Congratulations on getting this merged. > >=20 > > FWIW I've started investigating adding support for the new > > attribute to > > -fanalyzer (and am tracked this as PR analyzer/111567 > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D111567=C2=A0). >=20 > Thank you for starting looking at this. > >=20 > > The docs for the attribute speak of the implied relationship > > between > > the count field and size of the flex array, and say that: "It's the > > user's responsibility to make sure the above requirements to be > > kept > > all the time.=C2=A0 Otherwise the compiler *reports warnings*, at the > > same > > time, the results of the array bound sanitizer and the > > '__builtin_dynamic_object_size' is undefined." (my emphasis). > >=20 > > What are these warnings that are reported?=C2=A0 I looked through=20 > > r15-944-gf824acd0e80754 through r15-948-g4c5bea7def1361 and I > > didn't > > see any new warnings or test coverage for warnings (beyond misuing > > the > > attribute).=C2=A0 Sorry if I'm missing something obvious here. >=20 > These warnings will be in the remaining work (I listed the remaining > work in all versions except the last one): >=20 > > > > > ******Remaining works:=20 > > > > >=20 > > > > > 6=C2=A0 Improve __bdos to use the counted_by info in whole-object > > > > > size for the structure with FAM. > > > > > 7=C2=A0 Emit warnings when the user breaks the requirments for th= e > > > > > new counted_by attribute > > > > > compilation time: -Wcounted-by > > > > > run time: -fsanitizer=3Dcounted-by > > > > > =C2=A0=C2=A0 * The initialization to the size field should be don= e > > > > > before the first reference to the FAM field. > > > > > =C2=A0=C2=A0 * the array has at least # of elements specified by = the > > > > > size field all the time during the program. Aha - thanks. Sorry for missing this, I confess I haven't been paying close attention to this thread. >=20 > With the current patches that have been committed, the warnings are > not emitted.=20 > I believe that more analysis and more information are needed for > these warnings to be effective, it might not > be a trivial patch.=C2=A0 More discussion is needed for emitting such > warnings. >=20 > >=20 > > Does anyone have examples of cases that -fanalyzer ought to warn > > for? >=20 > At this moment, I don=E2=80=99t have concrete testing cases for this yet,= but > I can come up with several small examples and share with you in a > later email. FWIW I did some brainstorming and put together the following .c file, am posting it inline here for the sake of discussion; does this look like the kind of thing to test for (in terms of how users are expected to use the attribute, and the kinds of mistake they'd want warnings about) ? /* TODO: Some ideas for dimensions of test matrix: (a) concrete value vs symbolic value for "count" (b) concrete value vs symbolic value for size of array (c) dynamic vs static allocation of buffer (and malloc vs alloca) (d) relative size of array and of count - same size (not an issue) - array is too small compared to "count" - off by one =09 - off by more than one =09 - size is zero (but not negative) - negative size (which the docs say is OK) - array is too large compared to "count" (not an issue) (e) type of flex array: - char - non-char - type requiring padding (f) type/size/signedness of count field; what about overflow in (count * sizeof (type of array element)) ? ... etc: ideas? Other ideas for test coverage: - realloc - growing object - shrinking object - symbolic sizes where could be growth or shrinkage - failing realloc - ...etc: ideas? */ #include #include #include /* Example from the docs. */ struct P { size_t count; char other; char array[] __attribute__ ((counted_by (count))); } *p; struct P * test_malloc_with_correct_symbolic (size_t n) { struct P *p =3D malloc (sizeof (struct P) + n); if (!p) return NULL; p->count =3D n; // don't warn here return p; =20 } struct P * test_malloc_with_correct_count_concrete (void) { struct P *p =3D malloc (sizeof (struct P) + 42); if (!p) return NULL; p->count =3D 42; // don't warn here return p; =20 } struct P * test_malloc_with_array_smaller_than_count_concrete (void) { struct P *p =3D malloc (sizeof (struct P) + 42); if (!p) return NULL; p->count =3D 80; // TODO: warn here return p; } struct P * test_malloc_with_array_larger_than_count_concrete (void) { struct P *p =3D malloc (sizeof (struct P) + 80); if (!p) return NULL; p->count =3D 42; // don't warn here return p; =20 } struct P * test_malloc_with_array_access_before_count_init_concrete_1 (void) { struct P *p =3D malloc (sizeof (struct P) + 42); if (!p) return NULL; /* Forgetting to set count altogether. */ __builtin_memset (p->array, 0, 42); // TODO: should warn here return p; =20 } struct P * test_malloc_with_array_access_before_count_init_concrete_2 (void) { struct P *p =3D malloc (sizeof (struct P) + 42); if (!p) return NULL; /* Erroneously touching array before setting count. */ __builtin_memset (p->array, 0, 42); // TODO: should warn here p->count =3D 42; return p; =20 } struct P * test_malloc_with_array_access_before_count_init_symbolic_1 (size_t n) { struct P *p =3D malloc (sizeof (struct P) + n); if (!p) return NULL; /* Forgetting to set count altogether. */ __builtin_memset (p->array, 0, n); // TODO: should warn here return p; =20 } struct P * test_malloc_with_array_access_before_count_init_symbolic_2 (size_t n) { struct P *p =3D malloc (sizeof (struct P) + n); if (!p) return NULL; /* Erroneously touching array before setting count. */ __builtin_memset (p->array, 0, n); // TODO: should warn here p->count =3D n; return p; =20 } /* Example where sizeof array element !=3D 1. */ struct Q { size_t count; int32_t array[] __attribute__ ((counted_by (count))); }; =20 struct Q * test_malloc_of_non_char_array_valid_symbolic (size_t n) { size_t alloc_sz =3D sizeof (struct Q) + (sizeof (int32_t) * n); struct Q *q =3D malloc (alloc_sz); if (!q) return NULL; // Don't warn for this: q->count =3D n; __builtin_memset (q->array, 0, sizeof (int32_t) * n); return q; } =20 struct Q * test_malloc_of_non_char_array_bad_size_symbolic (size_t n) { /* Allocation size is too small: forgetting to multiply count by sizeof (array element). */ size_t alloc_sz =3D sizeof (struct Q) + n; struct Q *q =3D malloc (alloc_sz); if (!q) return NULL; /* TODO: should we warn here? Allocated size of flex array is too small relative to implicit size of accesses. */ q->count =3D n; /* TODO: should we warn here? This initializes the buffer we allocated, but only the first quarter of the flex array. */ __builtin_memset (q->array, 0, n); =20 /* TODO: should we warn here? This initializes the full flex array as specified by "count", but is out-of-bounds relative to our heap allocation. */ __builtin_memset (q->array, 0, sizeof (int32_t) * n); return q; }