search

hapostgres / pg_auto_failover

Postgres extension and service for automated failover and high-availability
Other
1.09k stars 114 forks source link

Check return value of strdup calls #862

Closed danielgustafsson closed 2 years ago

danielgustafsson commented 2 years ago

Albeit unlikely on modern server systems, memory allocation can still fail and the result, if unchecked, be null pointer dereferences. This adds a check to affected callsites of strdup to ensure allocation was successful, or abort if not.

DimCitus commented 2 years ago

Hi @danielgustafsson ; thanks for your contribution!

Can you have a look at using ALLOCATION_FAILED_ERROR instead of the error format you've been using here? It's already used in a couple places: https://github.com/citusdata/pg_auto_failover/search?q=ALLOCATION_FAILED_ERROR&type=code.

danielgustafsson commented 2 years ago

Hi @danielgustafsson ; thanks for your contribution!

Can you have a look at using ALLOCATION_FAILED_ERROR instead of the error format you've been using here? It's already used in a couple places: https://github.com/citusdata/pg_auto_failover/search?q=ALLOCATION_FAILED_ERROR&type=code.

Oh nice, that's a much bette fit. I'll fix that in a rebase which also handles the silly typo I fat-fingered in.

DimCitus commented 2 years ago

Sorry about that, I missed it before. For clarity, I think I would prefer using value == NULL rather than !value in the tests. What do you think? I like code better when it's obvious, and == NULL can't be more obvious, I believe...

danielgustafsson commented 2 years ago

Sorry about that, I missed it before. For clarity, I think I would prefer using value == NULL rather than !value in the tests. What do you think? I like code better when it's obvious, and == NULL can't be more obvious, I believe...

I don't have strong opinions either way, so I've updated to patch to your preference.

danielgustafsson commented 2 years ago

Thanks!