TF1/TString possible overflow fix + Warning in TString by AdvaitDhingra · Pull Request #8426 · root-project/root

AdvaitDhingra · 2021-06-14T18:51:40Z

I have tried to fix this issue.

This should prevent a possible overflow in TF1, as now the min of the length of strlen(formula) and 5 is taken, therefore preventing a possible overflow.

Also in TString if the first n characters of cs conatin \0 it warns you.

phsft-bot · 2021-06-14T18:51:43Z

Can one of the admins verify this patch?

AdvaitDhingra · 2021-06-14T19:39:12Z

I somehow managed to mess up the commits in multiple of my PR's. Sorry for the inconvenience.

hageboeck · 2021-06-15T10:46:09Z

hist/hist/src/TF1.cxx

+   const auto formulaLength = strlen(formula);
   // First check if we are making a convolution
-   if (TString(formula, 5) == "CONV(" && formula[strlen(formula) - 1] == ')') {
+   if (TString(formula, 5) == "CONV(" && formula[formulaLength - 1] == ')') {


Here you also need the strncmp trick, because that's the line that triggered the error that started all this.

Look up "git interactive rebase". You can use it to fix the existing commits (git commit --fixup xxxx), and you can also remove the "Added some constants" "Revert Added some constants" while rebasing.
Once you get the hang of it, you will love it!

Ah yes, I forgot that. I will change it as soon as I'm home

Ahh thanks for the interactive rebase tip. I was looking for something like that 😀

AdvaitDhingra · 2021-06-15T16:39:28Z

Ah, there we go. git rebase -i HEAD~4 worked. Thanks @hageboeck :D

hageboeck

Now that you know how to rebase, could you edit the commit messages, please?
(git rebase -i origin/master) and then use e to edit the messages.

Use a short title, max 80 characters, e.g. "Add warning in TString constructor"
Then, after an empty line, explain in detail what you did and why. E.g. "Warn when a TString of a fixed length is constructed from an input string that's too short."

Like this, it will be easy to understand what happened when people are looking into these commits at some point in the future.

For the TF1 fix, explain what the bug was. Edit: And mention something like Fix #8136 in the commit message.

hist/hist/src/TF1.cxx

AdvaitDhingra · 2021-06-16T08:32:50Z

Now that you know how to rebase, could you edit the commit messages, please?
(git rebase -i origin/master) and then use e to edit the messages.

Use a short title, max 80 characters, e.g. "Add warning in TString constructor"

Then, after an empty line, explain in detail what you did and why. E.g. "Warn when a TString of a fixed length is constructed from an input string that's too short."

Like this, it will be easy to understand what happened when people are looking into these commits at some point in the future.

For the TF1 fix, explain what the bug was. Edit: And mention something like Fix #8136 in the commit message.

Okay. I will change it

AdvaitDhingra · 2021-06-16T13:51:53Z

Are these commit names better?

hageboeck · 2021-06-16T13:57:37Z

Are these commit names better?

A bit, but look at this commit for example:
6ddcc0d

You can use the message to explain a bit better what was going on e.g.:

[TF1] Fix buffer overflow.

TF1 was copying 5 characters from an incoming string without checking its length.
This was replaced by strncmp, which takes into account the length.

AdvaitDhingra · 2021-06-16T14:00:07Z

Are these commit names better?

A bit, but look at this commit for example:
6ddcc0d

You can use the message to explain a bit better what was going on e.g.:
[TF1] Fix buffer overflow.

TF1 was copying 5 characters from an incoming string without checking its length.
This was replaced by strncmp, which takes into account the length.

So in the interactive rebase I just write this detailed description under the commit? Because when I do that it tells me that that line is invalid. Do I need to use some special indentation?

hageboeck · 2021-06-16T14:05:29Z

Are these commit names better?

A bit, but look at this commit for example:
6ddcc0d
You can use the message to explain a bit better what was going on e.g.:
[TF1] Fix buffer overflow.

TF1 was copying 5 characters from an incoming string without checking its length.
This was replaced by strncmp, which takes into account the length.
So in the interactive rebase I just write this detailed description under the commit? Because when I do that it tells me that that line is invalid. Do I need to use some special indentation?

Ah, no! That's r for "reword". git will stop and let you edit the commit message. :-)

The user now gets a warning in the TString::TString constructor if the input string is shorter than the requested size.

AdvaitDhingra · 2021-06-16T14:35:55Z

I think I got it. Thank you @hageboeck for all of the help :D

hageboeck · 2021-06-16T14:37:08Z

Very nice! Interactive rebase is awesome once you learn how to use it! 👏

hageboeck · 2021-06-16T14:37:13Z

@phsft-bot build

phsft-bot · 2021-06-16T14:37:19Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

AdvaitDhingra · 2021-06-16T14:37:31Z

Very nice! Interactive rebase is awesome once you learn how to use it!

Yeah, it is. Feels like I've been living under a rock hehe

phsft-bot · 2021-06-16T15:01:19Z

Build failed on ROOT-performance-centos8-multicore/default.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Warnings:

[2021-06-16T14:38:59.129Z] /data/sftnight/workspace/root-pullrequests-build/root/core/base/src/TString.cxx:134:19: warning: comparison of integer expressions of different signedness: ‘size_t’ {aka ‘long unsigned int’} and ‘Ssiz_t’ {aka ‘int’} [-Wsign-compare]

phsft-bot · 2021-06-16T15:12:16Z

Build failed on ROOT-ubuntu16/nortcxxmod.
Running on sft-ubuntu-1604-1.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Warnings:

[2021-06-16T14:41:17.945Z] /mnt/build/workspace/root-pullrequests-build/root/core/base/src/TString.cxx:134:19: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

AdvaitDhingra · 2021-06-16T15:17:30Z

Are these warnings critical? Is there something I can do?

Axel-Naumann · 2021-06-16T15:31:48Z

Yes they need to be fixed. I could tell you - but what I'd recommend (look, we're teaching you here :-) ) is to copy the warning text, search the Internet, and pick the most helpful posting on StackOverflow: that'll explain you what to do. And it's a key pattern to become a successful developer, it's how all of us fix our warnings 🤣

AdvaitDhingra · 2021-06-16T15:40:11Z

Yes they need to be fixed. I could tell you - but what I'd recommend (look, we're teaching you here :-) ) is to copy the warning text, search the Internet, and pick the most helpful posting on StackOverflow: that'll explain you what to do. And it's a key pattern to become a successful developer, it's how all of us fix our warnings

Ahh hehe. Okay, I'll try to fix it :D . Thanks for the learning opportunity

phsft-bot · 2021-06-16T15:56:52Z

Build failed on mac11.0/cxx17.
Running on macphsft20.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Warnings:

[2021-06-16T14:59:31.970Z] /Users/sftnight/build/workspace/root-pullrequests-build/root/core/base/src/TString.cxx:134:19: warning: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'Ssiz_t' (aka 'int') [-Wsign-compare]

phsft-bot · 2021-06-16T16:13:09Z

Build failed on mac1014/python3.
Running on macitois21.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Warnings:

[2021-06-16T15:20:46.897Z] /Volumes/HD2/build/workspace/root-pullrequests-build/root/core/base/src/TString.cxx:134:19: warning: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'Ssiz_t' (aka 'int') [-Wsign-compare]

AdvaitDhingra · 2021-06-16T16:13:39Z

That should take care of that

phsft-bot · 2021-06-16T17:49:26Z

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Errors:

[2021-06-16T17:48:56.047Z] /home/sftnight/build/workspace/root-pullrequests-build/root/core/base/src/TString.cxx:134:31: error: expected ‘;’ before ‘{’ token
[2021-06-16T17:48:56.047Z] /home/sftnight/build/workspace/root-pullrequests-build/root/core/base/src/TString.cxx:137:27: error: expected ‘)’ before ‘;’ token
[2021-06-16T17:48:56.047Z] /home/sftnight/build/workspace/root-pullrequests-build/root/core/base/src/TString.cxx:138:11: error: ‘data’ was not declared in this scope

Warnings:

[2021-06-16T17:48:56.047Z] /home/sftnight/build/workspace/root-pullrequests-build/root/core/base/src/TString.cxx:134:8: warning: init-statement in selection statements only available with -std=c++17 or -std=gnu++17
[2021-06-16T17:48:56.047Z] /home/sftnight/build/workspace/root-pullrequests-build/root/core/base/src/TString.cxx:134:19: warning: statement has no effect [-Wunused-value]
[2021-06-16T17:48:56.047Z] /home/sftnight/build/workspace/root-pullrequests-build/root/core/base/src/TString.cxx:137:27: warning: suggest braces around empty body in an ‘if’ statement [-Wempty-body]
[2021-06-16T17:48:56.047Z] /home/sftnight/build/workspace/root-pullrequests-build/root/core/base/src/TString.cxx:137:10: warning: unused variable ‘data’ [-Wunused-variable]

hageboeck · 2021-06-17T06:44:37Z

@phsft-bot build

phsft-bot · 2021-06-17T06:44:44Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

Axel-Naumann

Please correct the whitespace issue. LGTM otherwise!

hist/hist/src/TF1.cxx

Axel-Naumann · 2021-06-17T09:26:01Z

Can you squash all commits but "Added Warning to TString constuctor" into one, such that this branch ends up having two commits? You can do that with interactive rebase, making all commits after the second (!) as "fixup", such that they just modify / update the second commit.

The length of formula is now chached and reused. Furthermore strncmp is used instead of TString for preformance reasons.

AdvaitDhingra · 2021-06-17T12:44:52Z

That should be all there is to format. At least according to clang-format

hageboeck · 2021-06-17T12:48:36Z

@phsft-bot build

phsft-bot · 2021-06-17T12:48:43Z

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

hageboeck · 2021-06-17T12:48:49Z

Looks good to me, let's wait for the build!

AdvaitDhingra · 2021-06-17T14:53:05Z

I think the builds were successful

hageboeck · 2021-06-18T07:58:07Z

I think the builds were successful

Yes, they were. Yesterday, when you wrote this message, Jenkins (the most important step) was still running.
Well done, thanks!

hageboeck · 2021-06-18T07:58:32Z

@Axel-Naumann please merge.

AdvaitDhingra requested review from Axel-Naumann, lmoneta and pcanal as code owners June 14, 2021 18:51

AdvaitDhingra force-pushed the tf1-issue branch 4 times, most recently from 1b529b2 to b3aa3b1 Compare June 14, 2021 19:32

eguiraud assigned Axel-Naumann and pcanal Jun 15, 2021

hageboeck reviewed Jun 15, 2021

View reviewed changes

AdvaitDhingra force-pushed the tf1-issue branch from 4c23076 to 0baf82c Compare June 15, 2021 16:38

hageboeck requested changes Jun 16, 2021

View reviewed changes

hist/hist/src/TF1.cxx Outdated Show resolved Hide resolved

AdvaitDhingra force-pushed the tf1-issue branch 2 times, most recently from 51ee9a6 to 6cd4ca7 Compare June 16, 2021 13:51

Added Warning to TString constuctor

b23d976

The user now gets a warning in the TString::TString constructor if the input string is shorter than the requested size.

AdvaitDhingra force-pushed the tf1-issue branch from 6cd4ca7 to 8e04aaf Compare June 16, 2021 14:34

hageboeck approved these changes Jun 16, 2021

View reviewed changes

lmoneta approved these changes Jun 16, 2021

View reviewed changes

Axel-Naumann requested changes Jun 17, 2021

View reviewed changes

hist/hist/src/TF1.cxx Outdated Show resolved Hide resolved

AdvaitDhingra force-pushed the tf1-issue branch 4 times, most recently from 3750978 to 435f546 Compare June 17, 2021 12:40

TF1 CONV if statement fix

6e76152

The length of formula is now chached and reused. Furthermore strncmp is used instead of TString for preformance reasons.

AdvaitDhingra force-pushed the tf1-issue branch from 39c949d to 6e76152 Compare June 17, 2021 12:44

Axel-Naumann approved these changes Jun 18, 2021

View reviewed changes

Axel-Naumann merged commit a9d2405 into root-project:master Jun 18, 2021

AdvaitDhingra deleted the tf1-issue branch June 18, 2021 18:15

Conversation

AdvaitDhingra commented Jun 14, 2021

Uh oh!

phsft-bot commented Jun 14, 2021

Uh oh!

AdvaitDhingra commented Jun 14, 2021

Uh oh!

hageboeck Jun 15, 2021

Choose a reason for hiding this comment

Uh oh!

hageboeck Jun 15, 2021

Choose a reason for hiding this comment

Uh oh!

AdvaitDhingra Jun 15, 2021

Choose a reason for hiding this comment

Uh oh!

AdvaitDhingra Jun 15, 2021

Choose a reason for hiding this comment

Uh oh!

AdvaitDhingra commented Jun 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hageboeck left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AdvaitDhingra commented Jun 16, 2021

Uh oh!

AdvaitDhingra commented Jun 16, 2021

Uh oh!

hageboeck commented Jun 16, 2021

Uh oh!

AdvaitDhingra commented Jun 16, 2021

Uh oh!

hageboeck commented Jun 16, 2021

Uh oh!

AdvaitDhingra commented Jun 16, 2021

Uh oh!

hageboeck commented Jun 16, 2021

Uh oh!

hageboeck commented Jun 16, 2021

Uh oh!

phsft-bot commented Jun 16, 2021

Uh oh!

AdvaitDhingra commented Jun 16, 2021

Uh oh!

phsft-bot commented Jun 16, 2021

Warnings:

Uh oh!

phsft-bot commented Jun 16, 2021

Warnings:

Uh oh!

AdvaitDhingra commented Jun 16, 2021

Uh oh!

Axel-Naumann commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AdvaitDhingra commented Jun 16, 2021

Uh oh!

phsft-bot commented Jun 16, 2021

Warnings:

Uh oh!

phsft-bot commented Jun 16, 2021

Warnings:

Uh oh!

AdvaitDhingra commented Jun 16, 2021

Uh oh!

phsft-bot commented Jun 16, 2021

Errors:

Warnings:

Uh oh!

hageboeck commented Jun 17, 2021

Uh oh!

phsft-bot commented Jun 17, 2021

Uh oh!

Axel-Naumann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AdvaitDhingra commented Jun 15, 2021 •

edited

Loading

hageboeck left a comment •

edited

Loading

Axel-Naumann commented Jun 16, 2021 •

edited

Loading