search

memononen / nanosvg

Simple stupid SVG parser
zlib License
1.69k stars 357 forks source link

Avoid signed integer overflow in nsvg__RGBA() #207

Closed chrstphrchvz closed 2 years ago

chrstphrchvz commented 2 years ago

a is implicitly cast to (signed) int before shifting, and when int is 32-bit, it is undefined behavior to left-shift a by 24 when its most significant bit is 1. Error from UBSan (-fsanitize=shift-base):

nanosvgrast.h:975:41: runtime error: left shift of 255 by 24 places cannot be represented in type 'int'

Casting a to unsigned int before shifting avoids undefined behavior.

oehhar commented 2 years ago

Dear Christopher, thank you to be also active here. I suppose, it really needs an expert to understand this issue.

The subexpression is "a << 24". "a" is of type unsigned char. Thus, left operand is of type "unsigned char". Right operand ("24") is of type "int". Due to that, the result is of type "int".

So, would it be worth to specify the constants as of type unsigned int: "a << 24U" ? This looks prettier and may be done for all constants of the line ?

Thank you again, Harald

chrstphrchvz commented 2 years ago

Hello Harald,

I have identified similar issues in Tcl/Tk, the most recent example https://core.tcl-lang.org/tcl/info/0061c7a476 strongly resembles the issue here. Jan Nijtmans has been very helpful in getting these taken care of. If desired, I can propose this issue be fixed in Tk first.

Your suggestion of implicitly casting a to unsigned int by making the shift amount unsigned will also work. I understand there are likely various factors in deciding which style/approach is best: line/statement compactness, minimizing change invasiveness, consistency with existing code, best practices (see https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=87152374), etc.; but the most important might be to make it clear to others that it is for avoiding undefined behavior (short of leaving a comment saying so), and so I would not mind a more verbose approach.

oehhar commented 2 years ago

Dear Christopher, thank you for the explanation and valuable contributions, I appreciate! I suppose, memononen will make a decission here, when he has time. Tk and then tksvg will follow. I will take care of it.

Thank you, Harald

memononen commented 2 years ago

Thank you for the fix!