Re: [BUG] SEGV getop() src/bltin/test.c:171:7

Hi,

On 31/07/2025 12:30, Aleksander Ushakov wrote:
> Hello Dash maintainers,
> 
> I encountered a bug in Dash in commit b4ef25d7 and would like to report
> it. The details are provided below.
> 
> AddressSanitizer:DEADLYSIGNAL
> 
> =================================================================
> ==18517==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000a09
> (pc 0x58db5c62ec40 bp 0x7ffe4f593610 sp 0x7ffe4f592db0 T0)
> ==18517==The signal is caused by a READ memory access.
> ==18517==Hint: address points to the zero page.
>        #0 0x58db5c62ec40 in strcmp (/upstream/test/dash/src/dash+0x51c40)
> (BuildId: 535a280f2cc73be57729926bfac9fbc4801d63be)
>        #1 0x58db5c72ef74 in getop
> /upstream/test/dash/src/bltin/test.c:171:7
>        #2 0x58db5c72f022 in t_lex
> /upstream/test/dash/src/bltin/test.c:444:7
>        #3 0x58db5c72f535 in aexpr
> /upstream/test/dash/src/bltin/test.c:269:7
>        #4 0x58db5c72f199 in oexpr
> /upstream/test/dash/src/bltin/test.c:252:10
>        #5 0x58db5c72ee30 in testcmd
> /upstream/test/dash/src/bltin/test.c:228:9
>        #6 0x58db5c6fee10 in evalbltin
> /upstream/test/dash/src/eval.c:975:12
>        #7 0x58db5c6fc183 in evalcommand
> /upstream/test/dash/src/eval.c:923:7
>        #8 0x58db5c6fa925 in evaltree /upstream/test/dash/src/eval.c:305:12
>        #9 0x58db5c717762 in cmdloop /upstream/test/dash/src/main.c:246:8
>        #10 0x58db5c71736f in main /upstream/test/dash/src/main.c:180:3
>        #11 0x76be525ad249 in __libc_start_call_main
> csu/../sysdeps/nptl/libc_start_call_main.h:58:16
>        #12 0x76be525ad304 in __libc_start_main
> csu/../csu/libc-start.c:360:3
>        #13 0x58db5c6165d0 in _start (/upstream/test/dash/src/dash+0x395d0)
> (BuildId: 535a280f2cc73be57729926bfac9fbc4801d63be)
> ==18517==Register values:
> rax = 0x000000005c74e201  rbx = 0x000058db5c74e260  rcx =
> 0x0000000000000000  rdx = 0x000058db5c74e6a0
> rdi = 0x0000000000000a09  rsi = 0x000058db5c74e260  rbp =
> 0x00007ffe4f593610  rsp = 0x00007ffe4f592db0
>     r8 = 0xf9f9f903f9f9f901   r9 = 0x00000b1beb8e1cd4  r10 =
> 0x00000b1beba13ac2  r11 = 0x00000b1beb8e1cd0
> r12 = 0x0000000000000000  r13 = 0x00007ffe4f5941c8  r14 =
> 0x0000000000000a09  r15 = 0x0000000000000000
> AddressSanitizer can not provide additional info.
> SUMMARY: AddressSanitizer: SEGV (/upstream/test/dash/src/dash+0x51c40)
> (BuildId: 535a280f2cc73be57729926bfac9fbc4801d63be) in strcmp
> ==18517==ABORTING
> 
> Environment:
> 
> Debian-12, x86-64
> clang-19 compiler
> 
> Steps to reproduce:
> 
> ./autogen.sh
> CC=clang CFLAGS=" -fsanitize=address -g " ./configure --disable-fnmatch
> --disable-lineno --disable-glob
> make
> cd src
> echo -e '0000000000000000000000000000000000000000"$IFS"\ntest "" -a' |
> ./dash

Thanks for the report.

> I think I fixed the problem here:

Did you fix it, or did you ask AI to fix it? Asking because the 
description reads very much as if it is AI-generated, and if it is, that 
really should be disclosed in my opinion.

The patch does not look complete (-o has the same problem and is not 
fixed), and does not look right. It's not enough to not crash, we have 
to actually do the right thing. Here, doing the right thing is reporting 
the error for the invalid use of the test command.

For e.g. test x =, dash already reports:

   $ src/dash -c 'test x ='
   src/dash: 1: test: =: argument expected

For test x -a and test x -o, dash could do the same, as with the 
attached patch.

Note: it is not necessary to check t_wp[1] because t_lex supports null 
pointers and returns EOI. We could use this for the check too, we could 
check for EOI after the last t_lex, rather than what my patch does. I am 
not sure what is better, the effect would be the same.

Cheers,
Harald van Dijk

From 34d861fd50abf4693eb9e424a9cfe757e51542ce Mon Sep 17 00:00:00 2001
From: Harald van Dijk <[email protected]>
Date: Thu, 31 Jul 2025 13:18:28 +0100
Subject: [PATCH] test: avoid out of bounds access

When a boolean operator appears at the end of an expression, make sure we
do not try to continue parsing the expression past its end.
---
 src/bltin/test.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/bltin/test.c b/src/bltin/test.c
index 6d844f1..0876559 100644
--- a/src/bltin/test.c
+++ b/src/bltin/test.c
@@ -253,6 +253,8 @@ oexpr(enum token n)
 		n = t_lex(t_wp + 1);
 		if (n != BOR)
 			break;
+		if (t_wp[2] == NULL)
+			syntax(t_wp[1], "argument expected");
 		n = t_lex(t_wp += 2);
 	}
 	return res;
@@ -269,6 +271,8 @@ aexpr(enum token n)
 		n = t_lex(t_wp + 1);
 		if (n != BAND)
 			break;
+		if (t_wp[2] == NULL)
+			syntax(t_wp[1], "argument expected");
 		n = t_lex(t_wp += 2);
 	}
 	return res;
-- 
2.47.2