Skip to content

Conversation

@jsetje
Copy link
Collaborator

@jsetje jsetje commented Aug 7, 2025

This is a simple typo that is currently breaking http boot. When the interior loop goes looking for a second Content-Length field, will always match the first one, and if that's some other field things break.

@jsetje
Copy link
Collaborator Author

jsetje commented Aug 7, 2025

@dennis-tseng99 can you please confirm that I did not misunderstand your intent?

@jsetje jsetje requested a review from vathpela August 7, 2025 20:01
@rosslagerwall
Copy link
Contributor

This makes sense to me, though using nested loops seems unnecessarily complicated. Surely something like this would be better...

	/* Check the length of the file */
	buf_size_set = false;
	for (i = 0; i < rx_message.HeaderCount; i++) {
		if (!strcasecmp(rx_message.Headers[i].FieldName,
				(CHAR8 *)"Content-Length")) {
			UINT64 new_buf_size = ascii_to_int(rx_message.Headers[i].FieldValue);
			if (buf_size_set && new_buf_size != *buf_size) {
				perror(L"Content-Length is invalid\n");
				goto error;
			}
			*buf_size = new_buf_size;
			buf_size_set = true;
		}
	}

@jsetje
Copy link
Collaborator Author

jsetje commented Aug 8, 2025

I suspect you mean unnecessarily computationally complex, and yes doing one loop for each didn't strike me as ideal either, although I'm not sure there are enough items here to really make a difference.

@vathpela let me know what you prefer, I'd probably drop buf_size_set from the above code and just test new_buf_size for not being 0.

@vathpela
Copy link
Member

vathpela commented Aug 8, 2025

I think @rosslagerwall's way is fine.

This is a simple typo that is currently breaking http boot.
When the interior loop goes looking for a second Content-Length
field, will always match the first one, and if that's some other
field things break.

Instead of just fixing the typo this also just loops over all
the fields once rather than doing a loop for each one. Thanks
to Ross Lagerwall for pushing me to clean that up as well.

Signed-off-by: Jan Setje-Eilers <Jan.SetjeEilers@oracle.com>
@jsetje
Copy link
Collaborator Author

jsetje commented Aug 11, 2025

@rosslagerwall thanks for pushing to clean that up as well!

buf_size_set is kind of required since someone could spoof a 0 content length, and if they do, we probably want to catch that.

@marta-lewandowska
Copy link

Works for me on x86 and aarch64.

@vathpela vathpela merged commit 5481105 into rhboot:main Aug 13, 2025
50 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.

4 participants