BUG: Integer underflow causes HUGE space() in sendGeneric()

[crankyoldgit]

A mesgtime of zero (or less than the elapsed time to send a message) tickled
a bug in sendGeneric() due to the subtraction of two unsigned values.

This bug isn't catchable with our current unit test setup.
This was discovered by "on hardware" testing.

FYI @roidayan That elegant solution wasn't so elegant after all.

[crankyoldgit]

I'm submitting this as is, as it is rather a show-stopping bug. I don't want to leave master broken for too long.

[roidayan]

you still avoid duplicating the rest of the function so i think its worth it.
if you don't like the if statement another way is using one line if statement like this:

uint32_t space_gap = elapsed > mesgtime ? gap : std::max(gap, mesgtime - elapsed);
space(space_gap);

[crankyoldgit]

Agreed. No doubt the de-duplication of the procedure was worth it. Just the road to heck is/was paved with good intentions. ;-)

I don't think changing the code to the one-liner actually saves any compiled code/space. The compiler should do that the optimal anyway. If it does save space, I'll change it. However I think the code is more readable to others with a if statement verses the conditional assignment.

[roidayan]

It doesn't have space I think. I didn't look at it that way just thought about reading code wise as I thought you didnt like the if-else statement. Another option is doing max first between the substraction and 0 so you won't get negative number. Leaving as is is of course fine :-) Thanks for the hard work!

…

On Tue, Jan 2, 2018, 11:12 David Conran ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/IRsend.cpp <#381 (comment)> : > @@ -340,7 +340,12 @@ void IRsend::sendGeneric(const uint16_t headermark, const uint32_t headerspace, // Footer if (footermark) mark(footermark); - space(std::max(gap, mesgtime - usecs.elapsed())); + uint32_t elapsed = usecs.elapsed(); + // Avoid potential unsigned integer underflow. e.g. when mesgtime is 0. + if (elapsed >= mesgtime) + space(gap); + else + space(std::max(gap, mesgtime - elapsed)); Agreed. No doubt the de-duplication of the procedure was worth it. Just the road to heck is/was paved with good intentions. ;-) I don't think changing the code to the one-liner actually saves any compiled code/space. The compiler should do that the optimal anyway. If it does save space, I'll change it. However I think the code is more readable to others with a if statement verses the conditional assignment. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#381 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAoSnvv7PtPvN4MVlOChjJHNBbioaGqOks5tGfMOgaJpZM4RIJex> .