Subject: Re: bug#77325: Crash in Fjson_parse_buffer: ZV changes underneath it?

From: Pip Cet <[email-masked]>
Date: Sat, 29 Mar 2025 15:37:17 +0000

"Eli Zaretskii" <[email-masked]> writes:

>> Date: Sat, 29 Mar 2025 11:53:49 +0000
>> From: Pip Cet <[email-masked]>
>> Cc: [email-masked], [email-masked]
>>
>> "Eli Zaretskii" <[email-masked]> writes:
>>
>> 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.
>
> Sorry, I've misread your patch.

No problem at all.

The code does seem a bit complicated for what it's trying to achieve, to
be honest. I think it'd be clearer just to write:

  unsigned char *begin = PT_ADDR;
  unsigned char *end = min (GPT_ADDR, ZV_ADDR);
  unsigned char *secondary_begin = min (GAP_END_ADDR, ZV_ADDR));
  unsigned char *secondary_end = ZV_ADDR;

  json_parser_init (&p, conf, begin, end, secondary_begin,
		    secondary_end);

json_parser_init fixes up secondary_begin and secondary_end to be NULL
pointers in this case.

("min (GAP_END_ADDR, ZV_ADDR)" could also be replaced by just
"GAP_END_ADDR", since json_parser_init fixes paradoxical ranges itself,
but I think we should avoid ever creating those where possible.)

But if that's not okay, let me know and I'll push the other patch.

Pip