Avoid ABEND when receiving request content and chunks

[ChongZhou-Broadcom]

Proposed changes

When processHttpFragment is receiving the request content, either the first block or the subsequent chunks, it calls SLHAlloc to allocate buffer. However, SLHAlloc will trigger ABEND when the preallocated short-lived buffer runs out of space. For example:

zowe-common-c/c/httpserver.c

Lines 2390 to 2395 in 9803cfc

	parser->content = SLHAlloc(parser->slh, parser->specifiedContentLength + parser->chunkSize);
	if (parser->content == NULL) {
	/* allocation failure */
	parser->httpReasonCode = 500;
	return 0;
	}

zowe-common-c/c/utils.c

Lines 1495 to 1513 in 9803cfc

	char *SLHAlloc(ShortLivedHeap *slh, int size){
	/* expand for fullword alignment */
	int rem = size & 0x7;
	if (rem != 0){
	size += (8-rem);
	}
	char *data;
	/*
	printf("slh=0x%p\n",slh);fflush(stdout);
	printf("SLHAlloc me=0x%p size=%d bc=%d\n",slh,size,slh->blockCount);fflush(stdout);
	*/
	int remainingHeapBytes = (slh->blockSize * (slh->maxBlocks - slh->blockCount));
	if (size > remainingHeapBytes){
	printf("SLH at 0x%p cannot allocate above block size %d > %d mxbl %d bkct %d bksz %d\n",
	slh,size,remainingHeapBytes,slh->maxBlocks,slh->blockCount,slh->blockSize);
	fflush(stdout);
	char *mem = (char*)0;
	mem[0] = 13;
	return NULL;

It makes DDoS possible. This PR changes the behavior to return http/500 instead of ABEND when it fails to allocate buffer for request content.

However, I did not implement a configurable limit for the max content size, I only avoided ABEND. I was hesitating because it sounds like a good idea but the HTTP spec does not force such limit.

This PR addresses Issue: N/A

This PR depends upon the following PRs: N/A

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Change in a documentation
• Refactor the code
• Chore, repository cleanup, updates the dependencies.
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

PR Checklist

Please delete options that are not relevant.

• If the changes in this PR are meant for the next release / mainline, this PR targets the "staging" branch.
• My code follows the style guidelines of this project (see: Contributing guideline)
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation (N/A)
• New and existing unit tests pass locally with my changes (N/A)
• video or image is included if visual changes are made (N/A)
• Relevant update to CHANGELOG.md (N/A)
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works, or describe a test method below (N/A)

Testing

N/A

Further comments

N/A

[JoeNemo]

I prefer that this becomes SLHAlloc2, by our current standard, adding an argument and having the original API function call a new one that has the new argument determine if the call should abort vs. return NULL. Default is abort. Also, is this code being called in a recovery.c recovery context. That would handle the abort already with no DDOS. AND when we have bugs that assert potential memory problems or DDOS, please make new files or subdirs under zowe-common-c/tests to repro and allow future regression testing.

[ChongZhou-Broadcom]

@JoeNemo Thank you for your review! My response is as below.

I prefer that this becomes SLHAlloc2, by our current standard, adding an argument and having the original API function call a new one that has the new argument determine if the call should abort vs. return NULL. Default is abort.

Totally agreed. Will change it soon.

Also, is this code being called in a recovery.c recovery context. That would handle the abort already with no DDOS.

Please correct me if I'm wrong. The only recoveryPush ... recoveryPop section in httpserver.c is found in handleHttpService. According to serviceLoop, processHttpFragment is not called inside handleHttpService but before it, which means ABENDs happen in processHttpFragment is not recoverable.

AND when we have bugs that assert potential memory problems or DDOS, please make new files or subdirs under zowe-common-c/tests to repro and allow future regression testing.

Will do.

[ifakhrutdinov]

I think we should just factor out the logic out of SLHAlloc into something like SLHAllocInternal and then modify the behaviour in the wrapper functions (SLHAlloc and e.g., SLHAlloc2).

This kind of leaks abstraction.

[ChongZhou-Broadcom]

Yes, I agree. @JoeNemo suggested something similar. I have updated the implementation.

[ChongZhou-Broadcom]

@JoeNemo I implemented the "SLHAlloc2" according to your suggestion. I created "SLHAlloc_NoAbend" because I was thinking about c++'s "new(std::nothrow)" :)

However, I didn't implement the test. To be honest, I created a test but I don't know how to build and run it. The code change is in processHttpFragment in httpserver.c, so if I want to unit test that function, I nearly have to rebuild a whole zss.

[ChongZhou-Broadcom]

@JoeNemo Hi Joe, as you requested earlier, I added two test cases in tests/. Please review it when you have a moment. Thanks.

[sonarqubecloud]

Quality Gate failed

Failed conditions
4 Security Hotspots

See analysis details on SonarQube Cloud