On Thu, 2023-03-30 at 16:11 +0000, John Morris wrote:
> Hi Reid!
> Some thoughts
> I was looking at lmgr/proc.c, and I see a potential integer overflow
> - bothmax_total_bkend_mem and result are declared as “int”, so the
> expression “max_total_bkend_mem * 1024 * 1024 - result * 1024 * 1024”
> could have a problem whenmax_total_bkend_mem is set to 2G or more.
> /*
> * Account for shared
> memory size and initialize
> *
> max_total_bkend_mem_bytes.
> */
>
> pg_atomic_init_u64(&ProcGlobal->max_total_bkend_mem_bytes,
>
> max_total_bkend_mem *
> 1024 * 1024 - result * 1024 * 1024);
>
>
> As more of a style thing (and definitely not an error), the calling
> code might look smoother if the memory check and error handling were
> moved into a helper function, say “backend_malloc”. For example, the
> following calling code
>
> /* Do not exceed maximum allowed memory allocation */
> if
> (exceeds_max_total_bkend_mem(Slab_CONTEXT_HDRSZ(chunksPerBlock)))
> {
> MemoryContextStats(TopMemoryContext);
> ereport(ERROR,
>
> (errcode(ERRCODE_OUT_OF_MEMORY),
>
> errmsg("out of memory - exceeds max_total_backend_memory"),
>
> errdetail("Failed while creating memory context \"%s\".",
>
> name)));
> }
>
> slab = (SlabContext *)
> malloc(Slab_CONTEXT_HDRSZ(chunksPerBlock));
> if (slab == NULL)
> ….
> Could become a single line:
> Slab = (SlabContext *)
> backend_malloc(Slab_CONTEXT_HDRSZ(chunksPerBlock);
>
> Note this is a change in error handling as backend_malloc() throws
> memory errors. I think this change is already happening, as the error
> throws you’ve already added are potentially visible to callers (to be
> verified). It also could result in less informative error messages
> without the specifics of “while creating memory context”. Still, it
> pulls repeated code into a single wrapper and might be worth
> considering.
>
> I do appreciate the changes in updating the global memory counter. My
> recollection is the original version updated stats with every
> allocation, and there was something that looked like a spinlock
> around the update. (My memory may be wrong …). The new approach
> eliminates contention, both by having fewer updates and by using
> atomic ops. Excellent.
>
> I also have some thoughts about simplifying the atomic update logic
> you are currently using. I need to think about it a bit more and will
> get back to you later on that.
>
> * John Morris
>
>
>
>
John,
Thanks for looking this over and catching this. I appreciate the catch
and the guidance.
Thanks,
Reid
