DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] pmdinfogen: allow padding after NUL terminator
@ 2021-05-26 21:43 Dmitry Kozlyuk
  2021-05-27  6:53 ` David Marchand
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Dmitry Kozlyuk @ 2021-05-26 21:43 UTC (permalink / raw)
  To: dev; +Cc: Zhihong Peng, Dmitry Kozlyuk, Neil Horman

Size of string constant symbol may be larger than its length
measured up to NUL terminator. In this case pmdinfogen included padding
bytes after NUL terminator in generated source, yielding incorrect code.

Always trim string data to NUL terminator while reading ELF.
It was already done for COFF because there's no symbol size.

Bugzilla ID: 720
Fixes: f0f93a7adfee ("buildtools: use Python pmdinfogen")

Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
 buildtools/coff.py       |  6 ------
 buildtools/pmdinfogen.py | 10 ++++++++--
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/buildtools/coff.py b/buildtools/coff.py
index 86fb0602b7..a7b6c37e32 100644
--- a/buildtools/coff.py
+++ b/buildtools/coff.py
@@ -146,9 +146,3 @@ def get_section_data(self, number):
 
     def get_string(self, offset):
         return decode_asciiz(self._strings[offset:])
-
-
-def decode_asciiz(data):
-    index = data.find(b'\x00')
-    end = index if index >= 0 else len(data)
-    return data[:end].decode()
diff --git a/buildtools/pmdinfogen.py b/buildtools/pmdinfogen.py
index 7a739ec7d4..8bb59699b8 100755
--- a/buildtools/pmdinfogen.py
+++ b/buildtools/pmdinfogen.py
@@ -19,6 +19,12 @@
 import coff
 
 
+def decode_asciiz(data):
+    index = data.find(b'\x00')
+    end = index if index >= 0 else len(data)
+    return data[:end].decode()
+
+
 class ELFSymbol:
     def __init__(self, image, symbol):
         self._image = image
@@ -28,7 +34,7 @@ def __init__(self, image, symbol):
     def string_value(self):
         size = self._symbol["st_size"]
         value = self.get_value(0, size)
-        return value[:-1].decode() if value else ""
+        return decode_asciiz(value) if value else ""
 
     def get_value(self, offset, size):
         section = self._symbol["st_shndx"]
@@ -86,7 +92,7 @@ def get_value(self, offset, size):
     @property
     def string_value(self):
         value = self._symbol.get_value(0)
-        return coff.decode_asciiz(value) if value else ''
+        return decode_asciiz(value) if value else ""
 
 
 class COFFImage:
-- 
2.29.3


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] pmdinfogen: allow padding after NUL terminator
  2021-05-26 21:43 [dpdk-dev] [PATCH] pmdinfogen: allow padding after NUL terminator Dmitry Kozlyuk
@ 2021-05-27  6:53 ` David Marchand
  2021-05-27  7:09   ` Dmitry Kozlyuk
  2021-05-27  7:31 ` David Marchand
  2021-05-27 21:24 ` [dpdk-dev] [PATCH v2] " Dmitry Kozlyuk
  2 siblings, 1 reply; 10+ messages in thread
From: David Marchand @ 2021-05-27  6:53 UTC (permalink / raw)
  To: Dmitry Kozlyuk; +Cc: dev, Zhihong Peng, Neil Horman

On Wed, May 26, 2021 at 11:44 PM Dmitry Kozlyuk
<dmitry.kozliuk@gmail.com> wrote:
>
> Size of string constant symbol may be larger than its length
> measured up to NUL terminator. In this case pmdinfogen included padding
> bytes after NUL terminator in generated source, yielding incorrect code.
>
> Always trim string data to NUL terminator while reading ELF.
> It was already done for COFF because there's no symbol size.
>
> Bugzilla ID: 720
> Fixes: f0f93a7adfee ("buildtools: use Python pmdinfogen")
>
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

Reproduced the issue described in bz on my (old) RHEL7 with clang 3.4.2.

Reviewed-by: David Marchand <david.marchand@redhat.com>

Just to confirm my reading of the C version of pmdinfogen: the C
version formats those symbols fine with printf %s.
So there should be no need for a fix in stable branches, right?


-- 
David Marchand


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] pmdinfogen: allow padding after NUL terminator
  2021-05-27  6:53 ` David Marchand
@ 2021-05-27  7:09   ` Dmitry Kozlyuk
  0 siblings, 0 replies; 10+ messages in thread
From: Dmitry Kozlyuk @ 2021-05-27  7:09 UTC (permalink / raw)
  To: David Marchand; +Cc: dev, Zhihong Peng, Neil Horman

On Thu, May 27, 2021, 09:53 David Marchand <david.marchand@redhat.com>
wrote:

> Just to confirm my reading of the C version of pmdinfogen: the C version
> formats those symbols fine with printf %s.
> So there should be no need for a fix in stable branches, right?
>

Yes.

>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] pmdinfogen: allow padding after NUL terminator
  2021-05-26 21:43 [dpdk-dev] [PATCH] pmdinfogen: allow padding after NUL terminator Dmitry Kozlyuk
  2021-05-27  6:53 ` David Marchand
@ 2021-05-27  7:31 ` David Marchand
  2021-05-27 21:24 ` [dpdk-dev] [PATCH v2] " Dmitry Kozlyuk
  2 siblings, 0 replies; 10+ messages in thread
From: David Marchand @ 2021-05-27  7:31 UTC (permalink / raw)
  To: Dmitry Kozlyuk; +Cc: dev, Zhihong Peng, Neil Horman

On Wed, May 26, 2021 at 11:44 PM Dmitry Kozlyuk
<dmitry.kozliuk@gmail.com> wrote:
>
> Size of string constant symbol may be larger than its length
> measured up to NUL terminator. In this case pmdinfogen included padding
> bytes after NUL terminator in generated source, yielding incorrect code.
>
> Always trim string data to NUL terminator while reading ELF.
> It was already done for COFF because there's no symbol size.
>
> Bugzilla ID: 720
> Fixes: f0f93a7adfee ("buildtools: use Python pmdinfogen")
>
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---
>  buildtools/coff.py       |  6 ------
>  buildtools/pmdinfogen.py | 10 ++++++++--
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/buildtools/coff.py b/buildtools/coff.py
> index 86fb0602b7..a7b6c37e32 100644
> --- a/buildtools/coff.py
> +++ b/buildtools/coff.py
> @@ -146,9 +146,3 @@ def get_section_data(self, number):
>
>      def get_string(self, offset):
>          return decode_asciiz(self._strings[offset:])
                  ^^^^

coff.py still needs this helper.

Caught in the lab:
Traceback (most recent call last):

  File "../buildtools/pmdinfogen.py", line 280, in <module>
    main()
  File "../buildtools/pmdinfogen.py", line 275, in main
    drivers = load_drivers(image)
  File "../buildtools/pmdinfogen.py", line 208, in load_drivers
    for symbol in image.find_by_prefix("this_pmd_name"):
  File "../buildtools/pmdinfogen.py", line 108, in find_by_prefix
    if symbol.name.startswith(prefix):
  File "C:\Users\builder\jenkins\workspace\Windows-Compile-DPDK-Mingw64\dpdk\buildtools\coff.py",
line 84, in name
    return decode_asciiz(bytes(self._coff.name.immediate))
NameError: name 'decode_asciiz' is not defined
Traceback (most recent call last):
  File "../buildtools/gen-pmdinfo-cfile.py", line 20, in <module>
    subprocess.run(pmdinfogen + paths + [output], check=True)
  File "c:\python38\lib\subprocess.py", line 512, in run
    raise CalledProcessError(retcode, process.args,


-- 
David Marchand


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [dpdk-dev] [PATCH v2] pmdinfogen: allow padding after NUL terminator
  2021-05-26 21:43 [dpdk-dev] [PATCH] pmdinfogen: allow padding after NUL terminator Dmitry Kozlyuk
  2021-05-27  6:53 ` David Marchand
  2021-05-27  7:31 ` David Marchand
@ 2021-05-27 21:24 ` Dmitry Kozlyuk
  2021-06-09 15:52   ` Dmitry Kozlyuk
  2021-06-18 14:39   ` [dpdk-dev] " Thomas Monjalon
  2 siblings, 2 replies; 10+ messages in thread
From: Dmitry Kozlyuk @ 2021-05-27 21:24 UTC (permalink / raw)
  To: dev; +Cc: Zhihong Peng, Dmitry Kozlyuk, Neil Horman

Size of string constant symbol may be larger than its length
measured up to NUL terminator. In this case pmdinfogen included padding
bytes after NUL terminator in generated source, yielding incorrect code.

Always trim string data to NUL terminator while reading ELF.
It was already done for COFF because there's no symbol size.

Bugzilla ID: 720
Fixes: f0f93a7adfee ("buildtools: use Python pmdinfogen")

Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
v2: return helper to coff.py, where it's needed (David Marchand).

 buildtools/pmdinfogen.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/buildtools/pmdinfogen.py b/buildtools/pmdinfogen.py
index 7a739ec7d4..2a44f17bda 100755
--- a/buildtools/pmdinfogen.py
+++ b/buildtools/pmdinfogen.py
@@ -28,7 +28,7 @@ def __init__(self, image, symbol):
     def string_value(self):
         size = self._symbol["st_size"]
         value = self.get_value(0, size)
-        return value[:-1].decode() if value else ""
+        return coff.decode_asciiz(value)  # not COFF-specific
 
     def get_value(self, offset, size):
         section = self._symbol["st_shndx"]
-- 
2.29.3


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH v2] pmdinfogen: allow padding after NUL terminator
  2021-05-27 21:24 ` [dpdk-dev] [PATCH v2] " Dmitry Kozlyuk
@ 2021-06-09 15:52   ` Dmitry Kozlyuk
  2021-06-09 16:01     ` [dpdk-dev] [dpdk-ci] " Lincoln Lavoie
  2021-06-18 14:39   ` [dpdk-dev] " Thomas Monjalon
  1 sibling, 1 reply; 10+ messages in thread
From: Dmitry Kozlyuk @ 2021-06-09 15:52 UTC (permalink / raw)
  To: dev; +Cc: Zhihong Peng, Neil Horman, ci

2021-05-28 00:24 (UTC+0300), Dmitry Kozlyuk:
> Size of string constant symbol may be larger than its length
> measured up to NUL terminator. In this case pmdinfogen included padding
> bytes after NUL terminator in generated source, yielding incorrect code.
> 
> Always trim string data to NUL terminator while reading ELF.
> It was already done for COFF because there's no symbol size.
> 
> Bugzilla ID: 720
> Fixes: f0f93a7adfee ("buildtools: use Python pmdinfogen")
> 
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---
> v2: return helper to coff.py, where it's needed (David Marchand).
> 
>  buildtools/pmdinfogen.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/buildtools/pmdinfogen.py b/buildtools/pmdinfogen.py
> index 7a739ec7d4..2a44f17bda 100755
> --- a/buildtools/pmdinfogen.py
> +++ b/buildtools/pmdinfogen.py
> @@ -28,7 +28,7 @@ def __init__(self, image, symbol):
>      def string_value(self):
>          size = self._symbol["st_size"]
>          value = self.get_value(0, size)
> -        return value[:-1].decode() if value else ""
> +        return coff.decode_asciiz(value)  # not COFF-specific
>  
>      def get_value(self, offset, size):
>          section = self._symbol["st_shndx"]

There are CI failures that seem unrelated to this patch:
some tests with NICs that I can't check
and an Arch Linux build failure that I failed to reproduce.
GitHub Actions are passing.
Are these known CI bugs or does this patch need any corrections?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [dpdk-ci] [PATCH v2] pmdinfogen: allow padding after NUL terminator
  2021-06-09 15:52   ` Dmitry Kozlyuk
@ 2021-06-09 16:01     ` Lincoln Lavoie
  2021-06-09 16:48       ` Dmitry Kozlyuk
  0 siblings, 1 reply; 10+ messages in thread
From: Lincoln Lavoie @ 2021-06-09 16:01 UTC (permalink / raw)
  To: Dmitry Kozlyuk; +Cc: dev, Zhihong Peng, Neil Horman, ci

Hi Dmitry,

If the failing test is the unit test func_reentrancy_autotest, that is
being looked into , as it seems like the test case does not reliably run.
It passes / fails between different systems running both on bare metal and
in virtual environments, on different OSes and architectures.

Do you have a link to your patch in patchworks or the lab dashboard?

Cheers,
Lincoln



On Wed, Jun 9, 2021 at 11:52 AM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
wrote:

> 2021-05-28 00:24 (UTC+0300), Dmitry Kozlyuk:
> > Size of string constant symbol may be larger than its length
> > measured up to NUL terminator. In this case pmdinfogen included padding
> > bytes after NUL terminator in generated source, yielding incorrect code.
> >
> > Always trim string data to NUL terminator while reading ELF.
> > It was already done for COFF because there's no symbol size.
> >
> > Bugzilla ID: 720
> > Fixes: f0f93a7adfee ("buildtools: use Python pmdinfogen")
> >
> > Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > ---
> > v2: return helper to coff.py, where it's needed (David Marchand).
> >
> >  buildtools/pmdinfogen.py | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/buildtools/pmdinfogen.py b/buildtools/pmdinfogen.py
> > index 7a739ec7d4..2a44f17bda 100755
> > --- a/buildtools/pmdinfogen.py
> > +++ b/buildtools/pmdinfogen.py
> > @@ -28,7 +28,7 @@ def __init__(self, image, symbol):
> >      def string_value(self):
> >          size = self._symbol["st_size"]
> >          value = self.get_value(0, size)
> > -        return value[:-1].decode() if value else ""
> > +        return coff.decode_asciiz(value)  # not COFF-specific
> >
> >      def get_value(self, offset, size):
> >          section = self._symbol["st_shndx"]
>
> There are CI failures that seem unrelated to this patch:
> some tests with NICs that I can't check
> and an Arch Linux build failure that I failed to reproduce.
> GitHub Actions are passing.
> Are these known CI bugs or does this patch need any corrections?
>


-- 
*Lincoln Lavoie*
Principal Engineer, Broadband Technologies
21 Madbury Rd., Ste. 100, Durham, NH 03824
lylavoie@iol.unh.edu
https://www.iol.unh.edu
+1-603-674-2755 (m)
<https://www.iol.unh.edu>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [dpdk-ci] [PATCH v2] pmdinfogen: allow padding after NUL terminator
  2021-06-09 16:01     ` [dpdk-dev] [dpdk-ci] " Lincoln Lavoie
@ 2021-06-09 16:48       ` Dmitry Kozlyuk
  2021-06-09 17:19         ` Owen Hilyard
  0 siblings, 1 reply; 10+ messages in thread
From: Dmitry Kozlyuk @ 2021-06-09 16:48 UTC (permalink / raw)
  To: Lincoln Lavoie; +Cc: dev, Zhihong Peng, Neil Horman, ci

2021-06-09 12:01 (UTC-0400), Lincoln Lavoie:
> Hi Dmitry,
> 
> If the failing test is the unit test func_reentrancy_autotest, that is
> being looked into , as it seems like the test case does not reliably run.
> It passes / fails between different systems running both on bare metal and
> in virtual environments, on different OSes and architectures.
> 
> Do you have a link to your patch in patchworks or the lab dashboard?

Hi Lincoln,

Yes, this is func_reentrancy_autotest failure.
Dashboard link:
https://lab.dpdk.org/results/dashboard/patchsets/17240/
MTU and stats can hardly be affected by this patch.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [dpdk-ci] [PATCH v2] pmdinfogen: allow padding after NUL terminator
  2021-06-09 16:48       ` Dmitry Kozlyuk
@ 2021-06-09 17:19         ` Owen Hilyard
  0 siblings, 0 replies; 10+ messages in thread
From: Owen Hilyard @ 2021-06-09 17:19 UTC (permalink / raw)
  To: Dmitry Kozlyuk; +Cc: Lincoln Lavoie, dev, Zhihong Peng, Neil Horman, ci

Hi Dmitry,

Those failures are the result of a known issue with that machine. Those
tests were disabled on that machine shortly after you submitted that patch.
I've re-run the patch.

Owen Hilyard

On Wed, Jun 9, 2021 at 12:48 PM Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
wrote:

> 2021-06-09 12:01 (UTC-0400), Lincoln Lavoie:
> > Hi Dmitry,
> >
> > If the failing test is the unit test func_reentrancy_autotest, that is
> > being looked into , as it seems like the test case does not reliably run.
> > It passes / fails between different systems running both on bare metal
> and
> > in virtual environments, on different OSes and architectures.
> >
> > Do you have a link to your patch in patchworks or the lab dashboard?
>
> Hi Lincoln,
>
> Yes, this is func_reentrancy_autotest failure.
> Dashboard link:
> https://lab.dpdk.org/results/dashboard/patchsets/17240/
> MTU and stats can hardly be affected by this patch.
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH v2] pmdinfogen: allow padding after NUL terminator
  2021-05-27 21:24 ` [dpdk-dev] [PATCH v2] " Dmitry Kozlyuk
  2021-06-09 15:52   ` Dmitry Kozlyuk
@ 2021-06-18 14:39   ` Thomas Monjalon
  1 sibling, 0 replies; 10+ messages in thread
From: Thomas Monjalon @ 2021-06-18 14:39 UTC (permalink / raw)
  To: Dmitry Kozlyuk; +Cc: dev, Zhihong Peng, Dmitry Kozlyuk, Neil Horman

27/05/2021 23:24, Dmitry Kozlyuk:
> Size of string constant symbol may be larger than its length
> measured up to NUL terminator. In this case pmdinfogen included padding
> bytes after NUL terminator in generated source, yielding incorrect code.
> 
> Always trim string data to NUL terminator while reading ELF.
> It was already done for COFF because there's no symbol size.
> 
> Bugzilla ID: 720
> Fixes: f0f93a7adfee ("buildtools: use Python pmdinfogen")
> 
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

Applied, thanks



^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-06-18 14:39 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-26 21:43 [dpdk-dev] [PATCH] pmdinfogen: allow padding after NUL terminator Dmitry Kozlyuk
2021-05-27  6:53 ` David Marchand
2021-05-27  7:09   ` Dmitry Kozlyuk
2021-05-27  7:31 ` David Marchand
2021-05-27 21:24 ` [dpdk-dev] [PATCH v2] " Dmitry Kozlyuk
2021-06-09 15:52   ` Dmitry Kozlyuk
2021-06-09 16:01     ` [dpdk-dev] [dpdk-ci] " Lincoln Lavoie
2021-06-09 16:48       ` Dmitry Kozlyuk
2021-06-09 17:19         ` Owen Hilyard
2021-06-18 14:39   ` [dpdk-dev] " Thomas Monjalon

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git