Page MenuHomePhabricator

elm_spinner: crash on illegal format set issue fix.
ClosedPublic

Authored by shilpasingh on Feb 10 2016, 5:03 AM.

Details

Reviewers
cedric
Summary

Set any illegal format, spinner crashes, the format set to spinner
has to be valid hence a check is added initially itself to check for validity of
label formats.

Signed-off-by: Shilpa Singh <shilpa.singh@samsung.com>

Test Plan
  1. Set illegal format to spinner for e.g: elm_spinner_label_format_set(sp, "d");
  2. Run spinner demo

Crash is observed

Diff Detail

Repository
rELM core/elementary
Branch
Spinner_crash
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1286
Build 1351: arc lint + arc unit
shilpasingh updated this revision to Diff 8391.Feb 10 2016, 5:03 AM
shilpasingh retitled this revision from to elm_spinner: crash on illegal format set issue fix..
shilpasingh updated this object.
shilpasingh edited the test plan for this revision. (Show Details)
shilpasingh added subscribers: buds, govi.
shilpasingh updated this revision to Diff 8392.Feb 10 2016, 5:55 AM

Warning message added.

cedric accepted this revision.Feb 12 2016, 12:16 PM
cedric edited edge metadata.
This revision is now accepted and ready to land.Feb 12 2016, 12:16 PM
cedric closed this revision.Feb 12 2016, 12:16 PM

I think here should be added check for 'fmt' variable. Because code:

Evas_Object *spinner = elm_spinner_add(parent);
elm_spinner_label_format_set(spinner, "");

will cause segmentation fault.

jpeg added a subscriber: jpeg.Feb 14 2016, 11:40 PM

This feels a little insufficient. There should be one and only one number format character, that must be of type integer or floating point o, d, i, u, x, X or f, F.
If we pass in "%s %s %s" then we end up printing raw memory as a string.

_is_label_format_integer should probably return an error code if the format is not valid (ie. 0 or >1 format chars, or not a number format char).
Let's just be safe in case the format string can somehow be maliciously set by an attacker.

src/lib/elm_spinner.c
1295

No need to prefix with "Warning", this is already a warning message :) It could be ERR though.

@NikaWhite, elm_spinner_label_format_set(spinner, ""); will not crash as its en empty string and not a NULL string but yes this will crash elm_spinner_label_format_set(spinner, NULL); - fixing it in next patch

@jpeg, you are right, working on it, will raise a new patch soon along with @NikaWhite issue fixes as well.