Skip to content

fix(types): initialize Quote field in Message.UnmarshalJSON#42

Closed
Polpetta wants to merge 1 commit into
chatmail:mainfrom
Polpetta:fix/nil-reference-quote-message
Closed

fix(types): initialize Quote field in Message.UnmarshalJSON#42
Polpetta wants to merge 1 commit into
chatmail:mainfrom
Polpetta:fix/nil-reference-quote-message

Conversation

@Polpetta

Copy link
Copy Markdown

I've initialised the Quote field to avoid nil pointer dereferences when unmarshaling messages with quote data. Previously, if a message contained quote information, the Quote field would remain nil, causing potential panics when accessing quote properties.

Add test case to verify that messages with quotes are properly unmarshaled and the Quote field is correctly initialized. I've tried to keep the same style of yours, hope it's good enough.

Note

I've tested this manually as well by spinning up my bot locally and it seems to do the trick, with the bug disappearing completely.


Closes #41

Initialize the Quote field to avoid nil pointer dereferences when
unmarshaling messages with quote data. Previously, if a message
contained quote information, the Quote field would remain nil,
causing potential panics when accessing quote properties.

Add test case to verify that messages with quotes are properly
unmarshaled and the Quote field is correctly initialized.
@ccclxxiii ccclxxiii self-requested a review May 21, 2026 12:42
@ccclxxiii

Copy link
Copy Markdown

thanks for jumping on this. i can confirm this patch fixes the quoted-message panic path, but I think the unconditional allocation changes the semantics for messages without a quote.

right now the PR does:

s.Quote = new(MessageQuote)
if len(raw.Quote) > 0 && string(raw.Quote) != "null" {
	if err := unmarshalMessageQuote(raw.Quote, s.Quote); err != nil {
		return err
	}
}

it means msg.Quote != nil will be true even when the JSON has no quote field or has "quote": null. downstream code may reasonably treat msg.Quote != nil as “this message has a quote”, then hit a nil interface value inside the pointer.

i think safer fix is to allocate only when a non-null quote is present, and explicitly clear stale values on reused Message structs:

s.Quote = nil
if len(raw.Quote) > 0 && string(raw.Quote) != "null" {
	var quote MessageQuote
	if err := unmarshalMessageQuote(raw.Quote, &quote); err != nil {
		return err
	}
	s.Quote = &quote
}

it can preserve the expected optional-field behavior:

no quote field => msg.Quote == nil
"quote": null => msg.Quote == nil
non-null quote object => msg.Quote != nil and decodes correctly

it would also be good to add tests for the absent and null cases, not just the quoted case.

@ccclxxiii ccclxxiii requested a review from adbenitez May 21, 2026 12:54
@Polpetta

Copy link
Copy Markdown
Author

Hey @ccclxxiii,
regarding this part:

it means msg.Quote != nil will be true even when the JSON has no quote field or has "quote": null. downstream code may reasonably treat msg.Quote != nil as “this message has a quote”, then hit a nil interface value inside the pointer.

i think safer fix is to allocate only when a non-null quote is present, and explicitly clear stale values on reused Message structs:

Thanks for the heads up, I totally glossed over these cases! Just noticed an NPE and rushed to fix it without properly think ahead. Will try to update the PR applying your better thought suggestions + adding some tests to it asap

@adbenitez

adbenitez commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

s.Quote = nil

@ccclxxiii this is unnecessary or??? go always assign variables IIRC so a pointer is always nil without needing initialization 🤔

@Polpetta, @ccclxxiii thanks for bringing this up and trying to fix, but this code is automatically generated from the OpenRPC schema specification file that deltachat-rpc-server binary generates

I have fixed the generator at chatmail/dcrpcgen#9

and I created a PR updating the auto-generated code and adding a test for this problem at #43

we can close this one here, and sorry for taking so long to look into this and reply! thanks a lot! 🙏

@adbenitez adbenitez closed this Jun 1, 2026
@Polpetta

Polpetta commented Jun 9, 2026

Copy link
Copy Markdown
Author

Hey @adbenitez no issue whatsoever! Thanks for having a look at this and spending time in finding and fixing this issue at the root 🙌

Could you please make a release for this? So that people can update their rpc-clients/bots that rely on reply via quote

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SIGSEV when receving message with a quote

3 participants