How about adding a check for cc which is from configure file?

[eaglegai]

hi,
in pppd/plugins/pppoatm/ans.c, cc_len will read data from file /etc/e164_cc,
but it doesn't consider the situation that cc < 0 or cc > 300, which will cause out of bound reading or writting.
so if we shoud add a check about this?

and, cc_len\encode_e164\ans_byaddr is really useful? I can't find who use ans_byaddr in codes.

[Neustradamus]

@paulusmack, @enaess: Have you seen this ticket?

[enaess]

People should use libatm installed on their host instead of the provided version. I think the Debian pppd-2.5.0 package does this. Fedora or Red hat should follow suit. We can certainly take a fix for this, but in my opinion this code should probably be removed.

[eaglegai]

yes, i agree with @enaess

[enaess]

When I did the change to allow ./configure to use libatm1-dev on Debian, I never had a way to test it. The plugin loads in pppd so there shouldn't be any unresolved symbols. Would you be able to verify the functionality the configuration, i.e. compile pppd w/ pppoeatm.so and test it?

[eaglegai]

sorry, I just could compile ppp with linux-atm-libs-devel/linux-atm-libs which provide libatm.so , but don't know how to test it .

[enaess]

I don't know what you are working on and for whom? But generally, I'd recommend using a "better" maintained version of libatm.so than what is included in the pppd source tree. I have no way to validate this, but you'd see bugs coming in which pppd in ATM networks.

[eaglegai]

OK，I could't use libatm1-dev from debian, so get linux-atm-libs from fedora to compile ppp.
at last, will ppp remove the code instead of fixing it?

[enaess]

Whether or not we'll chose to remove it would be up to @paulusmack. We'd probably want a configure switch to enable the plugin if libatm.so is available on the system.

You'd have to use libatm.so provided by your distribution. If you are on Debian/Ubuntu, that would be the libatm1-dev package. If you are on Fedora/Redhat centric distributions, then you'd have to use the corresponding package (not sure what the package name would be).

I am not sure what you are trying to do or what your interest is in this project and to enable the pppoatm plugin. You packaging this for a distribution, or are you actually trying to enable a feature for ATM networks?

[eaglegai]

I just know we use ppp，but i don‘t know who use which funciton, so I have to pay attention to the whole ppp codes.
Thanks for your replies. :)

[paulusmack]

hi, in pppd/plugins/pppoatm/ans.c, cc_len will read data from file /etc/e164_cc, but it doesn't consider the situation that cc < 0 or cc > 300, which will cause out of bound reading or writting. so if we shoud add a check about this?

Yes, cc < 0 would be a problem. I don't see that 300 <= cc < 1000 would cause any issue. cc >= 1000 would, though.

and, cc_len\encode_e164\ans_byaddr is really useful? I can't find who use ans_byaddr in codes.

Right, it seems like those are dead code and could just be removed. (I have no particular expertise with ATM so I don't know if there is a better way to do all this these days.)

[enaess]

@paulusmack I think what we propose here is to via configure if libatm is present (or --with-libatm=), and if the library is present then build libpppoatm.so. With this change, nuke the libatm files imported into the tree. I know this would be a radical change, bug eventually I think we'd want the project to build against well known and maintained libraries of software. We'd want to use configure to detect the presence of these.

Are you proposing making a smaller change to remove just the functions that isn't used?

[Neustradamus]

@paulusmack: Have you seen the latest @enaess comment?

[paulusmack]

@paulusmack I think what we propose here is to via configure if libatm is present (or --with-libatm=), and if the library is present then build libpppoatm.so. With this change, nuke the libatm files imported into the tree. I know this would be a radical change, bug eventually I think we'd want the project to build against well known and maintained libraries of software. We'd want to use configure to detect the presence of these.

Sounds reasonable.

Are you proposing making a smaller change to remove just the functions that isn't used?

Yes, but your idea is better.

[enaess]

Just patching the small code would be less risk for 2.5.1 release. In the future, please consider removing the built-in libatm code drop.

[paulusmack]

I removed the unused code (ans_byaddr() and related functions).