Skip to content

Conversation

@marksg07
Copy link
Collaborator

@marksg07 marksg07 commented Nov 15, 2024

Main repo PR: https://github.com/10gen/mongo/pull/29265

  • Moves structs needed by fle_crypto to headers
  • Adds validation functions for mc_FLE2IndexedEncryptedValueV2_t and mc_FLE2TagAndEncryptedMetadataBlock_t which are needed by fle_crypto


typedef struct _mc_FLE2IndexedEncryptedValueV2_t mc_FLE2IndexedEncryptedValueV2_t;
typedef enum {
kTypeInitV2,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're going to find that even these names are too generic given how C ODRs its enums into the global symbol space. It wasn't a problem when these were scoped to a single TU, but now that they're in a header, we should give them more distinctness.

e.g.

typedef enum {
  kFLE2IEVTypeInitV2,
  kFLE2IEVTypeEqualityV2,
  kFLE2IEVTypeRangeV2,
};

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed

}
}

bool mc_is_valid_bson_type(int bson_type) {
Copy link

@sgolemon-corp sgolemon-corp Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function seems.... sus.

You seem to just be checking if the value is in the explicit list of named numeric values.

Thing is that's not very meaningful in terms of if a type is "valid" or not.

(MAXKEY, MINKEY] (the set of 128 values from 0x80..0xFF) are all "valid" as BSON types, for example. Those two constants define the endpoints of the "User Defined BSONElements" range.

On the other side, EOD and UNDEFINED don't make sense as valid encodable types. (Though "Undefined" is defined, it's just no longer a valid type, so that one gets.... complicated).

TL;DR - The only truly "invalid" type is EOD.

Unless I'm misunderstanding the purpose of this method, I'd probably just reduce it to:

return bson_type != BSON_TYPE_EOD;

Further Reading: https://bsonspec.org/spec.html

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and modified this in order to get a test passing, feel free to change it back if necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, I didn't realize that [MAXKEY, MINKEY] are valid, I was wondering what those meant. Thanks for the clarification.

@marksg07 marksg07 marked this pull request as ready for review December 16, 2024 17:06
Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with minor comments.

@marksg07 marksg07 merged commit dfbfad8 into mongodb:master Dec 19, 2024
46 of 53 checks passed
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.

5 participants