search

gnotclub / xst

st fork that uses Xresources and some pretty good patches
MIT License
536 stars 74 forks source link

Use snprintf instead of sprintf in xrdb_load. #160

Closed maybebyte closed 1 year ago

maybebyte commented 1 year ago

This section had a buffer overflow once before: https://github.com/gnotclub/xst/pull/7

actionless commented 1 year ago

but why?

maybebyte commented 1 year ago

but why?

Hi @actionless. If it feels like an unreasonable change, can you help me out by explaining why that is? I opened the pull request to test the waters and see what others' feedback would be.

In any case, I don't see an obvious way that sprintf could be a problem currently because of the way that the surrounding code functions. However, if the code were to change in such a way that the buffer could overflow, at least snprintf doesn't effectively assume an infinite size like sprintf does.

Can you explain your reasoning for wanting to keep it the way it is? I'd be happy to learn something new.

actionless commented 1 year ago

I don't see an obvious way that sprintf could be a problem currently because of the way that the surrounding code functions. However, if the code were to change in such a way that the buffer could overflow, at least snprintf doesn't effectively assume an infinite size like sprintf does.

because if somebody would change from 255 to longer number, let's say 1000 - this code would actually start behaving false-positive, transforming 1000 to 100, instead of failing with overflow

maybebyte commented 1 year ago

Hello, thank you for taking the time to write an explanation. I tested that change just to see what would happen, and it turns out xst fails with an arithmetic exception regardless of whether sprintf or snprintf is used. I will go ahead and close the pull request.