intel / idxd-config

Accel-config / libaccel-config
Other
56 stars 34 forks source link

Minor coverity complaints #4

Closed snits closed 2 years ago

snits commented 3 years ago

Coverity complains about not checking the return of sprintf in a couple of spots:

accfg_device_mdev_remove() accfg_create_mdev() add_device_mdev()

ramesh-thomas commented 2 years ago

Can we ignore them as not much can go wrong with sprintf ?

davejiang commented 2 years ago

Aren’t we supposed to avoid using sprintf because of buffer overflow issues?

On Aug 17, 2021, at 7:34 PM, Ramesh Thomas @.***> wrote:



Can we ignore them as not much can go wrong with sprintf ?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/intel/idxd-config/issues/4#issuecomment-900766211, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAWFNMEZNP6ZR2IYA6FS2X3T5ML3NANCNFSM477L3M6A. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email.

ramesh-thomas commented 2 years ago

@davejiang the sprintf calls are not using user inputs. It uses the sysfs attributes and other known values. The buffer size used is large enough to accommodate them so there is no risk of buffer overflow.

@snits are these the only Coverity complaints you get? If it cannot be marked as false positive and annoying I can add a check. Maybe it is aware of sprintf having the buffer overflow risk and encourages checking return value to verify number of bytes written doesn't exceed the buffer size. Wonder if it will still complain if it is replaced with snprintf.

snits commented 2 years ago

Looking at that coverity run again, it just looks like it is complaining because "Calling "sprintf" without checking return value (as is done elsewhere 23 out of 27 times)." It was just a minor thing that I was bringing up in case you wanted to clean them up.

Another coverity run for the rhel9 version of the package against the 3.2 source didn't mention it at all.