angstsmurf / spatterlight

Updated fork of Spatterlight
GNU General Public License v3.0
105 stars 5 forks source link

Remove hard-coded sizes from snprintf #65

Closed cspiegel closed 1 year ago

cspiegel commented 1 year ago

I was looking over the patches in more detail, and noticed that snprintf() is being called with a hard-coded size. This switches snprintf() calls to use sizeof to determine the target size.

In two places, there is a 1024-byte buffer, but the size "5" is hard-coded. I think the "5" was just accidentally used since previous buffer sizes are 5.

And in loaddatabase.cpp, the size was DirPathLength + 9 for one of the calls, but should just be the target size of the buffer (to ensure no overflow happens).

Finally, in pcdraw.c, namelength was being passed instead of the size of the target buffer, so I updated that.

angstsmurf commented 1 year ago

Huh. I assumed it was good to provide a hard-coded size when the length of the result is known in advance, so that we wouldn't have to waste cycles calculating it. If anything, the 1024 sized buffer is the mistake, because we know that the result will always be five bytes.

Would it be better to just set the bytes individually? buf[0] = type; buf[1] = '0'; and so on?

cspiegel commented 1 year ago

sizeof is computed at compile time, so it's just as efficient as hard-coding, but safer. Edit: I say safer because if you change the buffer size, you don't need to then remember to change the snprintf call.

Switching the 1024 to 5 is fine, definitely, if the result will always fit.

Unless it's a clear bottleneck, I think snprintf is the way to go. It's safe and makes the intent very obvious. Basically, it does all this work for you, and I love taking advantage of the system doing work so I don't have to.

angstsmurf commented 1 year ago

I made a new pull request over at the garglk repo. If that is fine I will commit it here as well.

cspiegel commented 1 year ago

OK, looks good, and I've merged things into garglk.

angstsmurf commented 1 year ago

A modified version of this is now merged as part of baca636. Thanks for this and for the other suggestions!