[PATCH] gas: Fix internal error on long local labels

[PATCH] gas: Fix internal error on long local labels

[Alex Coplan]

[-- Attachment #1: Type: text/plain, Size: 1245 bytes --]

Hello,

Currently, on an input such as "88888888888:", GAS hits a signed integer
overflow and (likely) an assertion failure. I see:

$ echo "88888888888:" | bin/aarch64-none-elf-as
{standard input}: Assembler messages:
{standard input}:1: Internal error in fb_label_name at ../gas/symbols.c:2049.
Please report this bug.

To fix this issue, I've taken two steps:

1. Change the type used for processing local labels in
   read_a_source_file() from int to long, to allow representing more
   local labels, and also since all uses of this variable (temp) are
   actually of type long.

2. Detect if we would overflow and bail out with an error message
   instead of actually overflowing and hitting the assertion in
   fb_label_name().

Testing:
 * Regression tested on a broad selection of targets.

OK for master?

Thanks,
Alex

---

gas/ChangeLog:

2020-08-05  Alex Coplan  <alex.coplan@arm.com>

	* read.c (read_a_source_file): Use long for local labels, detect
	overflow and raise an error for overly-long labels.
	* testsuite/gas/all/gas.exp: Add local-label-overflow test.
	* testsuite/gas/all/local-label-overflow.d: New test.
	* testsuite/gas/all/local-label-overflow.l: Error output.
	* testsuite/gas/all/local-label-overflow.s: Input.


[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 2891 bytes --]

diff --git a/gas/read.c b/gas/read.c
index f192cc16d57..64021e62a40 100644
--- a/gas/read.c
+++ b/gas/read.c
@@ -40,6 +40,8 @@
 #include "dw2gencfi.h"
 #include "wchar.h"
 
+#include <limits.h>
+
 #ifndef TC_START_LABEL
 #define TC_START_LABEL(STR, NUL_CHAR, NEXT_CHAR) (NEXT_CHAR == ':')
 #endif
@@ -816,7 +818,7 @@ read_a_source_file (const char *name)
   char nul_char;
   char next_char;
   char *s;		/* String of symbol, '\0' appended.  */
-  int temp;
+  long temp;
   pseudo_typeS *pop;
 
 #ifdef WARN_COMMENTS
@@ -1212,10 +1214,21 @@ read_a_source_file (const char *name)
 	      /* Read the whole number.  */
 	      while (ISDIGIT (*input_line_pointer))
 		{
-		  temp = (temp * 10) + *input_line_pointer - '0';
+		  const char digit = *input_line_pointer - '0';
+		  if (temp > ((LONG_MAX - digit) / 10))
+		    {
+		      as_bad (_("local label too large near %s"), backup);
+		      temp = -1;
+		      break;
+		    }
+		  temp = (temp * 10) + digit;
 		  ++input_line_pointer;
 		}
 
+	      /* Overflow: stop processing the label.  */
+	      if (temp == -1)
+		continue;
+
 	      if (LOCAL_LABELS_DOLLAR
 		  && *input_line_pointer == '$'
 		  && *(input_line_pointer + 1) == ':')
@@ -1224,7 +1237,7 @@ read_a_source_file (const char *name)
 
 		  if (dollar_label_defined (temp))
 		    {
-		      as_fatal (_("label \"%d$\" redefined"), temp);
+		      as_fatal (_("label \"%ld$\" redefined"), temp);
 		    }
 
 		  define_dollar_label (temp);
diff --git a/gas/testsuite/gas/all/gas.exp b/gas/testsuite/gas/all/gas.exp
index 5496d0bf7a7..a0158f38dde 100644
--- a/gas/testsuite/gas/all/gas.exp
+++ b/gas/testsuite/gas/all/gas.exp
@@ -102,6 +102,7 @@ if { ![istarget "bfin-*-*"] } then {
 }
 gas_test_error "assign-bad.s" "" "== assignment for symbol already set"
 run_dump_test assign-bad-recursive
+run_dump_test local-label-overflow
 
 run_dump_test simple-forward
 run_dump_test forward
diff --git a/gas/testsuite/gas/all/local-label-overflow.d b/gas/testsuite/gas/all/local-label-overflow.d
new file mode 100644
index 00000000000..1786a45743a
--- /dev/null
+++ b/gas/testsuite/gas/all/local-label-overflow.d
@@ -0,0 +1,6 @@
+#source: local-label-overflow.s
+#error_output: local-label-overflow.l
+#skip: hppa*-*-*
+#skip: ia64-*-vms
+#skip: mmix-*-*
+#skip: sh-*-pe
diff --git a/gas/testsuite/gas/all/local-label-overflow.l b/gas/testsuite/gas/all/local-label-overflow.l
new file mode 100644
index 00000000000..be24355bbd4
--- /dev/null
+++ b/gas/testsuite/gas/all/local-label-overflow.l
@@ -0,0 +1,2 @@
+[^:]*: Assembler messages:
+.*: Error: local label too large near 888888888888888888888888888:
diff --git a/gas/testsuite/gas/all/local-label-overflow.s b/gas/testsuite/gas/all/local-label-overflow.s
new file mode 100644
index 00000000000..e5569217f44
--- /dev/null
+++ b/gas/testsuite/gas/all/local-label-overflow.s
@@ -0,0 +1 @@
+8888888888888888888888888888:

Re: [PATCH] gas: Fix internal error on long local labels

[Alan Modra]

On Wed, Aug 05, 2020 at 02:51:05PM +0100, Alex Coplan wrote:
> 	* read.c (read_a_source_file): Use long for local labels, detect
> 	overflow and raise an error for overly-long labels.
> 	* testsuite/gas/all/gas.exp: Add local-label-overflow test.
> 	* testsuite/gas/all/local-label-overflow.d: New test.
> 	* testsuite/gas/all/local-label-overflow.l: Error output.
> 	* testsuite/gas/all/local-label-overflow.s: Input.

OK, but fix the following nits please.

> @@ -1212,10 +1214,21 @@ read_a_source_file (const char *name)
>  	      /* Read the whole number.  */
>  	      while (ISDIGIT (*input_line_pointer))
>  		{
> -		  temp = (temp * 10) + *input_line_pointer - '0';
> +		  const char digit = *input_line_pointer - '0';

"long digit" is probably better, since you're doing long arithmetic
with "digit".

> +		  if (temp > ((LONG_MAX - digit) / 10))

Unnecessary parens.

> +		    {
> +		      as_bad (_("local label too large near %s"), backup);
> +		      temp = -1;
> +		      break;
> +		    }
> +		  temp = (temp * 10) + digit;

Here too.  (Yes, they were in the original source.)

>  		  ++input_line_pointer;
>  		}
>  
> +	      /* Overflow: stop processing the label.  */
> +	      if (temp == -1)
> +		continue;
> +
>  	      if (LOCAL_LABELS_DOLLAR
>  		  && *input_line_pointer == '$'
>  		  && *(input_line_pointer + 1) == ':')
> @@ -1224,7 +1237,7 @@ read_a_source_file (const char *name)
>  
>  		  if (dollar_label_defined (temp))
>  		    {
> -		      as_fatal (_("label \"%d$\" redefined"), temp);
> +		      as_fatal (_("label \"%ld$\" redefined"), temp);
>  		    }
>  
>  		  define_dollar_label (temp);
> diff --git a/gas/testsuite/gas/all/gas.exp b/gas/testsuite/gas/all/gas.exp
> index 5496d0bf7a7..a0158f38dde 100644
> --- a/gas/testsuite/gas/all/gas.exp
> +++ b/gas/testsuite/gas/all/gas.exp
> @@ -102,6 +102,7 @@ if { ![istarget "bfin-*-*"] } then {
>  }
>  gas_test_error "assign-bad.s" "" "== assignment for symbol already set"
>  run_dump_test assign-bad-recursive
> +run_dump_test local-label-overflow
>  
>  run_dump_test simple-forward
>  run_dump_test forward
> diff --git a/gas/testsuite/gas/all/local-label-overflow.d b/gas/testsuite/gas/all/local-label-overflow.d
> new file mode 100644
> index 00000000000..1786a45743a
> --- /dev/null
> +++ b/gas/testsuite/gas/all/local-label-overflow.d
> @@ -0,0 +1,6 @@
> +#source: local-label-overflow.s
> +#error_output: local-label-overflow.l
> +#skip: hppa*-*-*
> +#skip: ia64-*-vms
> +#skip: mmix-*-*
> +#skip: sh-*-pe

Use
#notarget: hppa*-*-* ia64-*-vms mmix-*-* sh-*-pe
instead of #skip.  See binutils-common.exp for description of skip.

-- 
Alan Modra
Australia Development Lab, IBM

RE: [PATCH] gas: Fix internal error on long local labels

[Alex Coplan]

> -----Original Message-----
> From: Alan Modra <amodra@gmail.com>
> Sent: 06 August 2020 14:56
> To: Alex Coplan <Alex.Coplan@arm.com>
> Cc: binutils@sourceware.org
> Subject: Re: [PATCH] gas: Fix internal error on long local labels
> 
> On Wed, Aug 05, 2020 at 02:51:05PM +0100, Alex Coplan wrote:
> > 	* read.c (read_a_source_file): Use long for local labels, detect
> > 	overflow and raise an error for overly-long labels.
> > 	* testsuite/gas/all/gas.exp: Add local-label-overflow test.
> > 	* testsuite/gas/all/local-label-overflow.d: New test.
> > 	* testsuite/gas/all/local-label-overflow.l: Error output.
> > 	* testsuite/gas/all/local-label-overflow.s: Input.
> 
> OK, but fix the following nits please.

Thanks. Pushed to master with nits fixed as
d27aad4ec307d91a6b9056127c6c248044391d7f.

Is it worth backporting?

Alex

Re: [PATCH] gas: Fix internal error on long local labels

[H.J. Lu]

On Thu, Aug 6, 2020 at 9:49 AM Alex Coplan <Alex.Coplan@arm.com> wrote:
>
> > -----Original Message-----
> > From: Alan Modra <amodra@gmail.com>
> > Sent: 06 August 2020 14:56
> > To: Alex Coplan <Alex.Coplan@arm.com>
> > Cc: binutils@sourceware.org
> > Subject: Re: [PATCH] gas: Fix internal error on long local labels
> >
> > On Wed, Aug 05, 2020 at 02:51:05PM +0100, Alex Coplan wrote:
> > >     * read.c (read_a_source_file): Use long for local labels, detect
> > >     overflow and raise an error for overly-long labels.
> > >     * testsuite/gas/all/gas.exp: Add local-label-overflow test.
> > >     * testsuite/gas/all/local-label-overflow.d: New test.
> > >     * testsuite/gas/all/local-label-overflow.l: Error output.
> > >     * testsuite/gas/all/local-label-overflow.s: Input.
> >
> > OK, but fix the following nits please.
>
> Thanks. Pushed to master with nits fixed as
> d27aad4ec307d91a6b9056127c6c248044391d7f.

The test failed on 32-bit hosts:

./as-new -o x.o
/export/gnu/import/git/sources/binutils-gdb/gas/testsuite/gas/all/local-label-overflow.s
/export/gnu/import/git/sources/binutils-gdb/gas/testsuite/gas/all/local-label-overflow.s:
Assembler messages:
/export/gnu/import/git/sources/binutils-gdb/gas/testsuite/gas/all/local-label-overflow.s:1:
Error: local label too large near 888888888888888888888888888:

/export/gnu/import/git/sources/binutils-gdb/gas/testsuite/gas/all/local-label-overflow.s:1:
Error: local label too large near 888888888888888888:

/export/gnu/import/git/sources/binutils-gdb/gas/testsuite/gas/all/local-label-overflow.s:1:
Error: local label too large near 888888888:

I am checking in this to fix it:

diff --git a/gas/read.c b/gas/read.c
index bd896580e5..cb41cc0685 100644
--- a/gas/read.c
+++ b/gas/read.c
@@ -1227,7 +1227,10 @@ read_a_source_file (const char *name)

         /* Overflow: stop processing the label.  */
         if (temp == -1)
-     continue;
+     {
+       ignore_rest_of_line ();
+       continue;
+     }

         if (LOCAL_LABELS_DOLLAR
        && *input_line_pointer == '$'

> Is it worth backporting?
>
> Alex



-- 
H.J.

RE: [PATCH] gas: Fix internal error on long local labels

[Alex Coplan]

Thanks for the fix, H.J.

This has now been on master for 10 days. I've tested the original patch,
together with this fix, on the 2.35 branch and it looks good.

OK to backport to 2.35 branch?

If so, should the backport squash these two commits together or
cherry-pick them separately?

Cheers,
Alex

> -----Original Message-----
> From: H.J. Lu <hjl.tools@gmail.com>
> Sent: 07 August 2020 14:44
> To: Alex Coplan <Alex.Coplan@arm.com>
> Cc: Alan Modra <amodra@gmail.com>; binutils@sourceware.org
> Subject: Re: [PATCH] gas: Fix internal error on long local labels
> 
> On Thu, Aug 6, 2020 at 9:49 AM Alex Coplan <Alex.Coplan@arm.com> wrote:
> >
> > > -----Original Message-----
> > > From: Alan Modra <amodra@gmail.com>
> > > Sent: 06 August 2020 14:56
> > > To: Alex Coplan <Alex.Coplan@arm.com>
> > > Cc: binutils@sourceware.org
> > > Subject: Re: [PATCH] gas: Fix internal error on long local labels
> > >
> > > On Wed, Aug 05, 2020 at 02:51:05PM +0100, Alex Coplan wrote:
> > > >     * read.c (read_a_source_file): Use long for local labels,
> detect
> > > >     overflow and raise an error for overly-long labels.
> > > >     * testsuite/gas/all/gas.exp: Add local-label-overflow test.
> > > >     * testsuite/gas/all/local-label-overflow.d: New test.
> > > >     * testsuite/gas/all/local-label-overflow.l: Error output.
> > > >     * testsuite/gas/all/local-label-overflow.s: Input.
> > >
> > > OK, but fix the following nits please.
> >
> > Thanks. Pushed to master with nits fixed as
> > d27aad4ec307d91a6b9056127c6c248044391d7f.
> 
> The test failed on 32-bit hosts:
> 
> ./as-new -o x.o
> /export/gnu/import/git/sources/binutils-gdb/gas/testsuite/gas/all/local-
> label-overflow.s
> /export/gnu/import/git/sources/binutils-gdb/gas/testsuite/gas/all/local-
> label-overflow.s:
> Assembler messages:
> /export/gnu/import/git/sources/binutils-gdb/gas/testsuite/gas/all/local-
> label-overflow.s:1:
> Error: local label too large near 888888888888888888888888888:
> 
> /export/gnu/import/git/sources/binutils-gdb/gas/testsuite/gas/all/local-
> label-overflow.s:1:
> Error: local label too large near 888888888888888888:
> 
> /export/gnu/import/git/sources/binutils-gdb/gas/testsuite/gas/all/local-
> label-overflow.s:1:
> Error: local label too large near 888888888:
> 
> I am checking in this to fix it:
> 
> diff --git a/gas/read.c b/gas/read.c
> index bd896580e5..cb41cc0685 100644
> --- a/gas/read.c
> +++ b/gas/read.c
> @@ -1227,7 +1227,10 @@ read_a_source_file (const char *name)
> 
>          /* Overflow: stop processing the label.  */
>          if (temp == -1)
> -     continue;
> +     {
> +       ignore_rest_of_line ();
> +       continue;
> +     }
> 
>          if (LOCAL_LABELS_DOLLAR
>         && *input_line_pointer == '$'
> 
> > Is it worth backporting?
> >
> > Alex
> 
> 
> 
> --
> H.J.

Re: [PATCH] gas: Fix internal error on long local labels

[H.J. Lu]

On Mon, Aug 17, 2020 at 10:22 AM Alex Coplan <Alex.Coplan@arm.com> wrote:
>
> Thanks for the fix, H.J.
>
> This has now been on master for 10 days. I've tested the original patch,
> together with this fix, on the 2.35 branch and it looks good.
>
> OK to backport to 2.35 branch?
>
> If so, should the backport squash these two commits together or
> cherry-pick them separately?

When you do backport, you should backport 2 commits together with
a single commit.

> Cheers,
> Alex
>
> > -----Original Message-----
> > From: H.J. Lu <hjl.tools@gmail.com>
> > Sent: 07 August 2020 14:44
> > To: Alex Coplan <Alex.Coplan@arm.com>
> > Cc: Alan Modra <amodra@gmail.com>; binutils@sourceware.org
> > Subject: Re: [PATCH] gas: Fix internal error on long local labels
> >
> > On Thu, Aug 6, 2020 at 9:49 AM Alex Coplan <Alex.Coplan@arm.com> wrote:
> > >
> > > > -----Original Message-----
> > > > From: Alan Modra <amodra@gmail.com>
> > > > Sent: 06 August 2020 14:56
> > > > To: Alex Coplan <Alex.Coplan@arm.com>
> > > > Cc: binutils@sourceware.org
> > > > Subject: Re: [PATCH] gas: Fix internal error on long local labels
> > > >
> > > > On Wed, Aug 05, 2020 at 02:51:05PM +0100, Alex Coplan wrote:
> > > > >     * read.c (read_a_source_file): Use long for local labels,
> > detect
> > > > >     overflow and raise an error for overly-long labels.
> > > > >     * testsuite/gas/all/gas.exp: Add local-label-overflow test.
> > > > >     * testsuite/gas/all/local-label-overflow.d: New test.
> > > > >     * testsuite/gas/all/local-label-overflow.l: Error output.
> > > > >     * testsuite/gas/all/local-label-overflow.s: Input.
> > > >
> > > > OK, but fix the following nits please.
> > >
> > > Thanks. Pushed to master with nits fixed as
> > > d27aad4ec307d91a6b9056127c6c248044391d7f.
> >
> > The test failed on 32-bit hosts:
> >
> > ./as-new -o x.o
> > /export/gnu/import/git/sources/binutils-gdb/gas/testsuite/gas/all/local-
> > label-overflow.s
> > /export/gnu/import/git/sources/binutils-gdb/gas/testsuite/gas/all/local-
> > label-overflow.s:
> > Assembler messages:
> > /export/gnu/import/git/sources/binutils-gdb/gas/testsuite/gas/all/local-
> > label-overflow.s:1:
> > Error: local label too large near 888888888888888888888888888:
> >
> > /export/gnu/import/git/sources/binutils-gdb/gas/testsuite/gas/all/local-
> > label-overflow.s:1:
> > Error: local label too large near 888888888888888888:
> >
> > /export/gnu/import/git/sources/binutils-gdb/gas/testsuite/gas/all/local-
> > label-overflow.s:1:
> > Error: local label too large near 888888888:
> >
> > I am checking in this to fix it:
> >
> > diff --git a/gas/read.c b/gas/read.c
> > index bd896580e5..cb41cc0685 100644
> > --- a/gas/read.c
> > +++ b/gas/read.c
> > @@ -1227,7 +1227,10 @@ read_a_source_file (const char *name)
> >
> >          /* Overflow: stop processing the label.  */
> >          if (temp == -1)
> > -     continue;
> > +     {
> > +       ignore_rest_of_line ();
> > +       continue;
> > +     }
> >
> >          if (LOCAL_LABELS_DOLLAR
> >         && *input_line_pointer == '$'
> >
> > > Is it worth backporting?
> > >
> > > Alex
> >
> >
> >
> > --
> > H.J.



-- 
H.J.

RE: [PATCH] gas: Fix internal error on long local labels

[Alex Coplan]

> -----Original Message-----
> From: H.J. Lu <hjl.tools@gmail.com>
> Sent: 17 August 2020 18:35
> To: Alex Coplan <Alex.Coplan@arm.com>
> Cc: Alan Modra <amodra@gmail.com>; binutils@sourceware.org
> Subject: Re: [PATCH] gas: Fix internal error on long local labels
> 
> On Mon, Aug 17, 2020 at 10:22 AM Alex Coplan <Alex.Coplan@arm.com> wrote:
> >
> > Thanks for the fix, H.J.
> >
> > This has now been on master for 10 days. I've tested the original
> patch,
> > together with this fix, on the 2.35 branch and it looks good.
> >
> > OK to backport to 2.35 branch?

Ping?

> >
> > If so, should the backport squash these two commits together or
> > cherry-pick them separately?
> 
> When you do backport, you should backport 2 commits together with
> a single commit.

Thanks for clarifying.

> 
> > Cheers,
> > Alex
> >
> > > -----Original Message-----
> > > From: H.J. Lu <hjl.tools@gmail.com>
> > > Sent: 07 August 2020 14:44
> > > To: Alex Coplan <Alex.Coplan@arm.com>
> > > Cc: Alan Modra <amodra@gmail.com>; binutils@sourceware.org
> > > Subject: Re: [PATCH] gas: Fix internal error on long local labels
> > >
> > > On Thu, Aug 6, 2020 at 9:49 AM Alex Coplan <Alex.Coplan@arm.com>
> wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Alan Modra <amodra@gmail.com>
> > > > > Sent: 06 August 2020 14:56
> > > > > To: Alex Coplan <Alex.Coplan@arm.com>
> > > > > Cc: binutils@sourceware.org
> > > > > Subject: Re: [PATCH] gas: Fix internal error on long local labels
> > > > >
> > > > > On Wed, Aug 05, 2020 at 02:51:05PM +0100, Alex Coplan wrote:
> > > > > >     * read.c (read_a_source_file): Use long for local labels,
> > > detect
> > > > > >     overflow and raise an error for overly-long labels.
> > > > > >     * testsuite/gas/all/gas.exp: Add local-label-overflow test.
> > > > > >     * testsuite/gas/all/local-label-overflow.d: New test.
> > > > > >     * testsuite/gas/all/local-label-overflow.l: Error output.
> > > > > >     * testsuite/gas/all/local-label-overflow.s: Input.
> > > > >
> > > > > OK, but fix the following nits please.
> > > >
> > > > Thanks. Pushed to master with nits fixed as
> > > > d27aad4ec307d91a6b9056127c6c248044391d7f.
> > >
> > > The test failed on 32-bit hosts:
> > >
> > > ./as-new -o x.o
> > > /export/gnu/import/git/sources/binutils-
> gdb/gas/testsuite/gas/all/local-
> > > label-overflow.s
> > > /export/gnu/import/git/sources/binutils-
> gdb/gas/testsuite/gas/all/local-
> > > label-overflow.s:
> > > Assembler messages:
> > > /export/gnu/import/git/sources/binutils-
> gdb/gas/testsuite/gas/all/local-
> > > label-overflow.s:1:
> > > Error: local label too large near 888888888888888888888888888:
> > >
> > > /export/gnu/import/git/sources/binutils-
> gdb/gas/testsuite/gas/all/local-
> > > label-overflow.s:1:
> > > Error: local label too large near 888888888888888888:
> > >
> > > /export/gnu/import/git/sources/binutils-
> gdb/gas/testsuite/gas/all/local-
> > > label-overflow.s:1:
> > > Error: local label too large near 888888888:
> > >
> > > I am checking in this to fix it:
> > >
> > > diff --git a/gas/read.c b/gas/read.c
> > > index bd896580e5..cb41cc0685 100644
> > > --- a/gas/read.c
> > > +++ b/gas/read.c
> > > @@ -1227,7 +1227,10 @@ read_a_source_file (const char *name)
> > >
> > >          /* Overflow: stop processing the label.  */
> > >          if (temp == -1)
> > > -     continue;
> > > +     {
> > > +       ignore_rest_of_line ();
> > > +       continue;
> > > +     }
> > >
> > >          if (LOCAL_LABELS_DOLLAR
> > >         && *input_line_pointer == '$'
> > >
> > > > Is it worth backporting?
> > > >
> > > > Alex
> > >
> > >
> > >
> > > --
> > > H.J.
> 
> 
> 
> --
> H.J.