0

I just want to know is it a good way of programming style.

I know what is happening in this piece of code. look for the first occurrence of href save it next_next and then look for the first occurrence of "}" and save it end_marker.

Here my question is end_marker[-1] = '\0'; is needed? Because strstr, upon successful completion, strstr() shall return a pointer to the located string or a null pointer if the string is not found.

I know the endmarker '\0' is for string but don't know is it good to index the array in the negative number?

Code:

char *end_marker;
char *next_next = strstr(links_ptr, "href");
 if (next_next != NULL) {
     next_next += 7;
     end_marker= strstr(next_next, "}");
     end_marker[-1] = '\0'; // :)
}

EDIT: links_ptr contains this data

 "links": [
        {
            "rel": "next",
            "href": "https://www.randomstuff.com/blabla"
        }
          ]
  • 1
    In C unlike Python or whatever your background is, accessing the `-1` index isn't the last element. – Tony Tannous Aug 07 '17 at 09:04
  • `end_marker[-1]` is Undefined Behaviour in C – Nikolai Shalakin Aug 07 '17 at 09:04
  • Actually, I found this in legacy code and they are shipping lot of equipment with this piece of code and it runs without any warnings. –  Aug 07 '17 at 09:06
  • @NikolaiShalakin, I don't think it's UB. It will give you a warning as assignment makes integer from pointer without a cast. –  Aug 07 '17 at 09:07
  • 4
    It is well defined: https://stackoverflow.com/questions/3473675/are-negative-array-indexes-allowed-in-c as `E1[E2] is identical to (*((E1)+(E2)))`. – Andre Kampling Aug 07 '17 at 09:08
  • 1
    @NikolaiShalakin: Not necessarily. – alk Aug 07 '17 at 09:16
  • 1
    What kind of equipment is this? Sounds scary. – Jabberwocky Aug 07 '17 at 09:22
  • 1
    Just use a well tested JSON parser library. Do not reinvent the wheel. – alk Aug 07 '17 at 09:24
  • We are using YAJL. However, parsing library already has error check to see the downloaded data from the server is valid or not. –  Aug 07 '17 at 09:26
  • I got an answer from Andre Kampling link. I want to know the usage of negative number in array index. thanks. –  Aug 07 '17 at 09:28
  • 2
    Using a -1 as an index is perfectly correct, and your code is correct if all assumptions your code makes about the input string are fulfilled. But this is not robust code at all and `next_next += 7;` is rather hacky. – Jabberwocky Aug 07 '17 at 09:31
  • Yes, I felt like that, that is why I asked to get opinions from others, I'm rewriting that bit. I just want to know the suggestion, thanks for the valuable input. –  Aug 07 '17 at 09:34
  • If you are already using YAJL in your project, just use it also for extracting the "href" from the links_ptr buffer, that would be a clean solution. – Jabberwocky Aug 07 '17 at 09:38

3 Answers3

4

This usage of strstr assumes much about the input. Given input it doesn't expect, it can scan memory out of the string bounds, write to bad addresses, or try to dereference a null pointer.

If links_ptr is different - if it's part of user input or data downloaded on the internet - then it's a definite bug and security issue.

  • next_next += 7 assumes that strlen(next_next) >= 7. If the string is shorter you'll be scanning memory that doesn't belong to the string until the first '\0' or '}' is found.
  • if the previous scan finds '}' it will write '\0' to an unrelated address
  • if '}' isn't found, end_marker will be NULL and end_marker[-1] should crash
orip
  • 73,323
  • 21
  • 116
  • 148
  • It uses rest-api for data exchange from the server then it search for the content what it downloaded, links_ptr is json buffer. If it fail to download the data from server. –  Aug 07 '17 at 09:14
  • If it's a server response then it falls under "external input". That's a crash or security vulnerability waiting to happen. Technically speaking, you can rely on validation to make sure the input meets expectations, but why would you? Fixing this is pretty trivial. – orip Aug 07 '17 at 11:06
1

In C/C++, there's nothing evil in using a negative array index. In this way you are addressing the slot BEFORE the pointer represented by end_marker. However, you need to ensure that there's valid memory at this address.

SBS
  • 806
  • 5
  • 13
  • And how do you insure that ? – Tony Tannous Aug 07 '17 at 09:09
  • 1
    @TonyTannous - how do you ensure that *any* memory is valid? – Attie Aug 07 '17 at 09:13
  • 2
    This is ensured inside the program logic. When accessing end_marker[-1], the programmer is responsible for ensuring that this occurs only if end_marker is a pointer at least one slot after the base of the buffer he is using. – SBS Aug 07 '17 at 09:14
0

In this case it would be undefined behaviour, you should do

if (end_marker != NULL)
{
    end_marker[strlen(end_marker) - 1] = '\0';
}

Using negative number isnt good practice and you should do it. To do it you have to be sure there is still this array.

kocica
  • 6,412
  • 2
  • 14
  • 35