Perhaps we should restrict expand_number() to decimal as @jhb suggested in D20796...

Given how widely used expand_number(3) is, I think it's best to continue to support other bases, but not document that they are supported.

Hmm, the ctld code expects an unsigned value in the functions that are later called, so negative values are getting silently accepted when they are wrong. Probably all of the changes in parse.y need to fail if tmp < 0.

I'm surprised you aren't seeing other build failures. libvmmapi passes a size_t * to expand_number. In fact, pretty much every caller in the tree will have to be fixed to switch to signed arguments and probably reject negative values if they are not valid?

In D51542#1177245, @jhb wrote:

Hmm, the ctld code expects an unsigned value in the functions that are later called, so negative values are getting silently accepted when they are wrong. Probably all of the changes in parse.y need to fail if tmp < 0.

I'm surprised you aren't seeing other build failures. libvmmapi passes a size_t * to expand_number. In fact, pretty much every caller in the tree will have to be fixed to switch to signed arguments and probably reject negative values if they are not valid?

I wonder if there's a way we could stage this change. Maybe we could do something crazy with C _Generic where we provide a wrapper that takes uint64_t * and have it call expand_number but then fail with ERANGE if the result is negative? Then you could have expand_number be a wrapper macro that uses _Generic to pick the appropriate version based on the type of the second argument? That would allow for a staged transition which might be better.

I think something should be declared an ABI change only when there is no other way around. In particular, putting the consequences of bumping libutil version on everybody when it is only something for supposedly invalid input might be too much.

BTW, the idea with _Generic and dispatching to signed/unsigned variants of the function might be quite good if it can be implemented. I would even suggest to do it not as stage, but keep that.

In D51542#1177245, @jhb wrote:

Hmm, the ctld code expects an unsigned value in the functions that are later called, so negative values are getting silently accepted when they are wrong. Probably all of the changes in parse.y need to fail if tmp < 0.

No, in many cases the [u]int64_t value is passed without further checks to a function that takes an int (e.g. conf_set_debug(), conf_set_isns_period(), conf_set_isns_timeout(), conf_set_maxproc()) and doesn't validate its input. I agree that additional validation is needed, but this is a preexisting problem.

Unfortunately libutil doesn't have symbol versioning (but see D51694), otherwise I could have kept a copy of the unsigned implementation around for compatibility.

In D51542#1180747, @des wrote:

Unfortunately libutil doesn't have symbol versioning (but see D51694), otherwise I could have kept a copy of the unsigned implementation around for compatibility.

Err, I think the idea was to have 'expand_number_unsigned` as a new symbol (or even, just expand_number_signed as a new symbol) and then use a _Generic wrapper like we do for qsort_r that picks the correct underlying function based on the type of the second argument. I still don't understand how this currently compiles as all the other places in world pass a pointer to an unsigned variable. Maybe they just don't have the warning enabled? I think the _Generic thing is still worth doing as @kib notes.

See D51723