fix(postgres): don't panic if M or C Notice fields are not UTF-8
#3346
Conversation
This has been observed with an old version of PostgreSQL (11.0.4) running on Windows Server 2016 with windows-1252 encoding and French locale. This change replaces invalid UTF-8 fields with a default string, so the other fields can still be read if they are valid.
There was a problem hiding this comment.
I think it would be a little surprising for .code() or .message() to return an arbitrary string rather than the one that's expected.
Depending on how it's handled at the application level, the actual error might end up discarded or buried because Error::Database() is often going to be treated as a recoverable error.
code and message should be checked for validity during decoding and return Error::Protocol() if they fail. I think if code is ever not valid ASCII then that's a Postgres bug because it's not supposed to be localized.
I would prefer to keep the panic in get_cached_str() since I consider it a bug for it not to be valid UTF-8 at that point. In the event we do need to add decoding for additional fields, just leave a comment in the Decode impl saying to be sure to check for UTF-8 validity or else it may cause panics later.
|
It's a fair point that it might be useful to still be able to read the other fields, but I would just include those strings in the protocol error if they've been decoded already. Maybe the decode routine should be restructured so that it continues decoding the other fields on an error. |
…t UTF-8" This reverts commit 362ca98.
Otherwise, we return the invalid UTF-8 error to avoid panicking later.
…aunchbadge#3346) * fix(postgres): don't panic if `M` or `C` Notice fields are not UTF-8 This has been observed with an old version of PostgreSQL (11.0.4) running on Windows Server 2016 with windows-1252 encoding and French locale. This change replaces invalid UTF-8 fields with a default string, so the other fields can still be read if they are valid. * Revert "fix(postgres): don't panic if `M` or `C` Notice fields are not UTF-8" This reverts commit 362ca98. * Check that Notice M and C fields are valid UTF-8 Otherwise, we return the invalid UTF-8 error to avoid panicking later.
…aunchbadge#3346) * fix(postgres): don't panic if `M` or `C` Notice fields are not UTF-8 This has been observed with an old version of PostgreSQL (11.0.4) running on Windows Server 2016 with windows-1252 encoding and French locale. This change replaces invalid UTF-8 fields with a default string, so the other fields can still be read if they are valid. * Revert "fix(postgres): don't panic if `M` or `C` Notice fields are not UTF-8" This reverts commit 362ca98. * Check that Notice M and C fields are valid UTF-8 Otherwise, we return the invalid UTF-8 error to avoid panicking later.
This has been observed with an old version of PostgreSQL (11.0.4) running on Windows Server 2016 with windows-1252 encoding and French locale.
This change replaces invalid UTF-8 fields with a default string, so the other fields can still be read if they are valid.
fixes #3345