On Fri, Dec 18, 2020 at 11:35:14AM +0200, Heikki Linnakangas wrote: > pg_cryptohash_create() is now susceptible to leaking memory in > TopMemoryContext, if the allocations fail. I think the attached should fix > it (but I haven't tested it at all).
Yeah, you are right here. If the second allocation fails the first one would leak. I don't think that your suggested fix is completely right though because it ignores that the callers of pg_cryptohash_create() in the backend expect an error all the time, so it could crash. Perhaps we should just bite the bullet and switch the OpenSSL and fallback implementations to use allocation APIs that never cause an error, and always return NULL? That would have the advantage to be more consistent with the frontend that relies in malloc(), at the cost of requiring more changes for the backend code where the _create() call would need to handle the NULL case properly. The backend calls are already aware of errors so that would not be invasive except for the addition of some elog(ERROR) or similar, and we could change the fallback implementation to use palloc_extended() with MCXT_ALLOC_NO_OOM. > BTW, looking at pg_cryptohash_ctx and pg_cryptohash_state, why do we need > two structs? They're both allocated and controlled by the cryptohash > implementation. It would seem simpler to have just one. Depending on the implementation, the data to track can be completely different, and this split allows to know about the resowner dependency only in the OpenSSL part of cryptohashes, without having to include this knowledge in neither cryptohash.h nor in the fallback implementation that can just use palloc() in the backend. -- Michael
signature.asc
Description: PGP signature
