GNU bug report logs - #77325
Crash in Fjson_parse_buffer: ZV changes underneath it?

Previous Next

Full log

View this message in rfc822 format

From: Pip Cet <pipcet <at> protonmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: dancol <at> dancol.org, 77325 <at> debbugs.gnu.org
Subject: bug#77325: Crash in Fjson_parse_buffer: ZV changes underneath it?
Date: Sat, 29 Mar 2025 11:53:49 +0000

"Eli Zaretskii" <eliz <at> gnu.org> writes:

>> Date: Fri, 28 Mar 2025 17:00:24 +0000
>> From: Pip Cet <pipcet <at> protonmail.com>
>> Cc: dancol <at> dancol.org, 77325 <at> debbugs.gnu.org
>>
>> "Eli Zaretskii" <eliz <at> gnu.org> writes:
>>
>> >> Date: Fri, 28 Mar 2025 15:05:22 +0000
>> >> From: Pip Cet <pipcet <at> protonmail.com>
>> >> Cc: Eli Zaretskii <eliz <at> gnu.org>, 77325 <at> debbugs.gnu.org
>> >>
>> >> "Daniel Colascione" <dancol <at> dancol.org> writes:
>> >>
>> >> > Didn't have a good repro.  Pip's fix works though.  I was barking up
>> >> > the wrong tree: I'm parsing JSON out of a process buffer in a loop and
>> >> > dispatching commands as they come in. One of these commands switched the
>> >> > buffer, so in the next iteration of the loop, I started parsing JSON out
>> >> > of some other random buffer.  It just so happened that other buffer was
>> >> > narrowed, so we crashed.  I'll let Pip do the honors of checking in the
>> >> > fix if he wants.
>> >>
>> >> Eli, is that okay?  I'll simplify the else branch, which has an
>> >> unnecessary "else if" in the original patch.
>> >
>> > Can we discuss why you don't simply replace Z with ZV and BEG with
>> > BEGV?  I'm not sure I understand some parts of the change you
>> > proposed.
>>
>> Because the code assumes GPT <= Z, and GPT <= ZV isn't always true.
>
> Sorry, I don't understand: if the gap is beyond ZV, then there's no
> "secondary" region for json.c's purposes, which AFAIU is the only
> thing json-parse-buffer needs to know.

It's about the primary selection, not the secondary one.

The code currently reads:

  unsigned char *begin = PT_ADDR;
  unsigned char *end = GPT_ADDR;
  unsigned char *secondary_begin = NULL;
  unsigned char *secondary_end = NULL;
  if (GPT_ADDR < Z_ADDR)
    {
      secondary_begin = GAP_END_ADDR;
      if (secondary_begin < PT_ADDR)
	secondary_begin = PT_ADDR;
      secondary_end = Z_ADDR;
    }

Simply replacing Z_ADDR by ZV_ADDR would still set up the primary region
to be [PT, GPT].  If GPT > ZV, that would mean that the primary region
extends beyond ZV, which would mean we parse buffer text that should be
inaccessible.

So, in this case, we need to limit the primary region to end at ZV_ADDR.
That's what my patch does.

The code for the secondary region is correct, if unnecessary because
sending up a paradoxical [GPT, ZV] range if ZV < GPT wouldn't hurt.

> In addition, the value of 'end' should be limited to not exceed
> ZV_ADDR.  Or what am I missing?

That's what my patch does, yes.

> IOW, why does json-parse-buffer ignore the restriction?  No other
> primitive does, with rare exceptions that are explicitly documented.

I assumed it was an accident, and that's why my patch changes it to
respect the restriction.

Pip

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.