Skip to content

FIX: Mark uart parameters and sp as non-const pointers.#611

Closed
micolous wants to merge 1 commit intoProxmark:masterfrom
micolous:uart-pointers
Closed

FIX: Mark uart parameters and sp as non-const pointers.#611
micolous wants to merge 1 commit intoProxmark:masterfrom
micolous:uart-pointers

Conversation

@micolous
Copy link
Contributor

Internally, the uart_* implementations treat their parameter as a pointer.

While the C compiler technically doesn't have a way to enforce that you're not modifying memory referred to by a const pointer, there's not a good reason to prohibit an implementation of uart from doing so, if it makes sense for that platform or environment.

PM3 also fairly consistently casts the value to a non-const pointer anyway.

This also explicitly sets sp to NULL, which should cause an uninitialized use of sp to dereference a null pointer (generally crashing with a segfault, as the program tries to reference unmapped memory), rather than reading some nearby program memory (and have undefined behaviour).

That will make it very obvious in a debugger, when something has gone wrong. For example:

* thread #1: tid = 32727, 0x0000555555556e1b flasher`uart_send(sp=0x0000000000000000, pbtTx="", szTxLen=544) + 123 at uart_posix.c:204, name = 'flasher', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
    frame #0: 0x0000555555556e1b flasher`uart_send(sp=0x0000000000000000, pbtTx="", szTxLen=544) + 123 at uart_posix.c:204

This should address a number of compiler warnings on uart files.

Internally, the `uart_*` implementations treat their parameter as a pointer.

While the C compiler technically doesn't have a way to enforce that you're
not modifying memory referred to by a `const` pointer, there's not a good
reason to prohibit an implementation of `uart` from doing so, if it makes
sense for that platform or environment.

PM3 also fairly consistently casts the value to a non-const pointer anyway.

This also explicitly sets `sp` to NULL, which should cause an uninitialized
use of `sp` to dereference a null pointer (generally crashing with a
segfault, as the program tries to reference unmapped memory), rather
than reading some nearby program memory (and have undefined behaviour).

That will make it very obvious in a debugger, when something has gone wrong.
For example:

```
* thread iceman1001#1: tid = 32727, 0x0000555555556e1b flasher`uart_send(sp=0x0000000000000000, pbtTx="", szTxLen=544) + 123 at uart_posix.c:204, name = 'flasher', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
    frame #0: 0x0000555555556e1b flasher`uart_send(sp=0x0000000000000000, pbtTx="", szTxLen=544) + 123 at uart_posix.c:204
```

This should address a number of compiler warnings on uart files.
@pwpiwi
Copy link
Contributor

pwpiwi commented May 28, 2018

I am not sure about what you want to achieve here. What kind of warnings do you get? serial_port is already a pointer, so there is no reason to define a pointer to it. If you need functions to modify *serial_port, then just remove the const in your implementation of the uart functions.

Please don't expect that the upstream repository always suits your needs. Someone may fiddle with the comms or uart functions in the future and will have no idea that this would impact your downstream repository. You already have your own uart_ functions. You just need to add your own functions to replace comms.c (although you can copy most of it) and you are done as long as the API doesn't change.

However, if this are just small modifications then add some #ifdefs and raise a PR. We might as well come back to a common uart.c with #ifdefs for Windows, Unix, and Android.

@micolous
Copy link
Contributor Author

micolous commented May 31, 2018

I got mixed up because while serial_port is actually a void* (which is exactly what I want), serial_port_windows and serial_port_unix are both structs.

I did not intend it to be a void**. Oops.

This "issue" had been bothering me a while, and I was honestly surprised you hadn't found it yet. But it's good to get closure. As I have unintentionally made it worse with this PR, I'm retracting this.

Thanks!

@micolous micolous closed this May 31, 2018
@micolous micolous deleted the uart-pointers branch May 31, 2018 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants