Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTTP: Fix the parsing ContentLength bug by http_parser #2851

Closed
wants to merge 4 commits into from
Closed

HTTP: Fix the parsing ContentLength bug by http_parser #2851

wants to merge 4 commits into from

Conversation

mingyang0921
Copy link

@mingyang0921 mingyang0921 commented Jan 6, 2022

BUG
Solve http_parser_execute, the buffer size passed includes the length of the body. This causes parser->content_length to be reset to 0 even if the Content-Length length is included. Modify it so that when passed, the length is the length of the HTTP header.

Summary

Please describe the summary for this PR.

Details

Add more details about this PR.


TRANS_BY_GPT3

解决http_parser_execute,传入的buffer size包含了body的长度。导致即使包含了Content-Length长度,to_read又把parser->content_length重置为0.
修改为传入时,长度为http头的长度
@codecov-commenter
Copy link

codecov-commenter commented Jan 6, 2022

Codecov Report

Merging #2851 (57f2308) into 4.0release (dc43a11) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff               @@
##           4.0release    #2851      +/-   ##
==============================================
- Coverage       60.22%   60.21%   -0.01%     
==============================================
  Files             121      121              
  Lines           51076    51083       +7     
==============================================
+ Hits            30758    30759       +1     
- Misses          20318    20324       +6     

| Impacted Files | Coverage Δ | |'

Translated to English while maintaining the markdown structure:

'| Impacted Files | Coverage Δ | |
|---|---|---|
| trunk/src/protocol/srs_service_http_conn.cpp | 93.30% <100.00%> (-1.08%) | ⬇️ |


Continue to review full report at Codecov.

Legend - Click here to learn more
Translated to English while maintaining the markdown structure:

'> Δ = absolute <relative> (impact), ø = not affected, ? = missing data

Powered by Codecov. Last update dc43a11...57f2308. Read the comment docs.

TRANS_BY_GPT3

@mingyang0921
Copy link
Author

mingyang0921 commented Jan 6, 2022

Using this to create an HTTP Callback Server, you can reproduce this issue:

#include<stdio.h>
#include<string.h>
#include<stdlib.h>
#include<unistd.h>
#include<sys/socket.h>
#include<sys/types.h>
#include<netdb.h>
#include<arpa/inet.h>
 
void * get_in_addr(struct sockaddr * sa)
{	
	if(sa->sa_family == AF_INET)	
	{		
		return &(((struct sockaddr_in *)sa)->sin_addr); 
	}		
	return &(((struct sockaddr_in6 *)sa)->sin6_addr); 
}
int main(int argc, char * argv[])
{	
	int status;	
	struct addrinfo hints, * res;	
	int listner; 			
	int  ret = 0;
	int  flag = 0;
	char  buffer[1024];
	char * testbuffer_all = "HTTP/1.1 200 OK, Success\r\nContent-Type: application/json\r\nAccept-Ranges: bytes\r\nContent-Length: 23\r\nDate: Fri, 31 Dec 2021 08:47:59 GMT\r\nServer: \r\n\r\n{\"code\": 0, \"data\": \"\"}";
	char * testbuffer = "HTTP/1.1 200 OK, Success\r\nContent-Type: application/json\r\nAccept-Ranges: bytes\r\nContent-Length: 23\r\nDate: Fri, 31 Dec 2021 08:47:59 GMT\r\nServer: \r\n\r\n";
	char * testbuffer_body = "{\"code\": 0, \"data\": \"\"}";
	
	memset(& hints, 0, sizeof hints);		
	hints.ai_family = AF_UNSPEC; 	
	hints.ai_socktype = SOCK_STREAM; 
	hints.ai_flags = AI_PASSIVE; 		
 
	char sever_port_default[100]="6666";
	char * p_sever_port = NULL;
 
	if(argc>=2){
		flag  = atoi(argv[1]);
	}
	p_sever_port = sever_port_default;
			
	printf("Sever port:%s\n",p_sever_port);
	
	status = getaddrinfo(NULL, p_sever_port , &hints, &res);	
	if(status != 0)	
	{		
		fprintf(stderr,"getaddrinfo error: %s\n",gai_strerror(status));	
	}		
	listner = socket(res->ai_family,res->ai_socktype, res->ai_protocol);	
	if(listner < 0 )	
	{		
		fprintf(stderr,"socket error: %s\n",gai_strerror(status));	
	}			
	int opt = 1;
	setsockopt(listner, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof (opt));
	status = bind(listner, res->ai_addr, res->ai_addrlen); 	
	if(status < 0)	
	{		
		fprintf(stderr,"bind: %s\n",gai_strerror(status));	
	}	
	status = listen(listner, 10); 	if(status < 0)	
	{		
		fprintf(stderr,"listen: %s\n",gai_strerror(status));	
	}			
	freeaddrinfo(res);			
	int new_conn_fd;	
	struct sockaddr_storage client_addr;	socklen_t addr_size;	
	char s[INET6_ADDRSTRLEN];
	addr_size = sizeof client_addr;		
	printf("I am now accepting connections ...\n");		
	while(1)
	{			
		new_conn_fd = accept(listner, (struct sockaddr *) & client_addr, &addr_size);			
		if(new_conn_fd < 0)		
		{			
			fprintf(stderr,"accept: %s\n",gai_strerror(new_conn_fd));			
			continue;		
		}			
		inet_ntop(client_addr.ss_family, get_in_addr((struct sockaddr *) &client_addr),s ,sizeof s);

		memset(buffer,0,sizeof(buffer));
		ret = recv(new_conn_fd,buffer,sizeof(buffer)-1,0);
		printf("ret=%d buffer[%s]\n",ret,buffer);
		printf("______________________________________\n");
		if(!flag){
		status = send(new_conn_fd,testbuffer, strlen(testbuffer),0);
			printf("ret=%d testbuffer[%s]\n",status,testbuffer);
		status = send(new_conn_fd,testbuffer_body, strlen(testbuffer_body),0);
			printf("ret=%d testbuffer[%s]\n",status,testbuffer_body);

		}else{
		status = send(new_conn_fd,testbuffer_all, strlen(testbuffer_all),0);
			printf("ret=%d testbuffer[%s]\n",status,testbuffer_all);

		}
		if(status == -1)		
		{			
			close(new_conn_fd);			
			_exit(4);		
		}			
	}	
	close(new_conn_fd);			
	return 0;
}

TRANS_BY_GPT3

@mingyang0921
Copy link
Author

mingyang0921 commented Jan 6, 2022

When using httpcallback in srs4.0, occasionally there is a timeout issue when using the IP of the local network or 127 IP, but there is no problem when changing to the address of other servers. Colleague gdb debugging also has no problem. After debugging st recv data, it is found that there is no problem when receiving only the http head in a single packet. However, if the packet contains both the head and body, a timeout issue occurs. It is found that when parsing the header, if it contains both the http head and body, the content_length will be changed to 0, causing the problem.

TRANS_BY_GPT3

@winlinvip winlinvip force-pushed the 4.0release branch 3 times, most recently from 6ac5da9 to 623ca78 Compare January 6, 2022 13:59
@winlinvip winlinvip changed the title Update srs_service_http_conn.cpp HTTP: Fix the http_parser parsing ContentLength bug Jan 7, 2022
@winlinvip winlinvip changed the title HTTP: Fix the http_parser parsing ContentLength bug HTTP: Fix the parsing ContentLength bug by http_parser Jan 7, 2022
@winlinvip
Copy link
Member

winlinvip commented Jan 7, 2022

👍 It should be because we were using the http_parser incorrectly, thanks for the patch.

TRANS_BY_GPT3

@winlinvip winlinvip self-assigned this Jan 7, 2022
@winlinvip winlinvip added the Bug It might be a bug. label Jan 7, 2022
@winlinvip winlinvip added this to the 4.0 milestone Jan 7, 2022
@winlinvip
Copy link
Member

winlinvip commented Jan 8, 2022

Please @duiniuluantanqin take a look and see if we made any mistakes in calling the http parser, thank you.

TRANS_BY_GPT3

@duiniuluantanqin
Copy link
Member

duiniuluantanqin commented Jan 9, 2022

After testing, the problem you mentioned does indeed exist. Thumbs up for your test case.

However, the solution needs to be reconsidered.

TRANS_BY_GPT3

mingyang0921 and others added 3 commits January 19, 2022 11:19
p_header_tail指针为连续两个/r/n/r/n的第一个,进行判断时需要去掉2个字节
用content_length被置为0,退出时,将body长度赋值回去
@@ -136,7 +136,7 @@ srs_error_t SrsHttpParser::parse_message_imp(ISrsReader* reader)
// @remark We shouldn't use on_body, because it only works for normal case, and losts the chunk header and length.
// @see https://github.com/ossrs/srs/issues/1508
if (p_header_tail && buffer->bytes() < p_body_start) {
for (const char* p = p_header_tail; p <= p_body_start - 4; p++) {
for (const char* p = p_header_tail - 2; p <= p_body_start - 4; p++) {

This comment was marked as resolved.

@@ -3258,6 +3258,7 @@ size_t http_parser_execute (http_parser *parser,
p += to_read - 1;

if (parser->content_length == 0) {
parser->content_length = p - body_mark + 1;

This comment was marked as resolved.

@winlinvip
Copy link
Member

winlinvip commented Jan 26, 2022

Thank you @mingyang0921 for reproducing and submitting the patch. The final solution can be found at #2888 (comment).

TRANS_BY_GPT3

@winlinvip winlinvip added the TransByAI Translated by AI/GPT. label Jul 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug It might be a bug. TransByAI Translated by AI/GPT.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants