Skip to content

dnn: add another color to Net::Impl::dump()#19035

Merged
opencv-pushbot merged 1 commit intoopencv:masterfrom
berak:fix_dnn_net_dump_colors
Dec 7, 2020
Merged

dnn: add another color to Net::Impl::dump()#19035
opencv-pushbot merged 1 commit intoopencv:masterfrom
berak:fix_dnn_net_dump_colors

Conversation

@berak
Copy link
Copy Markdown
Contributor

@berak berak commented Dec 7, 2020

resolves #19034

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
  • The PR is proposed to proper branch
  • There is reference to original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

}
}
string colors[] = {"#ffffb3", "#fccde5", "#8dd3c7", "#bebada", "#80b1d3", "#fdb462", "#ff4848", "#b35151"};
string colors[] = {"#ffffb3", "#fccde5", "#8dd3c7", "#bebada", "#80b1d3", "#fdb462", "#ff4848", "#b35151", "#b662ff"};
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to replace to std::vector<std::string> and check colorId < colors.size()?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, cppcheck found this here.

arent there static analyzers running over the code on the buildbots ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's done time to time, but not for each PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check is good idea!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so i'll add it ...

@berak berak force-pushed the fix_dnn_net_dump_colors branch 3 times, most recently from 47f9e9a to 83f45ae Compare December 7, 2020 16:28
@berak berak force-pushed the fix_dnn_net_dump_colors branch from 83f45ae to cf28b5e Compare December 7, 2020 16:59
Copy link
Copy Markdown
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you 👍

case DNN_TARGET_CUDA_FP16: out << "CUDA_FP16"; colorId = 6; break;
// don't use default:
}
CV_Assert(colorId < colors.size());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, this check can help in runtime only.
(no problem detection if there is no tests for this code)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, i'd also rather favour something, that throws a compile time error instead.

unfortunately, all i could come up with is this:

enum X {A,B,C,D,MAX_X};
std::array<string,MAX_X> {"A","B","C","D","E"};

(works if there are excessive strings, but stays silent if there are not enough)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to add "MAX" value into enum then please use this approach to hide that from documentation:

https://github.com/opencv/opencv/blob/4.5.0/modules/calib3d/include/opencv2/calib3d.hpp#L474-L476

@opencv-pushbot opencv-pushbot merged commit 40ca8f4 into opencv:master Dec 7, 2020
@berak berak deleted the fix_dnn_net_dump_colors branch December 7, 2020 22:25
@alalek alalek mentioned this pull request Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dnn: buffer overflow in Net::Impl::dump()

5 participants