[{"id":1760173,"web_url":"http://patchwork.ozlabs.org/comment/1760173/","msgid":"<59A6C377.90705@iogearbox.net>","list_archive_url":null,"date":"2017-08-30T13:53:59","subject":"Re: [iproute PATCH] lib/bpf: Fix bytecode-file parsing","submitter":{"id":65705,"url":"http://patchwork.ozlabs.org/api/people/65705/","name":"Daniel Borkmann","email":"daniel@iogearbox.net"},"content":"On 08/29/2017 05:09 PM, Phil Sutter wrote:\n> The signedness of char type is implementation dependent, and there are\n> architectures on which it is unsigned by default. In that case, the\n> check whether fgetc() returned EOF failed because the return value was\n> assigned an (unsigned) char variable prior to comparison with EOF (which\n> is defined to -1). Fix this by using int as type for 'c' variable, which\n> also matches the declaration of fgetc().\n>\n> While being at it, fix the parser logic to correctly handle multiple\n> empty lines and consecutive whitespace and tab characters to further\n> improve the parser's robustness. Note that this will still detect double\n> separator characters, so doesn't soften up the parser too much.\n>\n> Fixes: 3da3ebfca85b8 (\"bpf: Make bytecode-file reading a little more robust\")\n> Cc: Daniel Borkmann \n> Signed-off-by: Phil Sutter \n\nDefinitely ack on the EOF bug:\n\nAcked-by: Daniel Borkmann \n\n[...]\n> @@ -228,18 +229,20 @@ static int bpf_parse_string(char *arg, bool from_file, __u16 *bpf_len,\n> \t\t\tcase '\\n':\n> \t\t\t\tif (c_prev != ',')\n> \t\t\t\t\t*(pos++) = ',';\n> +\t\t\t\tc_prev = ',';\n> \t\t\t\tbreak;\n> \t\t\tcase ' ':\n> \t\t\tcase '\\t':\n> \t\t\t\tif (c_prev != ' ')\n> \t\t\t\t\t*(pos++) = c;\n> +\t\t\t\tc_prev = ' ';\n> \t\t\t\tbreak;\n> \t\t\tdefault:\n> \t\t\t\t*(pos++) = c;\n> +\t\t\t\tc_prev = c;\n> \t\t\t}\n> \t\t\tif (pos - tmp_string == tmp_len)\n> \t\t\t\tbreak;\n> -\t\t\tc_prev = c;\n\nI don't really have a strong opinion on this, but the logic for\nnormalizing here is getting a bit convoluted. Is your use case\nfor making the parser more robust mainly so you can just use the\n-ddd output from tcpdump for cBPF w/o piping through tr? But even\nthat shouldn't give multiple empty lines afaik, no?","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xj6TJ0Ynnz9sNc\n\tfor ;\n\tWed, 30 Aug 2017 23:55:16 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751506AbdH3NzO (ORCPT );\n\tWed, 30 Aug 2017 09:55:14 -0400","from www62.your-server.de ([213.133.104.62]:54968 \"EHLO\n\twww62.your-server.de\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751330AbdH3NyP (ORCPT\n\t); Wed, 30 Aug 2017 09:54:15 -0400","from [92.105.166.74] (helo=localhost.localdomain)\n\tby www62.your-server.de with esmtpsa (TLSv1.2:DHE-RSA-AES256-SHA:256)\n\t(Exim 4.85_2) (envelope-from )\n\tid 1dn3RQ-0007dJ-DH; Wed, 30 Aug 2017 15:54:00 +0200"],"Message-ID":"<59A6C377.90705@iogearbox.net>","Date":"Wed, 30 Aug 2017 15:53:59 +0200","From":"Daniel Borkmann ","User-Agent":"Mozilla/5.0 (X11; Linux x86_64;\n\trv:31.0) Gecko/20100101 Thunderbird/31.7.0","MIME-Version":"1.0","To":"Phil Sutter , Stephen Hemminger ","CC":"netdev@vger.kernel.org","Subject":"Re: [iproute PATCH] lib/bpf: Fix bytecode-file parsing","References":"<20170829150945.7077-1-phil@nwl.cc>","In-Reply-To":"<20170829150945.7077-1-phil@nwl.cc>","Content-Type":"text/plain; charset=windows-1252; format=flowed","Content-Transfer-Encoding":"7bit","X-Authenticated-Sender":"daniel@iogearbox.net","X-Virus-Scanned":"Clear (ClamAV 0.99.2/23740/Wed Aug 30 14:35:11 2017)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1760190,"web_url":"http://patchwork.ozlabs.org/comment/1760190/","msgid":"<20170830141155.GH20614@orbyte.nwl.cc>","list_archive_url":null,"date":"2017-08-30T14:11:56","subject":"Re: [iproute PATCH] lib/bpf: Fix bytecode-file parsing","submitter":{"id":4285,"url":"http://patchwork.ozlabs.org/api/people/4285/","name":"Phil Sutter","email":"phil@nwl.cc"},"content":"Hi Daniel,\n\nOn Wed, Aug 30, 2017 at 03:53:59PM +0200, Daniel Borkmann wrote:\n> On 08/29/2017 05:09 PM, Phil Sutter wrote:\n[...]\n> > @@ -228,18 +229,20 @@ static int bpf_parse_string(char *arg, bool from_file, __u16 *bpf_len,\n> > \t\t\tcase '\\n':\n> > \t\t\t\tif (c_prev != ',')\n> > \t\t\t\t\t*(pos++) = ',';\n> > +\t\t\t\tc_prev = ',';\n> > \t\t\t\tbreak;\n> > \t\t\tcase ' ':\n> > \t\t\tcase '\\t':\n> > \t\t\t\tif (c_prev != ' ')\n> > \t\t\t\t\t*(pos++) = c;\n> > +\t\t\t\tc_prev = ' ';\n> > \t\t\t\tbreak;\n> > \t\t\tdefault:\n> > \t\t\t\t*(pos++) = c;\n> > +\t\t\t\tc_prev = c;\n> > \t\t\t}\n> > \t\t\tif (pos - tmp_string == tmp_len)\n> > \t\t\t\tbreak;\n> > -\t\t\tc_prev = c;\n> \n> I don't really have a strong opinion on this, but the logic for\n> normalizing here is getting a bit convoluted. Is your use case\n> for making the parser more robust mainly so you can just use the\n> -ddd output from tcpdump for cBPF w/o piping through tr? But even\n> that shouldn't give multiple empty lines afaik, no?\n\nWell, using tcpdump output was functional before already. I just noticed\nthat if I add an empty line to the end of bytecode-file, it will fail\nand I didn't like that. Then while searching for the EOF issue, I\nnoticed that the parser logic above is a bit faulty in that it will\ntreat different characters equally but doesn't make sure c_prev will be\nassigned only one of them. So apart from the added robustness, it really\nfixes an inconsistency in the parsing logic.\n\nCheers, Phil","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xj6rc4Yyjz9s8J\n\tfor ;\n\tThu, 31 Aug 2017 00:12:00 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751903AbdH3OL6 (ORCPT );\n\tWed, 30 Aug 2017 10:11:58 -0400","from orbyte.nwl.cc ([151.80.46.58]:59185 \"EHLO mail.nwl.cc\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751735AbdH3OL5 (ORCPT );\n\tWed, 30 Aug 2017 10:11:57 -0400","from mail.nwl.cc (orbyte.nwl.cc [127.0.0.1])\n\tby mail.nwl.cc (Postfix) with ESMTP id 1BDD8644CF;\n\tWed, 30 Aug 2017 16:11:56 +0200 (CEST)","by mail.nwl.cc (Postfix, from userid 1000)\n\tid 0ADB765A5E; Wed, 30 Aug 2017 16:11:56 +0200 (CEST)"],"Date":"Wed, 30 Aug 2017 16:11:56 +0200","From":"Phil Sutter ","To":"Daniel Borkmann ","Cc":"Stephen Hemminger , netdev@vger.kernel.org","Subject":"Re: [iproute PATCH] lib/bpf: Fix bytecode-file parsing","Message-ID":"<20170830141155.GH20614@orbyte.nwl.cc>","Mail-Followup-To":"Phil Sutter ,\n\tDaniel Borkmann ,\n\tStephen Hemminger ,\n\tnetdev@vger.kernel.org","References":"<20170829150945.7077-1-phil@nwl.cc>\n\t<59A6C377.90705@iogearbox.net>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<59A6C377.90705@iogearbox.net>","User-Agent":"Mutt/1.7.2 (2016-11-26)","X-Virus-Scanned":"ClamAV using ClamSMTP","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1761929,"web_url":"http://patchwork.ozlabs.org/comment/1761929/","msgid":"<59A9B15C.3060404@iogearbox.net>","list_archive_url":null,"date":"2017-09-01T19:13:32","subject":"Re: [iproute PATCH] lib/bpf: Fix bytecode-file parsing","submitter":{"id":65705,"url":"http://patchwork.ozlabs.org/api/people/65705/","name":"Daniel Borkmann","email":"daniel@iogearbox.net"},"content":"On 08/30/2017 04:11 PM, Phil Sutter wrote:\n> On Wed, Aug 30, 2017 at 03:53:59PM +0200, Daniel Borkmann wrote:\n>> On 08/29/2017 05:09 PM, Phil Sutter wrote:\n[...]\n>>\n>> I don't really have a strong opinion on this, but the logic for\n>> normalizing here is getting a bit convoluted. Is your use case\n>> for making the parser more robust mainly so you can just use the\n>> -ddd output from tcpdump for cBPF w/o piping through tr? But even\n>> that shouldn't give multiple empty lines afaik, no?\n>\n> Well, using tcpdump output was functional before already. I just noticed\n> that if I add an empty line to the end of bytecode-file, it will fail\n> and I didn't like that. Then while searching for the EOF issue, I\n> noticed that the parser logic above is a bit faulty in that it will\n> treat different characters equally but doesn't make sure c_prev will be\n> assigned only one of them. So apart from the added robustness, it really\n> fixes an inconsistency in the parsing logic.\n\nOk, fine by me.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xkTRk3QM6z9sQl\n\tfor ;\n\tSat, 2 Sep 2017 05:13:38 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752443AbdIATNg (ORCPT );\n\tFri, 1 Sep 2017 15:13:36 -0400","from www62.your-server.de ([213.133.104.62]:40577 \"EHLO\n\twww62.your-server.de\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1752202AbdIATNf (ORCPT\n\t); Fri, 1 Sep 2017 15:13:35 -0400","from [92.105.166.74] (helo=localhost.localdomain)\n\tby www62.your-server.de with esmtpsa (TLSv1.2:DHE-RSA-AES256-SHA:256)\n\t(Exim 4.85_2) (envelope-from )\n\tid 1dnrNk-0003wU-NH; Fri, 01 Sep 2017 21:13:32 +0200"],"Message-ID":"<59A9B15C.3060404@iogearbox.net>","Date":"Fri, 01 Sep 2017 21:13:32 +0200","From":"Daniel Borkmann ","User-Agent":"Mozilla/5.0 (X11; Linux x86_64;\n\trv:31.0) Gecko/20100101 Thunderbird/31.7.0","MIME-Version":"1.0","To":"Phil Sutter ,\n\tStephen Hemminger , netdev@vger.kernel.org","Subject":"Re: [iproute PATCH] lib/bpf: Fix bytecode-file parsing","References":"<20170829150945.7077-1-phil@nwl.cc>\n\t<59A6C377.90705@iogearbox.net>\n\t<20170830141155.GH20614@orbyte.nwl.cc>","In-Reply-To":"<20170830141155.GH20614@orbyte.nwl.cc>","Content-Type":"text/plain; charset=windows-1252; format=flowed","Content-Transfer-Encoding":"7bit","X-Authenticated-Sender":"daniel@iogearbox.net","X-Virus-Scanned":"Clear (ClamAV 0.99.2/23753/Fri Sep 1 18:36:32 2017)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1762845,"web_url":"http://patchwork.ozlabs.org/comment/1762845/","msgid":"<20170904120907.4c2221b2@xeon-e3>","list_archive_url":null,"date":"2017-09-04T19:09:06","subject":"Re: [iproute PATCH] lib/bpf: Fix bytecode-file parsing","submitter":{"id":21389,"url":"http://patchwork.ozlabs.org/api/people/21389/","name":"Stephen Hemminger","email":"stephen@networkplumber.org"},"content":"On Tue, 29 Aug 2017 17:09:45 +0200\nPhil Sutter wrote:\n\n> The signedness of char type is implementation dependent, and there are\n> architectures on which it is unsigned by default. In that case, the\n> check whether fgetc() returned EOF failed because the return value was\n> assigned an (unsigned) char variable prior to comparison with EOF (which\n> is defined to -1). Fix this by using int as type for 'c' variable, which\n> also matches the declaration of fgetc().\n> \n> While being at it, fix the parser logic to correctly handle multiple\n> empty lines and consecutive whitespace and tab characters to further\n> improve the parser's robustness. Note that this will still detect double\n> separator characters, so doesn't soften up the parser too much.\n> \n> Fixes: 3da3ebfca85b8 (\"bpf: Make bytecode-file reading a little more robust\")\n> Cc: Daniel Borkmann \n> Signed-off-by: Phil Sutter \n\nLooks fine applied.\n\nAlthough I think only Android is using unsigned for char type at this point.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=networkplumber-org.20150623.gappssmtp.com\n\theader.i=@networkplumber-org.20150623.gappssmtp.com\n\theader.b=\"MOw0ZWLg\"; dkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xmKCN71qXz9s7c\n\tfor ;\n\tTue, 5 Sep 2017 05:09:18 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753965AbdIDTJP (ORCPT );\n\tMon, 4 Sep 2017 15:09:15 -0400","from mail-pg0-f49.google.com ([74.125.83.49]:33137 \"EHLO\n\tmail-pg0-f49.google.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1753932AbdIDTJO (ORCPT\n\t); Mon, 4 Sep 2017 15:09:14 -0400","by mail-pg0-f49.google.com with SMTP id t3so3386603pgt.0\n\tfor ; Mon, 04 Sep 2017 12:09:14 -0700 (PDT)","from xeon-e3 (76-14-207-240.or.wavecable.com. [76.14.207.240])\n\tby smtp.gmail.com with ESMTPSA id\n\tw124sm12106335pfd.179.2017.09.04.12.09.13\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tMon, 04 Sep 2017 12:09:13 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=networkplumber-org.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:in-reply-to:references\n\t:mime-version:content-transfer-encoding;\n\tbh=wKb8ThfDyRD+XcnxJPiwtV+o4jdORkD5ew05FtE1H0I=;\n\tb=MOw0ZWLgDjXPN6Ez/6tgX2kSEVMo4N8y9WC/D1K98MD0PLikRbuDtuvLQSpjebvRm8\n\tBtDBP8zAtyk5eRvKp0vIJRCywBN4oiKib/ZBr9ZqLaE1kidR3qtw5i8OHHJBSc+b3w6W\n\tPCc6gabLGKdZAva0C8chctHGtTuWWkWiHU/7BOzMcrEvwhtE6XBqJR++59bnbCHsYe1a\n\t3Blp/z5GuxrBfCTYWq4JDUXwWlucPr0EG20vHXqyvtcrxt+aeyRHZ66v3eoHTTxxeSNL\n\t5COACcBku9bko7P0fgS/rgtnDgLgf8DlxmSXw4QieJEKkQoLOTIvoMyMrByYmzgZ/fcn\n\tFr4A==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to\n\t:references:mime-version:content-transfer-encoding;\n\tbh=wKb8ThfDyRD+XcnxJPiwtV+o4jdORkD5ew05FtE1H0I=;\n\tb=PXUOIirFWedr3+j6qRHdz2lPygK8PGs4W3Ettj58hrOJQi+I9tMgkH0A7fJAkpbC/3\n\t/HA+iG4TvEtdkbyXqq0FPIJLq+zSrTzvGQUVQRDuuMsJUpX9NtBGCHL21OhOcWBR3KdL\n\t893u3Eu9wxykfF+ZnJemcOgSRbtl9u0uEsB3w8ziC4Ob7dUK4sme1Da4cZ5nH31WvUia\n\t8BldseNISecJ/b4YHFC5q9nIrF9V/bBljZy0vXWbstpm8QvxEuJEkJXgV281eu+zJxQS\n\toqLgoYDXUDBIeyC7R7So2cNJUv3bgxPFxRiPN/5QUwFFDS40bd/M+EtdiYS3YuuREe7v\n\tf59Q==","X-Gm-Message-State":"AHPjjUj5/KlvlM6EDXfETTcpgQIMve07/xB4dhPEymN8TQKAh9tv0gUA\n\tfC+Qiz97qM9a+4D4","X-Google-Smtp-Source":"ADKCNb5qAfnMt7g9Wlc+wlg116k2rqnJcmsXrPnegrSpHSmvuySt5P+1uJQ5CajG7+THD8nzx0Lcuw==","X-Received":"by 10.98.73.73 with SMTP id w70mr1247831pfa.335.1504552153867;\n\tMon, 04 Sep 2017 12:09:13 -0700 (PDT)","Date":"Mon, 4 Sep 2017 12:09:06 -0700","From":"Stephen Hemminger ","To":"Phil Sutter ","Cc":"netdev@vger.kernel.org, Daniel Borkmann ","Subject":"Re: [iproute PATCH] lib/bpf: Fix bytecode-file parsing","Message-ID":"<20170904120907.4c2221b2@xeon-e3>","In-Reply-To":"<20170829150945.7077-1-phil@nwl.cc>","References":"<20170829150945.7077-1-phil@nwl.cc>","MIME-Version":"1.0","Content-Type":"text/plain; charset=US-ASCII","Content-Transfer-Encoding":"7bit","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}}]