Fixed strcasestr()

[iskraman]

• Source : sdp.c
• Problem : strncasecmp() function -> This function may fail to find matches on some platforms
• Reference : https://www.gnu.org/software/gnulib/manual/html_node/strcasestr.html
• It has caused problems in our system and it would be good to solve them in a safe way.
• My System info

$> gcc --version
gcc (Ubuntu 11.2.0-19ubuntu1) 11.2.0
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE

$> getconf -a | grep libc
GNU_LIBC_VERSION                   glibc 2.35

• Test code

// test code
#include <stdio.h>
#include <string.h>

int main() {
	char *data = "sha-256 F1:CD:64:51:BE:0C:5E:3E:81:60:9B:64:AA:4E:27:91:B9:B6:E4:59:29:AB:EB:A2:83:71:1C:9A:86:56:DC:BD";

	if(strcasestr(data, "sha-256 ") == data) {
		printf("Ok!\n");
	} else {
		printf("Failed!\n");
	}

	return 0;
}

$> ./a.out
Failed!

[januscla]

Thanks for your contribution, @iskraman! Please make sure you sign our CLA, as it's a required step before we can merge this.

[lminiero]

Problem : strncasecmp() function -> This function may fail to find matches on some platforms

Is strncasecmp a typo here? We use that one A LOT in Janus, while strcasestr is only used for that SHA check.

[lminiero]

@iskraman ping...

[iskraman]

@pong~ Hi~ Lorenzo. We were conducting a modular test based on Janus gateway's source. During the test process, it was found that the function of the strcasestr() function did not operate normally. And we find that _GNU_SOURCE must be defined to use this. Because strcasestr() function is a nonstandard extention. This can lead to unintended problems. It needs to be replaced by safe features supported by the standard. However, if you think there is no problem, you can reject my offer. I respect and like your team. Thank you. 2022년 10월 3일 (월) 오후 7:21, Lorenzo Miniero ***@***.***>님이 작성:

…

@iskraman <https://github.com/iskraman> ping... — Reply to this email directly, view it on GitHub <#3076 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIOL7IJEPXPSUBZ6ECK2MI3WBKXRZANCNFSM6AAAAAAQVEX7EI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[atoppi]

TL;DR: I agree with this proposal.
Before merging please:

1. fix also the other two occurrences of strcasestr in sdp.c
2. consider the empty char after sha-1 or sha-256 like in the original code
3. fix indentation

---

As @iskraman mentioned, being strcasestr not a standard function it needs the definition of _GNU_SOURCE in order to be correctly used.

If that macro is not defined, the shared C snippet will successfully build, but with the following warning:

test.c: In function ‘main’:
test.c:7:12: warning: implicit declaration of function ‘strcasestr’; did you mean ‘strcasecmp’? [-Wimplicit-function-declaration]
    7 |         if(strcasestr(data, "sha-256 ") == data) {
      |            ^~~~~~~~~~
      |            strcasecmp
test.c:7:41: warning: comparison between pointer and integer
    7 |         if(strcasestr(data, "sha-256 ") == data) {
      |

and the output will not be the expected one -> matching failed.

I didn't check, but I guess the compiler is replacing strcasestr with strcasecmp?
In environments with different std libraries that function could not exist at all, leading to building errors.

Since we are basically using strcasestr to check the presence of sha-1/sha-256 at the beginning of the string, replacing it with strncasecmp looks functionally equivalent to me.

[atoppi]

TL;DR: checked through gdb and assembly why this is happening, and I am still strongly convinced that a patch is needed here.

---

When _GNU_SOURCE is not defined the declaration of strcasestr is being skipped and the compiler expects int to be the return type of strcasestr, whereas it should be a pointer (long).
Linking is not failing though, since the standard library actually has the needed symbol so a binary is being successfully produced.

The actual return value of strcasestr is correct in the standard lib (checked going step by step), but the code is casting it to an int value when returning it.
Indeed the pointer returned by strcasestr 0x555555556008 becomes 0x55556008, hence the check failure.

This is confirmed by inspecting the assembly, due to the code fetching the result of strcasestr from eax (32 bit) and extending it to rax (64 bit) through the cdqe instruction, then using the extended value to perform the check:

        mov     eax, 0
        call    strcasestr                 ;     save the output in rax/eax
        cdqe                               ;     extend eax in rax
        cmp     QWORD PTR [rbp-8], rax     ;     compare rax and the ptr to the string

In the version with _GNU_SOURCE defined the rax register includes the return value of strcasestr and is used to check the matching:

        call    strcasestr                 ;     save the output in rax/eax
        cmp     QWORD PTR [rbp-8], rax     ;     compare rax and the ptr to the string

[lminiero]

@iskraman can you please address the feedback @atoppi provided, so that we can merge? Thanks!

Before merging please:

1. fix also the other two occurrences of `strcasestr` in `sdp.c`

2. consider the empty char after `sha-1` or `sha-256` like in the original code

3. fix indentation

[iskraman]

@lminiero @atoppi Please check if what you said is correct.

[atoppi]

please remove this empty line

[lminiero]

Thanks! Merging then (I'll remove the extra line myself). I'll also take care of porting the fix to 0.x too.

[tmatth]

BTW in case anyone was curious, calling strlen on a literal as done here on strlen("sha-1 ") gets compiled away to a mov of whatever the length (known at compile-time) is, which is cool:
https://godbolt.org/z/ETs1MzW7v

[lminiero]

@tmatth that's the kind of things that always gets @atoppi way too excited 😄

[iskraman]

thanks! @lminiero @atoppi