Re: [PATCH] ip.7: Add not supported by SOCK_STREAM to socket options

[Alejandro Colomar <alx@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 10858 bytes --]

Hi Oliver,

On Sun, Mar 17, 2024 at 05:02:39AM -0400, Oliver Crumrine wrote:
> I just realized I had cc linked to a homebrew c compler on my system and
> that's why Wall and Wextra weren't giving me the same warnings they were
> giving you. Oops.
> 
> Anyway, I have put cc back to gcc, and I finally see the unused variable
> warnings, and I cleaned them up. 

Okay, with the programs below, and after some whitespace fixes, I see
the following diff of the servers, which sounds reasonable:

$ diff -u d.c s.c 
--- d.c	2024-03-17 14:27:26.476377947 +0100
+++ s.c	2024-03-17 14:27:36.800391759 +0100
@@ -24,7 +24,7 @@
 	struct sockaddr_in local_addr;
 	int s;
 	
-	s = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP);
+	s = socket(AF_INET, SOCK_STREAM, 0);
 	if (s == -1)
 		err(1, "error creating socket");
 	
@@ -40,8 +40,17 @@
 
 	if (bind(s, (struct sockaddr*)&local_addr, sizeof(local_addr)) == -1)
 		err(1, "error binding to port. try changing it or running as root");
+	
+	if (listen(s, 10) == -1) //10 is just an arbitrary number
+		err(1, "error listening on port");
 
 	while (1) {
+		int connfd = accept(s, (struct sockaddr*)NULL, NULL);
+		if (connfd == -1)
+			err(1, "error accepting connection");
+		if (setsockopt(s, IPPROTO_IP, SOCKOPT, &yes, sizeof(yes)) == -1) //stream sockets should have this set on the connected socket as well. I left it above for uniformity between the two programs.
+			err(1, "error setting socket option");
+	
 		struct msghdr mhdr;
 		struct iovec iov[1];
 		struct cmsghdr *cmhdr;
@@ -60,9 +69,9 @@
 		memset(databuf, 0, sizeof(databuf));	
 		
 		//this is blocking
-		int msglen = recvmsg(s, &mhdr, 0);
+		int msglen = recvmsg(connfd, &mhdr, 0);
 		if (msglen == -1)
-			err(1, "recvmsg");
+			err(1, "recvmsg\n");
 
 		cmhdr = CMSG_FIRSTHDR(&mhdr);
 		while (cmhdr) {
@@ -75,6 +84,7 @@
 		}
 		//print out the first byte of data recieved in hex. You can verify this in wireshark if you like.
 		printf("data read: %sbyte = %02X\n", databuf, tos);
+		close(connfd);
 	}
 
 	close(s);

> Peter said on the previous reply to this that netcat only worked for him
> when it was forced to IPv4, using the -4 option, so that may be the
> issue you are facing with the program.

And the programs behave as reported when adding -4.

> #include <stdio.h>
> #include <err.h>
> #include <string.h>
> #include <stdlib.h>
> #include <arpa/inet.h>
> #include <sys/socket.h>
> #include <unistd.h>
> 
> #define PORT 8888 //The port on which to send data
> #define ADDR "127.0.0.1" //The internet address to send packets to
> 
> int main(void){
> 	int s;
> 	struct sockaddr_in server_addr;
> 
> 	char buf[] = "testing 1 2 3\n";
> 
> 	s = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP);

Why use IPPROTO_UDP?  ip(7)'s SYNOPSIS uses 0.  Are there any other
protocols available with (AF_INET, SOCK_DGRAM)?

Now about the patch, it seems to miss IP_RETOPTS, which is also handled
in ip_cmsg_recv_offset()?  Or am I missing something?

Please resend the programs when you send v2 of the patch, not send the
client programs, and show a diff of both programs.

Have a lovely day!
Alex

> 	if(s == -1){
> 		err(1, "error creating socket");
> 	}
> 
> 	memset(&server_addr, 0, sizeof(server_addr));
> 	
> 	server_addr.sin_family = AF_INET;
> 	server_addr.sin_port = htons(PORT);
> 	if(inet_pton(AF_INET, ADDR, &server_addr.sin_addr) != 1){ //I realize I'm checking the return value differently here. If you read the man page for inet_pton, it'll make sense.
> 		err(1, "error converting network address");
> 	}
> 
> 	if(sendto(s, buf, strlen(buf), 0, (struct sockaddr*)&server_addr, sizeof(server_addr)) == -1){
> 		err(1, "error sending data");
> 	}
> 	
> 	close(s);
> }

> #include<stdio.h>
> #include<err.h>
> #include<string.h>
> #include<stdlib.h>
> #include<arpa/inet.h>
> #include<sys/socket.h>
> #include<unistd.h>
> 
> #define PORT 8888	//The port on which to listen for incoming data
> 
> 
> //Hi Alex,
> //These are the two lines that allow you to switch between the three socket options outlined in my patch
> //The socket options tell the kernel to add a control message (cmsg), allowing the program
> //to recieve the data it is requesting. The three options are: IP_RECVTOS for the type of service byte,
> //IP_RECVORIGDSTADDR for the orignial dst address, and IP_PKTINFO for some random packet info.
> #define SOCKOPT IP_RECVORIGDSTADDR
> //This field is synonymous with the above one. Valid options are: IP_TOS, IP_ORIGDSTADDR, and IP_PKTINFO
> #define RECIVEOPTION IP_ORIGDSTADDR
> 
> int main(void){
> 	struct sockaddr_in local_addr;	
> 	int s;
> 	
> 	s = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP);
> 	if (s == -1){
> 		err(1, "error creating socket");
> 	}
> 	
> 	memset(&local_addr, 0, sizeof(local_addr));
> 	
> 	local_addr.sin_family = AF_INET;
> 	local_addr.sin_port = htons(PORT);
> 	local_addr.sin_addr.s_addr = htonl(INADDR_ANY);
> 
> 	int yes = 1;
> 	if(setsockopt(s, IPPROTO_IP, SOCKOPT, &yes, sizeof(yes)) == -1){
> 		err(1, "error setting socket option");
> 	}
> 
> 
> 	if(bind(s, (struct sockaddr*)&local_addr, sizeof(local_addr) ) == -1){
> 		err(1, "error binding to port. try changing it or running as root");
> 	}
> 
> 	while(1){
> 		struct msghdr mhdr;
> 		struct iovec iov[1];
> 		struct cmsghdr *cmhdr;
> 		char control[1000];
> 		char databuf[BUFSIZ];
> 		unsigned char tos = 0;
> 
> 		mhdr.msg_name = &local_addr;
> 		mhdr.msg_namelen = sizeof(local_addr);
> 		mhdr.msg_iov = iov;
> 		mhdr.msg_iovlen = 1;
> 		mhdr.msg_control = &control;
> 		mhdr.msg_controllen = sizeof(control);
> 		iov[0].iov_base = databuf;
> 		iov[0].iov_len = sizeof(databuf);
> 		memset(databuf, 0, sizeof(databuf));	
> 		
> 		//this is blocking
> 		int msglen = recvmsg(s, &mhdr, 0);
> 		if (msglen == -1){
> 			err(1, "recvmsg");
> 		}
> 		cmhdr = CMSG_FIRSTHDR(&mhdr);
> 		while (cmhdr) {
> 			printf("cmsg recieved\n");
> 			if (cmhdr->cmsg_level == IPPROTO_IP && cmhdr->cmsg_type == RECIVEOPTION) {
> 				//read the byte recieved
> 				tos = ((unsigned char *)CMSG_DATA(cmhdr))[0];
> 			}
> 			cmhdr = CMSG_NXTHDR(&mhdr, cmhdr);
> 		}
> 		//print out the first byte of data recieved in hex. You can verify this in wireshark if you like.
> 		printf("data read: %sbyte = %02X\n", databuf, tos);		
> 		
> 	}
> 
> 	close(s);
> 	return 0;
> }

> #include <stdio.h>
> #include <err.h>
> #include <string.h>
> #include <stdlib.h>
> #include <arpa/inet.h>
> #include <sys/socket.h>
> #include <unistd.h>
> 
> #define PORT 8888 //The port on which to send data
> #define ADDR "127.0.0.1" //The internet address to send packets to
> 
> int main(void){
> 	int s;
> 	struct sockaddr_in server_addr;
> 
> 	char buf[] = "testing 1 2 3\n";
> 
> 	s = socket(AF_INET, SOCK_STREAM, 0);
> 	if(s == -1){
> 		err(1, "error creating socket");
> 	}
> 
> 	memset(&server_addr, 0, sizeof(server_addr));
> 	
> 	server_addr.sin_family = AF_INET;
> 	server_addr.sin_port = htons(PORT);
> 	if(inet_pton(AF_INET, ADDR, &server_addr.sin_addr) != 1){ // I realize I'm checking the return value differently here. If you read the man page for inet_pton, it'll make sense.
> 		err(1, "error converting network address");
> 	}
> 
> 	if(connect(s, (struct sockaddr*)&server_addr, sizeof(server_addr)) == -1){
> 		err(1, "error connecting");
> 	}
> 	if(send(s, buf, strlen(buf), 0) == -1){
> 		err(1, "error sending data");
> 	}
> 	
> 	close(s);
> }

> #include<stdio.h>
> #include<err.h>
> #include<string.h>
> #include<stdlib.h>
> #include<arpa/inet.h>
> #include<sys/socket.h>
> #include<unistd.h>
> 
> #define PORT 8888	//The port on which to listen for incoming data
> 
> 
> //Hi Alex,
> //These are the two lines that allow you to switch between the three socket options outlined in my patch
> //The socket options tell the kernel to add a control message (cmsg), allowing the program
> //to recieve the data it is requesting. The three options are: IP_RECVTOS for the type of service byte,
> //IP_RECVORIGDSTADDR for the orignial dst address, and IP_PKTINFO for some random packet info.
> #define SOCKOPT IP_RECVORIGDSTADDR
> //This field is synonymous with the above one. Valid options are: IP_TOS, IP_ORIGDSTADDR, and IP_PKTINFO
> #define RECIVEOPTION IP_ORIGDSTADDR
> 
> int main(void){
> 	struct sockaddr_in local_addr;
> 	int s;
> 	
> 	s = socket(AF_INET, SOCK_STREAM, 0);
> 	if (s == -1){
> 		err(1, "error creating socket");
> 	}
> 	
> 	memset(&local_addr, 0, sizeof(local_addr));
> 	
> 	local_addr.sin_family = AF_INET;
> 	local_addr.sin_port = htons(PORT);
> 	local_addr.sin_addr.s_addr = htonl(INADDR_ANY);
> 
> 	int yes = 1;
> 	if(setsockopt(s, IPPROTO_IP, SOCKOPT, &yes, sizeof(yes)) == -1){
> 		err(1, "error setting socket option");
> 	}
> 
> 
> 	if(bind(s, (struct sockaddr*)&local_addr, sizeof(local_addr) ) == -1){
> 		err(1, "error binding to port. try changing it or running as root");
> 	}
> 	
> 	if(listen(s, 10) == -1){ //10 is the backlog of un-accepted connections. its just an arbitrary number
> 		err(1, "error listening on port");
> 	}
> 
> 	while(1){
> 		int connfd = accept(s, (struct sockaddr*)NULL, NULL);
> 		if(connfd == -1){
> 			err(1, "error accepting connection");
> 		}
> 		if(setsockopt(s, IPPROTO_IP, SOCKOPT, &yes, sizeof(yes)) == -1){ //stream sockets should have this set on the connected socket as well. I left it above for uniformity between the two programs.
> 			err(1, "error setting socket option");
> 		}
> 	
> 		struct msghdr mhdr;
> 		struct iovec iov[1];
> 		struct cmsghdr *cmhdr;
> 		char control[1000];
> 		char databuf[BUFSIZ];
> 		unsigned char tos = 0;
> 
> 		mhdr.msg_name = &local_addr;
> 		mhdr.msg_namelen = sizeof(local_addr);
> 		mhdr.msg_iov = iov;
> 		mhdr.msg_iovlen = 1;
> 		mhdr.msg_control = &control;
> 		mhdr.msg_controllen = sizeof(control);
> 		iov[0].iov_base = databuf;
> 		iov[0].iov_len = sizeof(databuf);
> 		memset(databuf, 0, sizeof(databuf));	
> 		
> 		//this is blocking
> 		int msglen = recvmsg(connfd, &mhdr, 0);
> 		if (msglen == -1){
> 			err(1, "recvmsg\n");
> 		}
> 		cmhdr = CMSG_FIRSTHDR(&mhdr);
> 		while (cmhdr) {
> 			printf("cmsg recieved\n");
> 			if (cmhdr->cmsg_level == IPPROTO_IP && cmhdr->cmsg_type == RECIVEOPTION) {
> 				//read the byte recieved
> 				tos = ((unsigned char *)CMSG_DATA(cmhdr))[0];
> 			}
> 			cmhdr = CMSG_NXTHDR(&mhdr, cmhdr);
> 		}
> 		//print out the first byte of data recieved in hex. You can verify this in wireshark if you like.
> 		printf("data read: %sbyte = %02X\n", databuf, tos);	
> 		close(connfd);
> 	}
> 
> 	close(s);
> 	return 0;
> }


-- 
<https://www.alejandro-colomar.es/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]