[Bug]: `raw_log` type is `string` but can contain invalid UTF-8

[fmorency]

Is there an existing issue for this?

• I have searched the existing issues

What happened?

The raw_log field of TxResponse in abci.proto is of string type. The string type, as defined by the Protocol Buffer 3 Language specification 1,

A string must always contain UTF-8 encoded or 7-bit ASCII text, and cannot be longer than 232`.

However, the CosmosSDK can encode raw_log containing invalid UTF-8 strings, as shown in 2.

Cosmos SDK Version

0.50.8

How to reproduce?

No response

[julienrbrt]

I think we just print the error in case of an error:

cosmos-sdk/baseapp/errors.go

Line 30 in e0f644b

Log: log,

.

It's on the module to properly format their errors. I do see we've made a mistake on x/group about that, so thanks for pointing it out.

[fmorency]

Thank you @julienrbrt.

It's on the module to properly format their errors.

protobuf>=1.20.01 errors out when trying to Marshal/Unmarshal2 strings containing invalid UTF-8. CosmosSDK uses a mix of gogoproto and pulsar. Shouldn't CosmosSDK enforce this UTF-8 check to conform to the protocol buffer specification? It would mitigate errors like what happened in the x/group module.

[julienrbrt]

IMHO that's not the role of the SDK to ensure that but on module dev, it would as well make it harder to debug for developers.
We shouldn't have to validate each string field.

[fmorency]

Hey @julienrbrt, I might've misexplained.

I agree with you that it is not the role of the SDK to implement the check; it is the role of the protobuf library the SDK uses, namely gogoproto, which Cosmos is maintaining.

The gogoproto repository references UTF-8 validation 1 2. There's even a test 3 for catching invalid UTF-8 strings!

I don't know why one can marshal invalid strings in the SDK, but I strongly believe this is wrong. While developers are responsible for writing their error messages, one should not be required to have the protocol buffer specification in mind.

Allowing illegal strings adds a burden on external projects and leads to workaround such as 4.

Again, the protocol buffer specification clearly states

A string must always contain UTF-8 encoded or 7-bit ASCII text.

Thanks!

[julienrbrt]

I see, but I still don't think we should add checks, to be honest.

@kocubinski or @alpe are you opinionated on that matter?