sparse ctags segfaults (latest git)

sparse ctags segfaults (latest git)

[foobar]

git version 08890dcd7cc77b5d776768775484ed60fc23f08e
linux x64_64 (using musl libc)

gdb --args ./ctags ctags.c

Reading symbols from /home/foobar/sparse/ctags...done.
(gdb) r
Starting program: /home/foobar/sparse/./ctags ctags.c
warning: no loadable sections found in added symbol-file system-supplied DSO at 0x7ffff7ff9000
/usr/include/stdint.h:105:5: warning: constant 0xffffffffffffffffu is so big it is unsigned long
/usr/include/stdint.h:105:20: warning: constant 0xffffffffffffffffu is so big it is unsigned long

Program received signal SIGSEGV, Segmentation fault.
0x0000000000401703 in examine_symbol (sym=0x7ffff7fc2ff0) at ctags.c:138
138                     if (base->type == SYM_FN)
(gdb) p base
$1 = (struct symbol *) 0x0
(gdb) bt
#0  0x0000000000401703 in examine_symbol (sym=0x7ffff7fc2ff0) at ctags.c:138
#1  0x00000000004018c8 in examine_namespace (sym=0x7ffff7fc2ff0) at ctags.c:193
#2  0x0000000000401999 in examine_symbol_list (list=0x7ffff7fd7010) at ctags.c:209
#3  0x0000000000401aac in main (argc=2, argv=0x7fffffffeb68) at ctags.c:223
(gdb) p sym
$2 = (struct symbol *) 0x7ffff7fc2ff0
(gdb) l
133             add_tag(sym);
134             base = sym->ctype.base_type;
135
136             switch (sym->type) {
137             case SYM_NODE:
138                     if (base->type == SYM_FN)
139                             sym->kind = 'f';
140                     examine_symbol(base);
141                     break;
142             case SYM_STRUCT:
(gdb) 

[PATCH 0/2] fix crash in ctags.c

[Luc Van Oostenryck]

This series contains two patches fixing a null pointer
dereference in ctags.

Note: part of the problem here is that the ctags tools directly
      use global_scope->symbols and that the builtins are in
      the global scope. I think they should be in a separate
      scope, but it'll be another story.

These patches are available in the Git repository at:
  git://github.com/lucvoo/sparse-dev.git fix-ctags-crash
and soon will be pushed at:
  git://github.com/lucvoo/sparse.git     master

Luc Van Oostenryck (2):
  give a type to builtin functions
  ctags: avoid null deref

 builtin.c | 6 +++---
 ctags.c   | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

[PATCH 1/2] give a type to builtin functions

[Luc Van Oostenryck]

When creating builtin functions via init-builtins(), it shouldn't
really be needed to give them a type because their purpose is only
as a placeholder for the symbol->op->{evaluate,expand,..}. Also,
they should be part of the builtin_scope and never be visible.

However, currently the global scope is the same as the builtin_scope
and these symbols are thus visible via global_scope->symbols where
the missing type can be a problem.

Fix this to giving to the builtin functions which hadn't a type
the same phony type as the other functions had.

Reported-by: foobar <foobar@redchan.it>
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 builtin.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin.c b/builtin.c
index 9f90926cb..acb2fabe9 100644
--- a/builtin.c
+++ b/builtin.c
@@ -240,9 +240,9 @@ static struct sym_init {
 	{ "__builtin_warning", &builtin_fn_type, MOD_TOPLEVEL, &warning_op },
 	{ "__builtin_expect", &builtin_fn_type, MOD_TOPLEVEL, &expect_op },
 	{ "__builtin_choose_expr", &builtin_fn_type, MOD_TOPLEVEL, &choose_op },
-	{ "__builtin_bswap16", NULL, MOD_TOPLEVEL, &bswap_op },
-	{ "__builtin_bswap32", NULL, MOD_TOPLEVEL, &bswap_op },
-	{ "__builtin_bswap64", NULL, MOD_TOPLEVEL, &bswap_op },
+	{ "__builtin_bswap16", &builtin_fn_type, MOD_TOPLEVEL, &bswap_op },
+	{ "__builtin_bswap32", &builtin_fn_type, MOD_TOPLEVEL, &bswap_op },
+	{ "__builtin_bswap64", &builtin_fn_type, MOD_TOPLEVEL, &bswap_op },
 	{ NULL,		NULL,		0 }
 };
 
-- 
2.15.0

[PATCH 2/2] ctags: avoid null deref

[Luc Van Oostenryck]

Protect the dereference of null base_type.

Reported-by: foobar <foobar@redchan.it>
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 ctags.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ctags.c b/ctags.c
index 9ec6b3c37..30976542d 100644
--- a/ctags.c
+++ b/ctags.c
@@ -135,7 +135,7 @@ static void examine_symbol(struct symbol *sym)
 
 	switch (sym->type) {
 	case SYM_NODE:
-		if (base->type == SYM_FN)
+		if (base && base->type == SYM_FN)
 			sym->kind = 'f';
 		examine_symbol(base);
 		break;
-- 
2.15.0

Re: [PATCH 2/2] ctags: avoid null deref

[foobar]

Luc, thank you for addressing this issue so quickly!

On Tue,  2 Jan 2018 16:11:16 +0100
Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote:

> Protect the dereference of null base_type.

I wonder, maybe it would be better not to add this protection so future similar bugs can be found ?

Re: [PATCH 1/2] give a type to builtin functions

[foobar]

On Tue,  2 Jan 2018 16:11:15 +0100
Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote:

> When creating builtin functions via init-builtins(), it shouldn't
> really be needed to give them a type because their purpose is only
> as a placeholder for the symbol->op->{evaluate,expand,..}. Also,
> they should be part of the builtin_scope and never be visible.

yeah, it seems odd (and annoying at the same time) that ctags emits internal symbols __builtin_bswap16 and similar stuff, unlike exuberant ctags.

Re: [PATCH 1/2] give a type to builtin functions

[Luc Van Oostenryck]

On Wed, Jan 03, 2018 at 03:57:43PM +0000, foobar wrote:
> On Tue,  2 Jan 2018 16:11:15 +0100
> Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote:
> 
> > When creating builtin functions via init-builtins(), it shouldn't
> > really be needed to give them a type because their purpose is only
> > as a placeholder for the symbol->op->{evaluate,expand,..}. Also,
> > they should be part of the builtin_scope and never be visible.
> 
> yeah, it seems odd (and annoying at the same time) that ctags emits internal symbols __builtin_bswap16 and similar stuff, unlike exuberant ctags.

There is several ways to change this but I need to think a bit
about it.

There is also that sparse's ctags work on the preprocessed source
while, I think, other ctags work purely on the lexical level,
thus on non-preprocessed sources.

Ciao and hanks for the bug repport
-- Luc

Re: [PATCH 2/2] ctags: avoid null deref

[Luc Van Oostenryck]

On Wed, Jan 03, 2018 at 03:55:23PM +0000, foobar wrote:
> Luc, thank you for addressing this issue so quickly!
> 
> On Tue,  2 Jan 2018 16:11:16 +0100
> Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote:
> 
> > Protect the dereference of null base_type.
> 
> I wonder, maybe it would be better not to add this protection so future similar bugs can be found ?

When there have been a previous error, a warning is issued and
often (but depending on the exact nature of the problem)
the returned pointer is null. So it's kinda normal to have
such pointer and we need to test them before continuing.
So, I really think the patch here is needed.

-- Luc

Re: sparse ctags segfaults (latest git)

[Christopher Li]

On Tue, Jan 2, 2018 at 6:58 AM, foobar <foobar@redchan.it> wrote:
> git version 08890dcd7cc77b5d776768775484ed60fc23f08e
> linux x64_64 (using musl libc)
>
> gdb --args ./ctags ctags.c
>
> Reading symbols from /home/foobar/sparse/ctags...done.
> (gdb) r
> Starting program: /home/foobar/sparse/./ctags ctags.c
> warning: no loadable sections found in added symbol-file system-supplied DSO at 0x7ffff7ff9000
> /usr/include/stdint.h:105:5: warning: constant 0xffffffffffffffffu is so big it is unsigned long
> /usr/include/stdint.h:105:20: warning: constant 0xffffffffffffffffu is so big it is unsigned long

Thanks for the bug report.

Chris

Re: [PATCH 0/2] fix crash in ctags.c

[Christopher Li]

On Tue, Jan 2, 2018 at 11:11 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> This series contains two patches fixing a null pointer
> dereference in ctags.
>
> Note: part of the problem here is that the ctags tools directly
>       use global_scope->symbols and that the builtins are in
>       the global scope. I think they should be in a separate
>       scope, but it'll be another story.
>
> These patches are available in the Git repository at:
>   git://github.com/lucvoo/sparse-dev.git fix-ctags-crash
> and soon will be pushed at:
>   git://github.com/lucvoo/sparse.git     master
>
> Luc Van Oostenryck (2):
>   give a type to builtin functions
>   ctags: avoid null deref

Sorry for the late reply. I will take a look at it tomorrow.
Too late for me to think straight now.

Thanks for the patches

Chris

Re: [PATCH 1/2] give a type to builtin functions

[Christopher Li]

On Tue, Jan 2, 2018 at 11:11 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> When creating builtin functions via init-builtins(), it shouldn't
> really be needed to give them a type because their purpose is only
> as a placeholder for the symbol->op->{evaluate,expand,..}. Also,
> they should be part of the builtin_scope and never be visible.
>
> However, currently the global scope is the same as the builtin_scope
> and these symbols are thus visible via global_scope->symbols where
> the missing type can be a problem.
>
> Fix this to giving to the builtin functions which hadn't a type
> the same phony type as the other functions had.

My casual look this seem fine to me.

Thanks for the fix.

Chris

Re: [PATCH 1/2] give a type to builtin functions

[Christopher Li]

On Thu, Jan 4, 2018 at 12:41 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>>
>> yeah, it seems odd (and annoying at the same time) that ctags emits internal symbols __builtin_bswap16 and similar stuff, unlike exuberant ctags.
>
> There is several ways to change this but I need to think a bit
> about it.

The builtin needs to be in the global scope because we don't want to lose
those function when sparse switch to the next C files.
>
> There is also that sparse's ctags work on the preprocessed source
> while, I think, other ctags work purely on the lexical level,
> thus on non-preprocessed sources.

Yes, that is by design. It is a useful feature to find out those
symbol generated from the macro expansion.

I have one idea, I haven't look closely how piratical it is.
I think we can filter out the __buildin_xxx functions by looking at
the some builtin flags in the symbol.

Chris