Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1pkSy0-0000YA-Jb for pgsql-hackers@arkaria.postgresql.org; Thu, 06 Apr 2023 17:00:08 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1pkSxz-0001Zj-E2 for pgsql-hackers@arkaria.postgresql.org; Thu, 06 Apr 2023 17:00:07 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1pkSxy-0001ZJ-Uo for pgsql-hackers@lists.postgresql.org; Thu, 06 Apr 2023 17:00:07 +0000 Received: from mail-lj1-x22e.google.com ([2a00:1450:4864:20::22e]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1pkSxw-0002iG-JZ for pgsql-hackers@lists.postgresql.org; Thu, 06 Apr 2023 17:00:05 +0000 Received: by mail-lj1-x22e.google.com with SMTP id bx10so22981878ljb.8 for ; Thu, 06 Apr 2023 10:00:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; t=1680800403; h=in-reply-to:from:references:cc:to:content-language:subject :user-agent:mime-version:date:message-id:from:to:cc:subject:date :message-id:reply-to; bh=gXBEImY3atbXtZHPeLvFrSXneO5pLcNy2QncXhBlZUc=; b=MaqHZ915f6pRUYBxlqrP2iFAcnLYtAiLUNukW50ZPbvmLV1i65T6KZlzrcPTMWW8Ox irkC94BBuoyHCum40Frnqt/8BatYddYX9snInombWq/RcHUTfwKVogWgySQ9MU54WTSq qi7DwCBdq4QJExyOgbgg1yWSEnU4/O5SfeoHCDNvjbe87Qi0zW2m6quBFrlwyKNH+J2U qX7/8iCHNXx2bohNom6gt85zso6yu81PkJFTVzr15379UnUE9gMEI4a4ztKzqMmMHUmA fsLkk5Y7/ZiXqISrug7vRUlWx0fZIctINaZ4iWUGNULK3JPPPZoXbpQXyKV6IzKyQ+pb QZ2w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680800403; h=in-reply-to:from:references:cc:to:content-language:subject :user-agent:mime-version:date:message-id:x-gm-message-state:from:to :cc:subject:date:message-id:reply-to; bh=gXBEImY3atbXtZHPeLvFrSXneO5pLcNy2QncXhBlZUc=; b=30HiDDIdEiHf35CSnximux23yLHf/99RyKSGaixRTWbIMMO+mG+2MZ/j191ysOX2Nc HHS7CWVk40DKQMTNZtfHQsRyNktJH3atJjW+JxHr+y9ILGNwIn4gD7zcaehCjC0UP2nv iD1ekO7r8K8OhQHav9+tkhNqVm7JbYkPN5NmaCK3fat+RTnEJmhBUzGnt5VcLrN9fCck 0t1nyMaUG++fD4vmL5N5sjwqLjShUVBmtxPKGq6ihbyTW5IkGhD2EUM5fZkuS4tRrHQH et53z2BPcNOAdnCLT+qgQTlbjV8LA88Brt83cTxLegs+NmsXcc04nw3b1ViDFiaUeKHu LsVQ== X-Gm-Message-State: AAQBX9eiUtkxP9ZAnmqOHwpSh+FiiGeUo+h2bNntl1Dn7IibKZ1twFU0 +y8fGVFtxIQPoXyFzwp3xTw= X-Google-Smtp-Source: AKy350bt39JatIi9HMcmlywN85cJV11MvZy7Ja7wcCPXvV3eSgGFITI7xDfeNQyH75MGl2hxiH+eoA== X-Received: by 2002:a2e:9b01:0:b0:298:ac32:fe6a with SMTP id u1-20020a2e9b01000000b00298ac32fe6amr3110715lji.38.1680800402500; Thu, 06 Apr 2023 10:00:02 -0700 (PDT) Received: from [1.0.0.7] ([178.155.30.94]) by smtp.gmail.com with ESMTPSA id f20-20020a2e9e94000000b00299ab697a9esm352073ljk.111.2023.04.06.10.00.01 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 06 Apr 2023 10:00:01 -0700 (PDT) Content-Type: multipart/alternative; boundary="------------9CygqyFM4rvT0i4r4ivKFk8j" Message-ID: Date: Thu, 6 Apr 2023 20:00:00 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.2 Subject: Re: Rethinking the implementation of ts_headline() Content-Language: en-US To: Tom Lane , Alvaro Herrera Cc: pgsql-hackers@lists.postgresql.org, sebastian.patino-lang@posteo.net References: <20230119101629.vjdjktbol273zjpm@alvherre.pgsql> <4066528.1674144810@sss.pgh.pa.us> From: Alexander Lakhin In-Reply-To: <4066528.1674144810@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk This is a multi-part message in MIME format. --------------9CygqyFM4rvT0i4r4ivKFk8j Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi, 19.01.2023 19:13, Tom Lane wrote: > Alvaro Herrera writes: > >> Anyway, I don't think this needs to stop your current patch. > Many thanks for looking at it! I've found that starting from commit 5a617d75 this query: SELECT ts_headline('english', 'To be, or not to be', to_tsquery('english', 'or')); invokes a valgrind-detected error: ==00:00:00:03.950 3241424== Invalid read of size 1 ==00:00:00:03.950 3241424==    at 0x8EE2A9: TS_execute_locations_recurse (tsvector_op.c:2046) ==00:00:00:03.950 3241424==    by 0x8EE220: TS_execute_locations (tsvector_op.c:2017) ==00:00:00:03.950 3241424==    by 0x77EAC4: prsd_headline (wparser_def.c:2677) ==00:00:00:03.950 3241424==    by 0x94536C: FunctionCall3Coll (fmgr.c:1173) ==00:00:00:03.950 3241424==    by 0x778648: ts_headline_byid_opt (wparser.c:322) ==00:00:00:03.950 3241424==    by 0x9440F5: DirectFunctionCall3Coll (fmgr.c:836) ==00:00:00:03.950 3241424==    by 0x778763: ts_headline_byid (wparser.c:343) ==00:00:00:03.950 3241424==    by 0x4AC9F2: ExecInterpExpr (execExprInterp.c:752) ==00:00:00:03.950 3241424==    by 0x4AEDFE: ExecInterpExprStillValid (execExprInterp.c:1838) ==00:00:00:03.950 3241424==    by 0x636A7E: ExecEvalExprSwitchContext (executor.h:344) ==00:00:00:03.950 3241424==    by 0x63E92D: evaluate_expr (clauses.c:4843) ==00:00:00:03.950 3241424==    by 0x63DA53: evaluate_function (clauses.c:4345) ... (Initially I had encountered an asan-detected heap-buffer-overflow with a more informative document.) But the less-verbose call: SELECT ts_headline('', ''); discovers a different error even on 5a617d75~1: ==00:00:00:04.113 3139158== Conditional jump or move depends on uninitialised value(s) ==00:00:00:04.113 3139158==    at 0x77B44F: mark_fragment (wparser_def.c:2100) ==00:00:00:04.113 3139158==    by 0x77E2F2: mark_hl_words (wparser_def.c:2519) ==00:00:00:04.113 3139158==    by 0x77E891: prsd_headline (wparser_def.c:2610) ==00:00:00:04.113 3139158==    by 0x944B68: FunctionCall3Coll (fmgr.c:1173) ==00:00:00:04.113 3139158==    by 0x778648: ts_headline_byid_opt (wparser.c:322) ==00:00:00:04.113 3139158==    by 0x9438F1: DirectFunctionCall3Coll (fmgr.c:836) ==00:00:00:04.113 3139158==    by 0x7787B6: ts_headline (wparser.c:352) ==00:00:00:04.113 3139158==    by 0x4AC9F2: ExecInterpExpr (execExprInterp.c:752) ==00:00:00:04.113 3139158==    by 0x4AEDFE: ExecInterpExprStillValid (execExprInterp.c:1838) ==00:00:00:04.113 3139158==    by 0x50BD0C: ExecEvalExprSwitchContext (executor.h:344) ==00:00:00:04.113 3139158==    by 0x50BD84: ExecProject (executor.h:378) ==00:00:00:04.113 3139158==    by 0x50BFBB: ExecResult (nodeResult.c:136) ==00:00:00:04.113 3139158== I've reproduced it on REL9_4_STABLE (REL9_4_15) and it looks like the defect in mark_hl_words():         int                     bestb = -1,                                 beste = -1;         int                     bestlen = -1;         int                     pose = 0, ...         if (highlight == 0)         {                 while (hlCover(prs, query, &p, &q))                 { ...                 if (bestlen < 0)                 {                         curlen = 0;                         for (i = 0; i < prs->curwords && curlen < min_words; i++)                         {                                 if (!NONWORDTOKEN(prs->words[i].type))                                         curlen++;                                 pose = i;                         }                         bestb = 0;                         beste = pose;                 } ... // here we have bestb == 0, beste == 0, but no prs->words in this case         for (i = bestb; i <= beste; i++)         {                 if (prs->words[i].item)                         prs->words[i].selected = 1; exists since 2a0083ede. (Sorry for the distraction.) Best regards, Alexander --------------9CygqyFM4rvT0i4r4ivKFk8j Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit
Hi,

19.01.2023 19:13, Tom Lane wrote:
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:


Anyway, I don't think this needs to stop your current patch.

Many thanks for looking at it!

I've found that starting from commit 5a617d75 this query:
SELECT ts_headline('english', 'To be, or not to be', to_tsquery('english', 'or'));

invokes a valgrind-detected error:
==00:00:00:03.950 3241424== Invalid read of size 1
==00:00:00:03.950 3241424==    at 0x8EE2A9: TS_execute_locations_recurse (tsvector_op.c:2046)
==00:00:00:03.950 3241424==    by 0x8EE220: TS_execute_locations (tsvector_op.c:2017)
==00:00:00:03.950 3241424==    by 0x77EAC4: prsd_headline (wparser_def.c:2677)
==00:00:00:03.950 3241424==    by 0x94536C: FunctionCall3Coll (fmgr.c:1173)
==00:00:00:03.950 3241424==    by 0x778648: ts_headline_byid_opt (wparser.c:322)
==00:00:00:03.950 3241424==    by 0x9440F5: DirectFunctionCall3Coll (fmgr.c:836)
==00:00:00:03.950 3241424==    by 0x778763: ts_headline_byid (wparser.c:343)
==00:00:00:03.950 3241424==    by 0x4AC9F2: ExecInterpExpr (execExprInterp.c:752)
==00:00:00:03.950 3241424==    by 0x4AEDFE: ExecInterpExprStillValid (execExprInterp.c:1838)
==00:00:00:03.950 3241424==    by 0x636A7E: ExecEvalExprSwitchContext (executor.h:344)
==00:00:00:03.950 3241424==    by 0x63E92D: evaluate_expr (clauses.c:4843)
==00:00:00:03.950 3241424==    by 0x63DA53: evaluate_function (clauses.c:4345)
...
(Initially I had encountered an asan-detected heap-buffer-overflow with a
more informative document.)

But the less-verbose call:
SELECT ts_headline('', '');

discovers a different error even on 5a617d75~1:
==00:00:00:04.113 3139158== Conditional jump or move depends on uninitialised value(s)
==00:00:00:04.113 3139158==    at 0x77B44F: mark_fragment (wparser_def.c:2100)
==00:00:00:04.113 3139158==    by 0x77E2F2: mark_hl_words (wparser_def.c:2519)
==00:00:00:04.113 3139158==    by 0x77E891: prsd_headline (wparser_def.c:2610)
==00:00:00:04.113 3139158==    by 0x944B68: FunctionCall3Coll (fmgr.c:1173)
==00:00:00:04.113 3139158==    by 0x778648: ts_headline_byid_opt (wparser.c:322)
==00:00:00:04.113 3139158==    by 0x9438F1: DirectFunctionCall3Coll (fmgr.c:836)
==00:00:00:04.113 3139158==    by 0x7787B6: ts_headline (wparser.c:352)
==00:00:00:04.113 3139158==    by 0x4AC9F2: ExecInterpExpr (execExprInterp.c:752)
==00:00:00:04.113 3139158==    by 0x4AEDFE: ExecInterpExprStillValid (execExprInterp.c:1838)
==00:00:00:04.113 3139158==    by 0x50BD0C: ExecEvalExprSwitchContext (executor.h:344)
==00:00:00:04.113 3139158==    by 0x50BD84: ExecProject (executor.h:378)
==00:00:00:04.113 3139158==    by 0x50BFBB: ExecResult (nodeResult.c:136)
==00:00:00:04.113 3139158==

I've reproduced it on REL9_4_STABLE (REL9_4_15) and it looks like the defect
in mark_hl_words():
        int                     bestb = -1,
                                beste = -1;
        int                     bestlen = -1;
        int                     pose = 0,
...
        if (highlight == 0)
        {
                while (hlCover(prs, query, &p, &q))
                {
...
                if (bestlen < 0)
                {
                        curlen = 0;
                        for (i = 0; i < prs->curwords && curlen < min_words; i++)
                        {
                                if (!NONWORDTOKEN(prs->words[i].type))
                                        curlen++;
                                pose = i;
                        }
                        bestb = 0;
                        beste = pose;
                }
...
// here we have bestb == 0, beste == 0, but no prs->words in this case
        for (i = bestb; i <= beste; i++)
        {
                if (prs->words[i].item)
                        prs->words[i].selected = 1;

exists since 2a0083ede.
(Sorry for the distraction.)

Best regards,
Alexander
--------------9CygqyFM4rvT0i4r4ivKFk8j--