driftregion / iso14229

ISO 14229 (UDS) server and client for embedded systems
MIT License
226 stars 79 forks source link

clean the memory of struct ifreq ifr and check the return value of ioctl for errors #19

Closed muehlke closed 1 year ago

muehlke commented 1 year ago

Hello @driftregion ,

I tested creating some UDS clients with different interface names like "can2" or something like "can1234" to be surprised that the client could still be created and that it could send over the only available interface "can0". Then I realized that the variable struct ifreq ifr in the function static int LinuxSockBind() in the file iso14229.c is declared but its memory is not cleared before using it. After that, the return value of the call to ioctl() is not checked for errors like in the calls to socket(), setsockopt(), and bind() within the same function.

I think it did work because before I did these tests I had chosen the right interface name "can0" so the memory of the struct got the right index for interface but in another call with a wrong interface name, the call to ioctl was not successful but since the uncleared memory still uncleared the valid index for "can0" the client could still be bound to the right interface. I changed the code to:

    struct ifreq ifr;
    memset(&ifr, 0, sizeof(ifr));
    strncpy(ifr.ifr_name, if_name, sizeof(ifr.ifr_name) - 1);
    if (ioctl(fd, SIOCGIFINDEX, &ifr) < 0) {
        printf("ioctl: %s %s\n", strerror(errno), if_name);
        return -1;
    }

and this prevents the problem and is a cleaner way of dealing with the struct and the call to ioctl. The warning message pattern was taken from the call to bind() after it.

I see that within the same function you use either perror or printf() to report errors, is there a reason for that? Wouldn't it be better to keep it consistent?

What do you think about this change?

driftregion commented 1 year ago

Hi @muehlke , thank you. failing to initialize struct ifreq is definitely a bug, as is not checking the return code of ioctl. Please check whether https://github.com/driftregion/iso14229/pull/20 fixes it.

I see that within the same function you use either perror or printf() to report errors, is there a reason for that? Wouldn't it be better to keep it consistent?

You're correct. I added a commit to #20 that reports errors on stderr. Please let me know if this works for you.