From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12555 invoked by alias); 28 Mar 2017 16:57:12 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 12539 invoked by uid 89); 28 Mar 2017 16:57:10 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_NONE autolearn=no version=3.3.2 spammy=remembered X-HELO: mail-wm0-f47.google.com Received: from mail-wm0-f47.google.com (HELO mail-wm0-f47.google.com) (74.125.82.47) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 28 Mar 2017 16:57:09 +0000 Received: by mail-wm0-f47.google.com with SMTP id o81so3634486wmb.1 for ; Tue, 28 Mar 2017 09:57:10 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=JM7hsNjNyWYnzZHHn5Z1QhuEdgP+d07kJHfJOgtXYXY=; b=pYF+rwXU4wodeV1ceCGUB4XYtiYoLVkH4UMM7YCP+zRVpfwFozj6Pom8TgbEls1HTk fpSATpTFdF3gCTzHkR9fUlLHzLHS0RH+k5UkTWPNn7N8AVWU4vPJtLl3SfYmzzn2lAqS 2RNXqtSX9aCS3LGfrxEKfakkybwGieUNoPs0xS34Pm14D2A7cTaR+/qDHdIluWH6VS2D VmVRROrWwKlRJIqyOuw+cw491RtCk1WB+yw0MQF7RlACWSMO0F3icrCXP9PFxUQSCHPf Um3N1SrUTToRdvr0UKpuGGB+Is3dw65/6fwmfxLYOzEwz0gm7cCT2E2JEjgRy9h17zAw yxeQ== X-Gm-Message-State: AFeK/H3qa0x6+XmESTkqezfcvOawOfeGcGDAtlUz0xkUxrE3dDMECe/irTJjMYInn2g6B89x X-Received: by 10.28.232.21 with SMTP id f21mr10257426wmh.0.1490720228445; Tue, 28 Mar 2017 09:57:08 -0700 (PDT) Received: from [192.168.0.101] ([37.189.166.198]) by smtp.gmail.com with ESMTPSA id o196sm4361882wmg.12.2017.03.28.09.57.07 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 28 Mar 2017 09:57:07 -0700 (PDT) Subject: Re: extract_unsigned_integer API (Re: [PATCH] Remove MAX_REGISTER_SIZE from frame.c) To: Yao Qi References: <86lgspqisk.fsf@gmail.com> <5f2f0cb0-6265-46aa-4ad6-eda5ba817da4@redhat.com> <8660itnzvv.fsf@gmail.com> Cc: Alan Hayward , "gdb-patches@sourceware.org" , nd From: Pedro Alves Message-ID: <93774758-0354-c67b-9733-005b3d56fbfa@redhat.com> Date: Tue, 28 Mar 2017 16:57:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <8660itnzvv.fsf@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-03/txt/msg00490.txt.bz2 On 03/28/2017 05:13 PM, Yao Qi wrote: > Pedro Alves writes: > >> class extractor >> { >> public: >> extractor () = default; >> >> // Get buffer. Could take a "size" parameter too, >> // for pre-validation instead of passing "size" to "extract". >> // Or make that a separate size() method. Or add a "size" parameter >> // to the ctor and validate there. Whatever. The lambda-based >> // solution isn't validating upfront either. > > My lambda-based solution does validate the boundary before reading > contents to buffer, > > +ULONGEST > +extract_unsigned_integer (gdb::function_view content_provider, > + int len, enum bfd_endian byte_order) > +{ > + if (len > (int) sizeof (ULONGEST)) > + error (_("\ > +That operation is not available on integers of more than %d bytes."), > + (int) sizeof (ULONGEST)); > + > + gdb_byte buf[sizeof (ULONGEST)]; > + > + content_provider (buf, len); > + return extract_unsigned_integer_1 (buf, len, byte_order); > +} > The upfront check doesn't protect entirely. It still up to the lambda, to ultimately check for overflow. Here: return extract_unsigned_integer ([&] (gdb_byte *buf, size_t size) { frame_unwind_register (frame, regnum, buf); }, size, byte_order); ... frame_unwind_register can overflow the buffer, since it ignores "size". And if we require adding some check inside the lambda, then we've really not gained that much by doing the upfront check. >> >> extractor extr; >> frame_unwind_register (frame, regnum, ext.buffer ()); > > We may overflow ext.buffer (), because the boundary checking is done in > .extract below, > >> return extr.extract (size, byte_order); Yes, and that can sorted by e.g., passing the size to the buffer() method, as I mentioned in the comment. Like: extractor extr; frame_unwind_register (frame, regnum, ext.buffer (size)); return extr.extract (size, byte_order); extractor::buffer(size_t) would throw error on overflow. Or pass it to the ctor (which would likewise throw error on overflow): extractor extr (size); frame_unwind_register (frame, regnum, ext.buffer ()); return extr.extract (size, byte_order); Could even store the size and byte order inside the extractor object, and avoid writing the size more than once: extractor extr (size, byte_order); frame_unwind_register (frame, regnum, ext.buffer ()); return extr.extract (); Or make "extrator::buffer" remember the last size, so extractors can be reused. Or even support both, ctor with and without size, buffer() with and without size. extractror::extract would always used the last remembered size. So I still don't see any advantage in a callback-based interface. Thanks, Pedro Alves